Reland "Don't PostTask the RenderWidget::Close() call."

This is a reland of 8f0dc968

Original change's description:
> Don't PostTask the RenderWidget::Close() call.
>
> The post task in RenderWidget was in order to keep IPC receipt of
> destruction of a frame-based RenderWidget (ie a RenderView in the past)
> from happening while the RenderWidget was already closing due to the
> renderer-side detaching, but running a nested message loop.
>
> The RenderView destruction now already does a PostTask hop in
> RenderThreadImpl before starting destruction of the RenderViewImpl
> and its frame tree and RenderWidgets. A RenderWidget for a frame closes
> when the frame detaches, and that is built to be consistent even if
> it occurs inside of unload. The RenderWidget does not need to be kept
> around after the blink frame and RenderFrame and WebWidget associated
> with it are all gone.
>
> Popups and pepper RenderWidgets can close during a frame unload without
> a consistency problem.
>
> R=dcheng@chromium.org
>
> Bug: 419087
> Change-Id: Ia5f7ca07395f8a5bd2c60a974a0fb4fb5092d872
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1832612
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: danakj <danakj@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#702868}

TBR=dcheng

Bug: 419087
Change-Id: I0f68df454e2873d7e6f3eeb38ff41563c16f6a76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1841942Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702977}

content/renderer/compositor/compositor_dependencies.h

