Convert LayoutTestControl::CompositeWithRaster to asynchronous

Previous to this CL, there were corner case crashes occurring when another
call to LocalFrameView::UpdateLifecyclePhases needed to call synchronously
back to the browser process, for example to gather Font information. In
those cases, if CompositeWithRaster was called (synchronously), it would
be allowed to run by Mojo, which would cause a re-entrant call to
LocalFrameView::UpdateLifecyclePhases. By converting this method to
asynchronous, Mojo will block it and wait for existing synchronous calls
to complete, avoiding the re-entrancy.

Bug: 877093
Change-Id: Ide4036f485662bc576aaf661fb227f59955ac026
Reviewed-on: https://chromium-review.googlesource.com/c/1252141Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595975}

content/shell/browser/layout_test/blink_test_controller.cc

@@ -511,6 +511,8 @@ bool BlinkTestController::ResetAfterLayoutTest() {
   main_frame_dump_ = nullptr;
   waiting_for_pixel_results_ = false;
   waiting_for_main_frame_dump_ = false;
+  composite_all_frames_node_storage_.clear();
+  composite_all_frames_node_queue_ = std::queue<Node*>();
   weak_factory_.InvalidateWeakPtrs();
 
 #if defined(OS_ANDROID)
@@ -603,15 +605,12 @@ void BlinkTestController::OnInitiateCaptureDump(bool capture_navigation_history,
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kEnableThreadedCompositing)) {
       rwhv->EnsureSurfaceSynchronizedForLayoutTest();
+      EnqueueSurfaceCopyRequest();
     } else {
-      CompositeAllFrames();
+      CompositeAllFramesThen(
+          base::BindOnce(&BlinkTestController::EnqueueSurfaceCopyRequest,
+                         weak_factory_.GetWeakPtr()));
     }
-
-    // Enqueue an image copy output request.
-    rwhv->CopyFromSurface(
-        gfx::Rect(), gfx::Size(),
-        base::BindOnce(&BlinkTestController::OnPixelDumpCaptured,
-                       weak_factory_.GetWeakPtr()));
   }
 
   RenderFrameHost* rfh = main_window_->web_contents()->GetMainFrame();
@@ -621,33 +620,71 @@ void BlinkTestController::OnInitiateCaptureDump(bool capture_navigation_history,
                      weak_factory_.GetWeakPtr()));
 }
 
