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

This reverts commit 8f0dc968.

Reason for revert: Suspected of causing webkit_layout_tests to fail in http/tests/misc/frame-detached-in-animationstart-event.html on WebKit Linux ASAN.

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=avi@chromium.org,danakj@chromium.org,dcheng@chromium.org

Change-Id: I6edf67e9d004c7f2fb4f3edd36b2a86f4cb5f27a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 419087
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1841941Reviewed-by: Samuel Huang <huangs@chromium.org>
Commit-Queue: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702926}

content/renderer/compositor/compositor_dependencies.h

@@ -52,8 +52,6 @@ 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,19 +58,18 @@ LayerTreeView::LayerTreeView(
     scoped_refptr<base::SingleThreadTaskRunner> compositor_thread,
     cc::TaskGraphRunner* task_graph_runner,
     blink::scheduler::WebThreadScheduler* scheduler)
-    : main_thread_(std::move(main_thread)),
+    : delegate_(delegate),
+      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()),
-      delegate_(delegate) {}
+      animation_host_(cc::AnimationHost::CreateMainInstance()) {}
 
 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;
@@ -100,17 +99,7 @@ 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_)
@@ -119,7 +108,6 @@ 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;
@@ -128,26 +116,18 @@ 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
@@ -159,74 +139,52 @@ 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
@@ -259,8 +217,6 @@ 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;
@@ -272,35 +228,25 @@ 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());
@@ -315,14 +261,10 @@ 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);
 }
 
@@ -346,7 +288,6 @@ 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,10 +59,6 @@ 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);
@@ -125,21 +121,14 @@ 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,7 +30,8 @@ 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()) {
+  if (!render_widget || render_widget->IsUndeadOrProvisional() ||
+      render_widget->is_closing()) {
     return;
   }
   std::move(closure).Run();

content/renderer/input/widget_input_handler_manager.cc

@@ -567,7 +567,8 @@ void WidgetInputHandlerManager::HandleInputEvent(
     const ui::WebScopedInputEvent& event,
     const ui::LatencyInfo& latency,
     mojom::WidgetInputHandler::DispatchEventCallback callback) {
-  if (!render_widget_ || render_widget_->IsUndeadOrProvisional()) {
+  if (!render_widget_ || render_widget_->IsUndeadOrProvisional() ||
+      render_widget_->is_closing()) {
     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,13 +1540,6 @@ 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,7 +213,6 @@ 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,12 +1072,16 @@ 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->CloseForFrame(std::move(render_widget_));
+    closing_widget->Close(std::move(render_widget_));
   }
 
   delete this;
@@ -1567,15 +1571,19 @@ 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 will be closed, and it will close the WebWidget stored
-    // in |frame_widget_|. We just want to drop raw pointer here.
+    // The RenderWidget is 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->CloseForFrame(std::move(render_widget_));
+    closing_widget->Close(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,6 +254,16 @@ 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
@@ -275,6 +285,8 @@ 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_;
   }
@@ -301,6 +313,13 @@ 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,9 +706,6 @@ 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.
@@ -708,6 +724,8 @@ 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.
@@ -981,8 +999,9 @@ class CONTENT_EXPORT RenderWidget
   // ImeEventGuard. We keep track of the outermost one, and update it as needed.
   ImeEventGuard* ime_event_guard_ = nullptr;
 
-  // True once Close() is called, during the self-destruction process, and to
-  // verify destruction always goes through Close().
+  bool closed_ = false;
+  // True if we have requested this widget be closed.  No more messages will
+  // be sent, except for a Close.
   bool closing_ = false;
 
   // A RenderWidget is undead if it is the RenderWidget attached to the
@@ -1164,6 +1183,9 @@ 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,8 +188,6 @@ class InteractiveRenderWidget : public RenderWidget {
         mock_input_handler_host_->BindNewPipeAndPassRemote());
   }
 
-  using RenderWidget::Close;
-
   void SendInputEvent(const blink::WebInputEvent& event,
                       HandledEventCallback callback) {
     HandleInputEvent(blink::WebCoalescedInputEvent(
@@ -254,29 +252,31 @@ 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 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 DestroyWidget() {
+    if (widget_) {
+      widget_->PrepareForClose();
+      widget_->Close(std::move(widget_));
+    }
   }
 
+  void TearDown() override { DestroyWidget(); }
+
   InteractiveRenderWidget* widget() const { return widget_.get(); }
 
   const base::HistogramTester& histogram_tester() const {
     return histogram_tester_;
   }
 
- private:
+ protected:
   base::test::TaskEnvironment task_environment_;
+
+ private:
   MockRenderProcess render_process_;
   MockRenderThread render_thread_;
   FakeCompositorDependencies compositor_deps_;

content/test/fake_compositor_dependencies.cc

@@ -64,11 +64,6 @@ 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,7 +33,6 @@ 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;