@@ -52,6 +52,8 @@ class CONTENT_EXPORT CompositorDependencies {
   // compositor thread).
   virtual scoped_refptr<base::SingleThreadTaskRunner>
   GetCompositorImplThreadTaskRunner() = 0;
+  virtual scoped_refptr<base::SingleThreadTaskRunner>
+  GetCleanupTaskRunner() = 0;
   virtual blink::scheduler::WebThreadScheduler* GetWebMainThreadScheduler() = 0;
   virtual cc::TaskGraphRunner* GetTaskGraphRunner() = 0;
   virtual bool IsScrollAnimatorEnabled() = 0;

content/renderer/compositor/layer_tree_view.cc

@@ -58,18 +58,19 @@ LayerTreeView::LayerTreeView(
     scoped_refptr<base::SingleThreadTaskRunner> compositor_thread,
     cc::TaskGraphRunner* task_graph_runner,
     blink::scheduler::WebThreadScheduler* scheduler)
-    : delegate_(delegate),
-      main_thread_(std::move(main_thread)),
+    : main_thread_(std::move(main_thread)),
       compositor_thread_(std::move(compositor_thread)),
       task_graph_runner_(task_graph_runner),
       web_main_thread_scheduler_(scheduler),
-      animation_host_(cc::AnimationHost::CreateMainInstance()) {}
+      animation_host_(cc::AnimationHost::CreateMainInstance()),
+      delegate_(delegate) {}
 
 LayerTreeView::~LayerTreeView() = default;
 
 void LayerTreeView::Initialize(
     const cc::LayerTreeSettings& settings,
     std::unique_ptr<cc::UkmRecorderFactory> ukm_recorder_factory) {
+  DCHECK(delegate_);
   const bool is_threaded = !!compositor_thread_;
 
   cc::LayerTreeHost::InitParams params;
@@ -99,7 +100,17 @@ void LayerTreeView::Initialize(
   }
 }
 
+void LayerTreeView::Disconnect() {
+  DCHECK(delegate_);
+  // Drop compositor resources immediately, while keeping the compositor alive
+  // until after this class is destroyed.
+  layer_tree_host_->SetVisible(false);
+  layer_tree_host_->ReleaseLayerTreeFrameSink();
+  delegate_ = nullptr;
+}
+
 void LayerTreeView::SetVisible(bool visible) {
+  DCHECK(delegate_);
   layer_tree_host_->SetVisible(visible);
 
   if (visible && layer_tree_frame_sink_request_failed_while_invisible_)
@@ -108,6 +119,7 @@ void LayerTreeView::SetVisible(bool visible) {
 
 void LayerTreeView::SetLayerTreeFrameSink(
     std::unique_ptr<cc::LayerTreeFrameSink> layer_tree_frame_sink) {
+  DCHECK(delegate_);
   if (!layer_tree_frame_sink) {
     DidFailToInitializeLayerTreeFrameSink();
     return;
@@ -116,18 +128,26 @@ void LayerTreeView::SetLayerTreeFrameSink(
 }
 
 void LayerTreeView::WillBeginMainFrame() {
+  if (!delegate_)
+    return;
   delegate_->WillBeginCompositorFrame();
 }
 
 void LayerTreeView::DidBeginMainFrame() {
+  if (!delegate_)
+    return;
   delegate_->DidBeginMainFrame();
 }
 
 void LayerTreeView::WillUpdateLayers() {
+  if (!delegate_)
+    return;
   delegate_->BeginUpdateLayers();
 }
 
 void LayerTreeView::DidUpdateLayers() {
+  if (!delegate_)
+    return;
   delegate_->EndUpdateLayers();
   // Dump property trees and layers if run with:
   //   --vmodule=layer_tree_view=3
@@ -139,52 +159,74 @@ void LayerTreeView::DidUpdateLayers() {
 }
 
 void LayerTreeView::BeginMainFrame(const viz::BeginFrameArgs& args) {
+  if (!delegate_)
+    return;
   web_main_thread_scheduler_->WillBeginFrame(args);
   delegate_->BeginMainFrame(args.frame_time);
 }
 
 void LayerTreeView::OnDeferMainFrameUpdatesChanged(bool status) {
+  if (!delegate_)
+    return;
   delegate_->OnDeferMainFrameUpdatesChanged(status);
 }
 
 void LayerTreeView::OnDeferCommitsChanged(bool status) {
+  if (!delegate_)
+    return;
   delegate_->OnDeferCommitsChanged(status);
 }
 
 void LayerTreeView::BeginMainFrameNotExpectedSoon() {
+  if (!delegate_)
+    return;
   web_main_thread_scheduler_->BeginFrameNotExpectedSoon();
 }
 
 void LayerTreeView::BeginMainFrameNotExpectedUntil(base::TimeTicks time) {
+  if (!delegate_)
+    return;
   web_main_thread_scheduler_->BeginMainFrameNotExpectedUntil(time);
 }
 
 void LayerTreeView::UpdateLayerTreeHost() {
+  if (!delegate_)
+    return;
   delegate_->UpdateVisualState();
 }
 
 void LayerTreeView::ApplyViewportChanges(
     const cc::ApplyViewportChangesArgs& args) {
+  if (!delegate_)
+    return;
   delegate_->ApplyViewportChanges(args);
 }
 
 void LayerTreeView::RecordManipulationTypeCounts(cc::ManipulationInfo info) {
+  if (!delegate_)
+    return;
   delegate_->RecordManipulationTypeCounts(info);
 }
 
 void LayerTreeView::SendOverscrollEventFromImplSide(
     const gfx::Vector2dF& overscroll_delta,
     cc::ElementId scroll_latched_element_id) {
+  if (!delegate_)
+    return;
   delegate_->SendOverscrollEventFromImplSide(overscroll_delta,
                                              scroll_latched_element_id);
 }
 
 void LayerTreeView::SendScrollEndEventFromImplSide(
     cc::ElementId scroll_latched_element_id) {
+  if (!delegate_)
+    return;
   delegate_->SendScrollEndEventFromImplSide(scroll_latched_element_id);
 }
 
 void LayerTreeView::RequestNewLayerTreeFrameSink() {
+  if (!delegate_)
+    return;
   // When the compositor is not visible it would not request a
   // LayerTreeFrameSink so this is a race where it requested one then became
   // not-visible. In that case, we can wait for it to become visible again
@@ -217,6 +259,8 @@ void LayerTreeView::RequestNewLayerTreeFrameSink() {
 void LayerTreeView::DidInitializeLayerTreeFrameSink() {}
 
 void LayerTreeView::DidFailToInitializeLayerTreeFrameSink() {
+  if (!delegate_)
+    return;
   if (!layer_tree_host_->IsVisible()) {
     layer_tree_frame_sink_request_failed_while_invisible_ = true;
     return;
@@ -228,25 +272,35 @@ void LayerTreeView::DidFailToInitializeLayerTreeFrameSink() {
 }
 
 void LayerTreeView::WillCommit() {
+  if (!delegate_)
+    return;
   delegate_->WillCommitCompositorFrame();
 }
 
 void LayerTreeView::DidCommit() {
+  if (!delegate_)
+    return;
   delegate_->DidCommitCompositorFrame();
   web_main_thread_scheduler_->DidCommitFrameToCompositor();
 }
 
 void LayerTreeView::DidCommitAndDrawFrame() {
+  if (!delegate_)
+    return;
   delegate_->DidCommitAndDrawCompositorFrame();
 }
 
 void LayerTreeView::DidCompletePageScaleAnimation() {
+  if (!delegate_)
+    return;
   delegate_->DidCompletePageScaleAnimation();
 }
 
 void LayerTreeView::DidPresentCompositorFrame(
     uint32_t frame_token,
     const gfx::PresentationFeedback& feedback) {
+  if (!delegate_)
+    return;
   DCHECK(layer_tree_host_->GetTaskRunnerProvider()
              ->MainThreadTaskRunner()
              ->RunsTasksInCurrentSequence());
@@ -261,10 +315,14 @@ void LayerTreeView::DidPresentCompositorFrame(
 }
 
 void LayerTreeView::RecordStartOfFrameMetrics() {
+  if (!delegate_)
+    return;
   delegate_->RecordStartOfFrameMetrics();
 }
 
 void LayerTreeView::RecordEndOfFrameMetrics(base::TimeTicks frame_begin_time) {
+  if (!delegate_)
+    return;
   delegate_->RecordEndOfFrameMetrics(frame_begin_time);
 }
 
@@ -288,6 +346,7 @@ void LayerTreeView::DidLoseLayerTreeFrameSink() {}
 void LayerTreeView::AddPresentationCallback(
     uint32_t frame_token,
     base::OnceCallback<void(base::TimeTicks)> callback) {
+  DCHECK(delegate_);
   if (!presentation_callbacks_.empty()) {
     auto& previous = presentation_callbacks_.back();
     uint32_t previous_frame_token = previous.first;

content/renderer/compositor/layer_tree_view.h

@@ -59,6 +59,10 @@ class CONTENT_EXPORT LayerTreeView : public cc::LayerTreeHostClient,
   void Initialize(const cc::LayerTreeSettings& settings,
                   std::unique_ptr<cc::UkmRecorderFactory> ukm_recorder_factory);
 
+  // Drops any references back to the delegate in preparation for being
+  // destroyed.
+  void Disconnect();
+
   cc::AnimationHost* animation_host() { return animation_host_.get(); }
 
   void SetVisible(bool visible);
@@ -121,14 +125,21 @@ class CONTENT_EXPORT LayerTreeView : public cc::LayerTreeHostClient,
   void SetLayerTreeFrameSink(
       std::unique_ptr<cc::LayerTreeFrameSink> layer_tree_frame_sink);
 
-  LayerTreeViewDelegate* const delegate_;
   const scoped_refptr<base::SingleThreadTaskRunner> main_thread_;
   const scoped_refptr<base::SingleThreadTaskRunner> compositor_thread_;
   cc::TaskGraphRunner* const task_graph_runner_;
   blink::scheduler::WebThreadScheduler* const web_main_thread_scheduler_;
   const std::unique_ptr<cc::AnimationHost> animation_host_;
+
+  // The delegate_ becomes null when Disconnect() is called. After that, the
+  // class should do nothing in calls from the LayerTreeHost, and just wait to
+  // be destroyed. It is not expected to be used at all after Disconnect()
+  // outside of handling/dropping LayerTreeHost client calls.
+  LayerTreeViewDelegate* delegate_;
   std::unique_ptr<cc::LayerTreeHost> layer_tree_host_;
 
+  // This class should do nothing and access no pointers once this value becomes
+  // true.
   bool layer_tree_frame_sink_request_failed_while_invisible_ = false;
 
   base::circular_deque<

content/renderer/input/widget_input_handler_impl.cc

@@ -30,8 +30,7 @@ void RunClosureIfNotSwappedOut(base::WeakPtr<RenderWidget> render_widget,
                                base::OnceClosure closure) {
   // Input messages must not be processed if the RenderWidget was undead or is
   // closing.
-  if (!render_widget || render_widget->IsUndeadOrProvisional() ||
-      render_widget->is_closing()) {
+  if (!render_widget || render_widget->IsUndeadOrProvisional()) {
     return;
   }
   std::move(closure).Run();

content/renderer/input/widget_input_handler_manager.cc

@@ -567,8 +567,7 @@ void WidgetInputHandlerManager::HandleInputEvent(
     const ui::WebScopedInputEvent& event,
     const ui::LatencyInfo& latency,
     mojom::WidgetInputHandler::DispatchEventCallback callback) {
-  if (!render_widget_ || render_widget_->IsUndeadOrProvisional() ||
-      render_widget_->is_closing()) {
+  if (!render_widget_ || render_widget_->IsUndeadOrProvisional()) {
     if (callback) {
       std::move(callback).Run(InputEventAckSource::MAIN_THREAD, latency,
                               INPUT_EVENT_ACK_STATE_NOT_CONSUMED, base::nullopt,

content/renderer/render_thread_impl.cc

@@ -1540,6 +1540,13 @@ RenderThreadImpl::GetCompositorImplThreadTaskRunner() {
   return compositor_task_runner_;
 }
 
+scoped_refptr<base::SingleThreadTaskRunner>
+RenderThreadImpl::GetCleanupTaskRunner() {
+  return current_blink_platform_impl()
+      ->main_thread_scheduler()
+      ->CleanupTaskRunner();
+}
+
 gpu::GpuMemoryBufferManager* RenderThreadImpl::GetGpuMemoryBufferManager() {
   return gpu_->gpu_memory_buffer_manager();
 }

content/renderer/render_thread_impl.h

@@ -213,6 +213,7 @@ class CONTENT_EXPORT RenderThreadImpl
   GetCompositorMainThreadTaskRunner() override;
   scoped_refptr<base::SingleThreadTaskRunner>
   GetCompositorImplThreadTaskRunner() override;
+  scoped_refptr<base::SingleThreadTaskRunner> GetCleanupTaskRunner() override;
   blink::scheduler::WebThreadScheduler* GetWebMainThreadScheduler() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   bool IsScrollAnimatorEnabled() override;

content/renderer/render_view_impl.cc

@@ -1072,16 +1072,12 @@ void RenderViewImpl::Destroy() {
   // a main frame. So it should not be able to see this happening when there is
   // no local main frame.
   if (close_render_widget_here) {
-    // TODO(danakj): Go through CloseForFrame()? But we don't need/want to post-
-    // task the Close step here, do we? Since we're inside RenderViewImpl
-    // destruction?
-    render_widget_->PrepareForClose();
     // We pass ownership of |render_widget_| to itself. Grab a raw pointer to
     // call the Close() method on so we don't have to be a C++ expert to know
     // whether we will end up with a nullptr where we didn't intend due to order
     // of execution.
     RenderWidget* closing_widget = render_widget_.get();
-    closing_widget->Close(std::move(render_widget_));
+    closing_widget->CloseForFrame(std::move(render_widget_));
   }
 
   delete this;
@@ -1571,19 +1567,15 @@ void RenderViewImpl::DetachWebFrameWidget() {
     // We are inside RenderViewImpl::Destroy() and the main frame is being
     // detached as part of shutdown. So we can destroy the RenderWidget.
 
-    // The RenderWidget is closed and it will close the WebWidget stored in
-    // |frame_widget_|. We just want to drop raw pointer here.
+    // The RenderWidget will be closed, and it will close the WebWidget stored
+    // in |frame_widget_|. We just want to drop raw pointer here.
     frame_widget_ = nullptr;
-    // TODO(danakj): Go through CloseForFrame()? But we don't need/want to post-
-    // task the Close step here, do we? Since we're inside RenderViewImpl
-    // destruction?
-    render_widget_->PrepareForClose();
     // We pass ownership of |render_widget_| to itself. Grab a raw pointer to
     // call the Close() method on so we don't have to be a C++ expert to know
     // whether we will end up with a nullptr where we didn't intend due to order
     // of execution.
     RenderWidget* closing_widget = render_widget_.get();
-    closing_widget->Close(std::move(render_widget_));
+    closing_widget->CloseForFrame(std::move(render_widget_));
   } else {
     // We are not inside RenderViewImpl::Destroy(), the main frame is being
     // detached and replaced with a remote frame proxy. We can't close the

content/renderer/render_widget.h

@@ -254,16 +254,6 @@ class CONTENT_EXPORT RenderWidget
   // passed into this object to asynchronously delete itself.
   void CloseForFrame(std::unique_ptr<RenderWidget> widget);
 
-  // RenderWidgets cannot always be synchronously destroyed, since that may
-  // happen in a re-entrancy scenario, and there may be existing references on
-  // the stack. This method shuts down further sources of input to the
-  // RenderWidget. This must be called before Close().
-  void PrepareForClose();
-
-  // Close the underlying WebWidget and stop the compositor. This method deletes
-  // the object.
-  virtual void Close(std::unique_ptr<RenderWidget> widget);
-
   int32_t routing_id() const { return routing_id_; }
 
   // TODO(https://crbug.com/912193): Use CompositorDependencies on
@@ -285,8 +275,6 @@ class CONTENT_EXPORT RenderWidget
   bool is_fullscreen_granted() const { return is_fullscreen_granted_; }
   blink::mojom::DisplayMode display_mode() const { return display_mode_; }
   bool is_hidden() const { return is_hidden_; }
-  // Temporary for debugging purposes...
-  bool closing() const { return closing_; }
   bool has_host_context_menu_location() const {
     return has_host_context_menu_location_;
   }
@@ -313,13 +301,6 @@ class CONTENT_EXPORT RenderWidget
   // so should check for both states via this method instead.
   bool IsUndeadOrProvisional() { return is_undead_ || IsForProvisionalFrame(); }
 
-  // This is true once a Close IPC has been received. The actual action of
-  // closing must be done on another stack frame, in case the IPC receipt
-  // is in a nested message loop and will unwind back up to javascript (from
-  // plugins). So this will be true between those two things, to avoid work
-  // when the RenderWidget will be closed.
-  bool is_closing() const { return closing_; }
-
   // Manage edit commands to be used for the next keyboard event.
   const EditCommands& edit_commands() const { return edit_commands_; }
   void SetEditCommandForNextKeyEvent(const std::string& name,
@@ -687,13 +668,6 @@ class CONTENT_EXPORT RenderWidget
   virtual void SynchronizeVisualPropertiesFromRenderView(
       const VisualProperties& visual_properties);
 
-  bool in_synchronous_composite_for_testing() const {
-    return in_synchronous_composite_for_testing_;
-  }
-  void set_in_synchronous_composite_for_testing(bool in) {
-    in_synchronous_composite_for_testing_ = in;
-  }
-
   base::WeakPtr<RenderWidget> AsWeakPtr();
 
   // TODO(https://crbug.com/995981): Eventually, the lifetime of RenderWidget
@@ -706,6 +680,9 @@ class CONTENT_EXPORT RenderWidget
   // Notify subclasses that we initiated the paint operation.
   virtual void DidInitiatePaint() {}
 
+  // Destroy the RenderWidget. The |widget| is the owning pointer of |this|.
+  virtual void Close(std::unique_ptr<RenderWidget> widget);
+
  private:
   // Friend RefCounted so that the dtor can be non-public. Using this class
   // without ref-counting is an error.
@@ -724,8 +701,6 @@ class CONTENT_EXPORT RenderWidget
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetPopupUnittest, EmulatingPopupRect);
   FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, EmulatingPopupRect);
 
-  static scoped_refptr<base::SingleThreadTaskRunner> GetCleanupTaskRunner();
-
   // Called by Create() functions and subclasses to finish initialization.
   // |show_callback| will be invoked once WebWidgetClient::Show() occurs, and
   // should be null if Show() won't be triggered for this widget.
@@ -999,9 +974,8 @@ class CONTENT_EXPORT RenderWidget
   // ImeEventGuard. We keep track of the outermost one, and update it as needed.
   ImeEventGuard* ime_event_guard_ = nullptr;
 
-  bool closed_ = false;
-  // True if we have requested this widget be closed.  No more messages will
-  // be sent, except for a Close.
+  // True once Close() is called, during the self-destruction process, and to
+  // verify destruction always goes through Close().
   bool closing_ = false;
 
   // A RenderWidget is undead if it is the RenderWidget attached to the
@@ -1033,9 +1007,6 @@ class CONTENT_EXPORT RenderWidget
   // process, without the use of this mode, however it would be overridden by
   // the browser if they disagree.
   bool synchronous_resize_mode_for_testing_ = false;
-  // In web tests, synchronous composites should not be nested inside another
-  // composite, and this bool is used to guard against that.
-  bool in_synchronous_composite_for_testing_ = false;
 
   // Stores information about the current text input.
   blink::WebTextInputInfo text_input_info_;
@@ -1183,9 +1154,6 @@ class CONTENT_EXPORT RenderWidget
 
   uint32_t last_capture_sequence_number_ = 0u;
 
-  // This factory is invalidated when the WebWidget is closed.
-  base::WeakPtrFactory<RenderWidget> close_weak_ptr_factory_{this};
-  // This factory is invalidated when the RenderWidget is destroyed.
   base::WeakPtrFactory<RenderWidget> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(RenderWidget);

content/renderer/render_widget_unittest.cc

@@ -188,6 +188,8 @@ class InteractiveRenderWidget : public RenderWidget {
         mock_input_handler_host_->BindNewPipeAndPassRemote());
   }
 
+  using RenderWidget::Close;
+
   void SendInputEvent(const blink::WebInputEvent& event,
                       HandledEventCallback callback) {
     HandleInputEvent(blink::WebCoalescedInputEvent(
@@ -252,31 +254,29 @@ int InteractiveRenderWidget::next_routing_id_ = 0;
 class RenderWidgetUnittest : public testing::Test {
  public:
   RenderWidgetUnittest() : page_properties_(&compositor_deps_) {}
-  // testing::Test implementation.
+
   void SetUp() override {
     widget_ = std::make_unique<InteractiveRenderWidget>(&compositor_deps_,
                                                         &page_properties_);
   }
 
-  void DestroyWidget() {
-    if (widget_) {
-      widget_->PrepareForClose();
-      widget_->Close(std::move(widget_));
-    }
+  void TearDown() override {
+    widget_->Close(std::move(widget_));
+    // RenderWidget::Close() posts some destruction. Don't leak them.
+    base::RunLoop loop;
+    compositor_deps_.GetCleanupTaskRunner()->PostTask(FROM_HERE,
+                                                      loop.QuitClosure());
+    loop.Run();
   }
 
-  void TearDown() override { DestroyWidget(); }
-
   InteractiveRenderWidget* widget() const { return widget_.get(); }
 
   const base::HistogramTester& histogram_tester() const {
     return histogram_tester_;
   }
 
- protected:
-  base::test::TaskEnvironment task_environment_;
-
  private:
+  base::test::TaskEnvironment task_environment_;
   MockRenderProcess render_process_;
   MockRenderThread render_thread_;
   FakeCompositorDependencies compositor_deps_;

content/shell/test_runner/web_widget_test_proxy.cc

@@ -161,18 +161,6 @@ TestRunner* WebWidgetTestProxy::GetTestRunner() {
 }
 
 static void DoComposite(content::RenderWidget* widget, bool do_raster) {
-  if (!widget->layer_tree_view()->layer_tree_host()->IsVisible())
-    return;
-
-  if (widget->in_synchronous_composite_for_testing()) {
-    // Web tests can use a nested message loop to pump frames while inside a
-    // frame, but the compositor does not support this. In this case, we only
-    // run blink's lifecycle updates.
-    widget->BeginMainFrame(base::TimeTicks::Now());
-    widget->UpdateVisualState();
-    return;
-  }
-
   // Ensure that there is damage so that the compositor submits, and the display
   // compositor draws this frame.
   if (do_raster) {
@@ -180,10 +168,8 @@ static void DoComposite(content::RenderWidget* widget, bool do_raster) {
     layer_tree_view->layer_tree_host()->SetNeedsCommitWithForcedRedraw();
   }
 
-  widget->set_in_synchronous_composite_for_testing(true);
   widget->layer_tree_view()->layer_tree_host()->Composite(
       base::TimeTicks::Now(), do_raster);
-  widget->set_in_synchronous_composite_for_testing(false);
 }
 
 void WebWidgetTestProxy::SynchronouslyComposite(bool do_raster) {
@@ -193,7 +179,27 @@ void WebWidgetTestProxy::SynchronouslyComposite(bool do_raster) {
               ->GetSettings()
               .single_thread_proxy_scheduler);
 
+  if (!layer_tree_view()->layer_tree_host()->IsVisible())
+    return;
+
+  if (in_synchronous_composite_) {
+    // Web tests can use a nested message loop to pump frames while inside a
+    // frame, but the compositor does not support this. In this case, we only
+    // run blink's lifecycle updates.
+    BeginMainFrame(base::TimeTicks::Now());
+    UpdateVisualState();
+    return;
+  }
+
+  in_synchronous_composite_ = true;
+
+  // Composite() can detach the frame, which would destroy |this|.
+  base::WeakPtr<WebWidgetTestProxy> weak_this = weak_factory_.GetWeakPtr();
   DoComposite(this, do_raster);
+  if (!weak_this)
+    return;
+
+  in_synchronous_composite_ = false;
 
   // If the RenderWidget is for the main frame, we also composite the current
   // PagePopup afterward.
@@ -212,24 +218,10 @@ void WebWidgetTestProxy::SynchronouslyComposite(bool do_raster) {
 }
 
 void WebWidgetTestProxy::AnimateNow() {
-  // For child local roots, it's possible that the backing WebWidget gets
-  // closed between the ScheduleAnimation() call and this execution
-  // leading to a nullptr.  This happens because child local roots are
-  // owned by RenderFrames which drops the WebWidget before executing the
-  // Close() call that would invalidate the |weak_factory_| canceling the
-  // scheduled calls to AnimateNow(). In main frames, the WebWidget is
-  // dropped synchronously by Close() avoiding the problem.
-  //
-  // Ideally there would not be this divergence between frame types, and/or
-  // there would be a hook for signaling a stop to the animation. As this is
-  // not an ideal work, returning early when GetWebWidget() returns nullptr
-  // is good enough for a fake impl. Also, unicorns are mythical. Sorry.
-  if (!GetWebWidget())
-    return;
-
   bool do_raster = composite_requested_;
   animation_scheduled_ = false;
   composite_requested_ = false;
+  // Composite may destroy |this|, so don't use it afterward.
   SynchronouslyComposite(do_raster);
 }
 

content/shell/test_runner/web_widget_test_proxy.h

@@ -93,6 +93,8 @@ class TEST_RUNNER_EXPORT WebWidgetTestProxy : public content::RenderWidget {
   // When |do_raster| is false, only a main frame animation step is performed,
   // but when true, a full composite is performed and a frame submitted to the
   // display compositor if there is any damage.
+  // Note that compositing has the potential to detach the current frame and
+  // thus destroy |this| before returning.
   void SynchronouslyComposite(bool do_raster);
 
  private:
@@ -114,6 +116,9 @@ class TEST_RUNNER_EXPORT WebWidgetTestProxy : public content::RenderWidget {
   // https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_web_tests.md
   // for details on the optimization.
   bool composite_requested_ = false;
+  // Synchronous composites should not be nested inside another
+  // composite, and this bool is used to guard against that.
+  bool in_synchronous_composite_ = false;
 
   base::WeakPtrFactory<WebWidgetTestProxy> weak_factory_{this};
 

content/test/fake_compositor_dependencies.cc

@@ -64,6 +64,11 @@ FakeCompositorDependencies::GetCompositorImplThreadTaskRunner() {
   return nullptr;  // Currently never threaded compositing in unit tests.
 }
 
+scoped_refptr<base::SingleThreadTaskRunner>
+FakeCompositorDependencies::GetCleanupTaskRunner() {
+  return base::ThreadTaskRunnerHandle::Get();
+}
+
 blink::scheduler::WebThreadScheduler*
 FakeCompositorDependencies::GetWebMainThreadScheduler() {
   return &main_thread_scheduler_;

content/test/fake_compositor_dependencies.h

@@ -33,6 +33,7 @@ class FakeCompositorDependencies : public CompositorDependencies {
   GetCompositorMainThreadTaskRunner() override;
   scoped_refptr<base::SingleThreadTaskRunner>
   GetCompositorImplThreadTaskRunner() override;
+  scoped_refptr<base::SingleThreadTaskRunner> GetCleanupTaskRunner() override;
   blink::scheduler::WebThreadScheduler* GetWebMainThreadScheduler() override;
   cc::TaskGraphRunner* GetTaskGraphRunner() override;
   bool IsScrollAnimatorEnabled() override;