-void BlinkTestController::CompositeAllFrames() {
-  std::vector<Node> node_storage;
-  Node* root = BuildFrameTree(main_window_->web_contents()->GetAllFrames(),
-                              &node_storage);
+// Enqueue an image copy output request.
+void BlinkTestController::EnqueueSurfaceCopyRequest() {
+  auto* rwhv = main_window_->web_contents()->GetRenderWidgetHostView();
+  rwhv->CopyFromSurface(
+      gfx::Rect(), gfx::Size(),
+      base::BindOnce(&BlinkTestController::OnPixelDumpCaptured,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void BlinkTestController::CompositeAllFramesThen(
+    base::OnceCallback<void()> callback) {
+  // Start with fresh storage and queue.
+  DCHECK(composite_all_frames_node_storage_.empty())
+      << "Attempted to composite twice in one test";
+  DCHECK(composite_all_frames_node_queue_.empty())
+      << "Attempted to composite twice in one test";
+  Node* root = BuildFrameTree(main_window_->web_contents()->GetAllFrames());
+  BuildDepthFirstQueue(root);
+  // Now asynchronously run through the node queue.
+  CompositeNodeQueueThen(std::move(callback));
+}
+
+void BlinkTestController::CompositeNodeQueueThen(
+    base::OnceCallback<void()> callback) {
+  RenderFrameHost* next_node_host;
+  do {
+    if (composite_all_frames_node_queue_.empty()) {
+      // Done with the queue - call the callback.
+      std::move(callback).Run();
+      return;
+    }
+    next_node_host =
+        composite_all_frames_node_queue_.front()->render_frame_host;
+    composite_all_frames_node_queue_.pop();
+  } while (!next_node_host || !next_node_host->IsRenderFrameLive());
+  GetLayoutTestControlPtr(next_node_host)
+      ->CompositeWithRaster(
+          base::BindOnce(&BlinkTestController::CompositeNodeQueueThen,
+                         weak_factory_.GetWeakPtr(), std::move(callback)));
+}
 
-  mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_calls;
-  CompositeDepthFirst(root);
+void BlinkTestController::BuildDepthFirstQueue(Node* node) {
+  for (auto* child : node->children)
+    BuildDepthFirstQueue(child);
+  composite_all_frames_node_queue_.push(node);
 }
 
 BlinkTestController::Node* BlinkTestController::BuildFrameTree(
-    const std::vector<RenderFrameHost*>& frames,
-    std::vector<Node>* storage) const {
+    const std::vector<RenderFrameHost*>& frames) {
   // Ensure we don't reallocate during tree construction.
-  storage->reserve(frames.size());
+  composite_all_frames_node_storage_.reserve(frames.size());
 
   // Returns a Node for a given RenderFrameHost, or nullptr if doesn't exist.
-  auto node_for_frame = [storage](RenderFrameHost* rfh) {
+  auto node_for_frame = [this](RenderFrameHost* rfh) {
     auto it = std::find_if(
-        storage->begin(), storage->end(),
+        composite_all_frames_node_storage_.begin(),
+        composite_all_frames_node_storage_.end(),
         [rfh](const Node& node) { return node.render_frame_host == rfh; });
-    return it == storage->end() ? nullptr : &*it;
+    return it == composite_all_frames_node_storage_.end() ? nullptr : &*it;
   };
 
   // Add all of the frames to storage.
   for (auto* frame : frames) {
     DCHECK(!node_for_frame(frame)) << "Frame seen multiple times.";
-    storage->emplace_back(frame);
+    composite_all_frames_node_storage_.emplace_back(frame);
   }
 
   // Construct a tree rooted at |root|.
@@ -668,14 +705,6 @@ BlinkTestController::Node* BlinkTestController::BuildFrameTree(
   return root;
 }
 
-void BlinkTestController::CompositeDepthFirst(Node* node) {
-  if (!node->render_frame_host->IsRenderFrameLive())
-    return;
-  for (auto* child : node->children)
-    CompositeDepthFirst(child);
-  GetLayoutTestControlPtr(node->render_frame_host)->CompositeWithRaster();
-}
-
 bool BlinkTestController::IsMainWindow(WebContents* web_contents) const {
   return main_window_ && web_contents == main_window_->web_contents();
 }

content/shell/browser/layout_test/blink_test_controller.h

@@ -250,11 +250,20 @@ class BlinkTestController : public WebContentsObserver,
   void OnCaptureDumpCompleted(mojom::LayoutTestDumpPtr dump);
   void OnPixelDumpCaptured(const SkBitmap& snapshot);
   void ReportResults();
-
-  void CompositeAllFrames();
-  Node* BuildFrameTree(const std::vector<RenderFrameHost*>& frames,
-                       std::vector<Node>* storage) const;
-  void CompositeDepthFirst(Node* node);
+  void EnqueueSurfaceCopyRequest();
+
+  // CompositeAllFramesThen() first builds a frame tree based on
+  // frame->GetParent(). Then, it builds a queue of frames in depth-first order,
+  // so that compositing happens from the leaves up. Finally,
+  // CompositeNodeQueueThen() is used to composite one frame at a time,
+  // asynchronously, continuing on to the next frame once each composite
+  // finishes. Once all nodes have been composited, the final callback is run.
+  // Each call to CompositeWithRaster() is an asynchronous Mojo call, to avoid
+  // reentrancy problems.
+  void CompositeAllFramesThen(base::OnceCallback<void()> callback);
+  Node* BuildFrameTree(const std::vector<RenderFrameHost*>& frames);
+  void CompositeNodeQueueThen(base::OnceCallback<void()> callback);
+  void BuildDepthFirstQueue(Node* node);
 
   std::unique_ptr<BlinkTestResultPrinter> printer_;
 
@@ -329,6 +338,9 @@ class BlinkTestController : public WebContentsObserver,
   bool waiting_for_pixel_results_ = false;
   bool waiting_for_main_frame_dump_ = false;
 
+  std::vector<Node> composite_all_frames_node_storage_;
+  std::queue<Node*> composite_all_frames_node_queue_;
+
   // Map from one frame to one mojo pipe.
   std::map<GlobalFrameRoutingId, mojom::LayoutTestControlAssociatedPtr>
       layout_test_control_map_;

content/shell/common/layout_test.mojom

@@ -51,7 +51,7 @@ struct LayoutTestDump {
 interface LayoutTestControl {
   CaptureDump() => (LayoutTestDump result);
 
-  [Sync] CompositeWithRaster() => ();
+  CompositeWithRaster() => ();
 
   // Dumps the frame's contents into a string.
   DumpFrameLayout() => (string frame_layout_dump);

mojo/public/cpp/bindings/sync_call_restrictions.h

@@ -21,10 +21,6 @@ namespace sync_preferences {
 class PrefServiceSyncable;
 }
 
-namespace content {
-class BlinkTestController;
-}
-
 namespace leveldb {
 class LevelDBMojoProxy;
 }
@@ -96,8 +92,6 @@ class MOJO_CPP_BINDINGS_EXPORT SyncCallRestrictions {
   // For destroying the GL context/surface that draw to a platform window before
   // the platform window is destroyed.
   friend class viz::HostFrameSinkManager;
-  // Allow for layout test pixel dumps.
-  friend class content::BlinkTestController;
   // For preventing frame swaps of wrong size during resize on Windows.
   // (https://crbug.com/811945)
   friend class ui::HostContextFactoryPrivate;

third_party/WebKit/LayoutTests/TestExpectations

@@ -2648,6 +2648,7 @@ crbug.com/605059 [ Retina ] fast/text/international/rtl-negative-letter-spacing.
 crbug.com/654477 [ Win ] compositing/video/video-controls-layer-creation.html [ Pass Failure ]
 crbug.com/654477 fast/hidpi/video-controls-in-hidpi.html [ Failure ]
 crbug.com/654477 fast/layers/video-layer.html [ Failure ]
+# These could likely be removed - see https://chromium-review.googlesource.com/c/chromium/src/+/1252141
 crbug.com/654477 media/controls-focus-ring.html [ Failure ]
 crbug.com/654477 virtual/video-surface-layer/media/controls-focus-ring.html [ Failure ]
 crbug.com/654477 [ Mac10.10 Mac10.11 Retina ] http/tests/media/video-buffered-range-contains-currentTime.html [ Failure ]