Revert "Make browser process own RenderView." This reverts commit 55bc919aff4f9d30d03090859321050557407e85. Reason for revert: Suspected cause of https://crbug.com/990192 Since this changes the ownership of RenderWidget and the newly flaky test (which started failing on MSAN 1 build after this landed) has a use-after-free in RenderWidget, I am suspecting this CL. Original change's description: > Make browser process own RenderView. > > Historically, RenderView and RenderWidget were 1:1, and their lifetimes were > entwined. > 1) RenderViewHost would create the RenderView. > 2) RenderView would create a RenderWidget and pass ownership of itself to the > RenderWidget. > 3) RenderViewHost's destructor would destroy the RenderWidget, thus destroying > the RenderView > > We want the lifetime of RenderView and RenderWidget to be decoupled. The first > step of this is making RenderView explicitly owned by the browser. This means > that instead of (3), RenderViewHost's destructor will destroy the RenderView, > which will in turn destroy the RenderWidget. > > One subtlety is that prior to this CL, RenderWidget was always destroyed > asynchronously. The original reason for supporting this -- dealing with > re-entrancy from nested message loops -- is no longer applicable. The IPC that > destroys RenderWidget is asynchronous, which means it can never be called from a > re-entrant context. However, it is possible for a RenderWidget associated with a > child-frame to be synchronously destroyed by JS. This can be re-entrant. This CL > updates destruction of RenderWidget to be synchronous when called from IPC. > > Bug: 987731 > Change-Id: If4b319fab19d02c5495ba14e5cc929f441ca4d2e > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1717456 > Commit-Queue: Avi Drissman <avi@chromium.org> > Reviewed-by: Avi Drissman <avi@chromium.org> > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > Auto-Submit: Erik Chen <erikchen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#683247} TBR=avi@chromium.org,dcheng@chromium.org,erikchen@chromium.org Change-Id: I762230d0df57c6bcd75af80c1aa231d7b4d2183e No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 987731, 990192 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1731991 Reviewed-by: Matt Giuca <mgiuca@chromium.org> Commit-Queue: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#683457}
diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc index 0cda89d4..f81232a 100644 --- a/content/browser/renderer_host/render_view_host_impl.cc +++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -266,12 +266,7 @@ GetWidget()->GetRoutingID()); } - // Destroy the RenderWidgetHost. GetWidget()->ShutdownAndDestroyWidget(false); - if (IsRenderViewLive()) { - // Destroy the RenderView, which will also destroy the RenderWidget. - GetProcess()->GetRendererInterface()->DestroyView(GetRoutingID()); - } ui::GpuSwitchingManager::GetInstance()->RemoveObserver(this); @@ -378,9 +373,6 @@ GetWidget()->GetVisualProperties(¶ms->visual_properties, &needs_ack); GetWidget()->SetInitialVisualProperties(params->visual_properties, needs_ack); - // The RenderView is owned by this process. This call must be accompanied by a - // DestroyView [see destructor] or else there will be a leak in the renderer - // process. GetProcess()->GetRendererInterface()->CreateView(std::move(params)); // Let our delegate know that we created a RenderView.
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc index 0ba907f..af5be0a 100644 --- a/content/browser/renderer_host/render_widget_host_impl.cc +++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -599,10 +599,8 @@ CancelKeyboardLock(); RejectMouseLockOrUnlockIfNecessary(); - if (process_->IsInitializedAndNotDead() && !owner_delegate()) { - // Tell the RendererWidget to close. We only want to do this if the - // RenderWidget is the root of the renderer object graph, which is for - // pepper fullscreen and popups. + if (process_->IsInitializedAndNotDead()) { + // Tell the RendererWidget to close. bool rv = Send(new WidgetMsg_Close(routing_id_)); DCHECK(rv); }
diff --git a/content/browser/renderer_host/render_widget_host_impl.h b/content/browser/renderer_host/render_widget_host_impl.h index 5ab1218..ed6447bd 100644 --- a/content/browser/renderer_host/render_widget_host_impl.h +++ b/content/browser/renderer_host/render_widget_host_impl.h
@@ -120,34 +120,6 @@ // This implements the RenderWidgetHost interface that is exposed to // embedders of content, and adds things only visible to content. -// -// Several core rendering primitives are mirrored between the browser and -// renderer. These are RenderWidget, RenderFrame and RenderView. Their browser -// counterparts are RenderWidgetHost, RenderFrameHost and RenderViewHost. -// -// For simplicity and clarity, we want the object ownership graph in the -// renderer to mirror the object ownership graph in the browser. The IPC message -// that tears down the renderer object graph should be targeted at the root -// object, and should be sent by the destructor/finalizer of the root object in -// the browser. -// -// Note: We must tear down the renderer object graph with a single IPC to avoid -// inconsistencies in renderer state. -// -// RenderWidget represents a surface that can paint and receive input. It is -// used in four contexts: -// * Main frame for webpage -// * Child frame for webpage -// * Popups -// * Pepper Fullscreen -// -// In the first two cases, the RenderFrame is not the root of the renderer -// object graph. For the main frame, the root is the RenderView. For child -// frames, the root is RenderFrame. As such, for the first two cases, -// destruction of the RenderWidgetHost will not trigger destruction of the -// RenderWidget. -// -// Note: We want to converge on RenderFrame always being the root. class CONTENT_EXPORT RenderWidgetHostImpl : public RenderWidgetHost, public FrameTokenMessageQueue::Client, @@ -988,8 +960,6 @@ RenderWidgetHostDelegate* delegate_; // The delegate of the owner of this object. - // This member is non-null if and only if this RenderWidgetHost is associated - // with a main frame RenderWidget. RenderWidgetHostOwnerDelegate* owner_delegate_ = nullptr; // Created during construction and guaranteed never to be NULL, but its
diff --git a/content/browser/service_worker/embedded_worker_test_helper.cc b/content/browser/service_worker/embedded_worker_test_helper.cc index e187617..8698fcc4 100644 --- a/content/browser/service_worker/embedded_worker_test_helper.cc +++ b/content/browser/service_worker/embedded_worker_test_helper.cc
@@ -40,7 +40,6 @@ NOTREACHED(); } void CreateView(mojom::CreateViewParamsPtr) override { NOTREACHED(); } - void DestroyView(int32_t) override { NOTREACHED(); } void CreateFrame(mojom::CreateFrameParamsPtr) override { NOTREACHED(); } void SetUpEmbeddedWorkerChannelForServiceWorker( blink::mojom::EmbeddedWorkerInstanceClientRequest client_request)
diff --git a/content/common/renderer.mojom b/content/common/renderer.mojom index 6c83151b..462be9b 100644 --- a/content/common/renderer.mojom +++ b/content/common/renderer.mojom
@@ -180,11 +180,6 @@ // Tells the renderer to create a new view. CreateView(CreateViewParams params); - // Tells the renderer to destroy an existing view. This method must be called - // exactly once for each invocation of CreateView. |view_id| is synonymous - // with |routing_id|. - DestroyView(int32 view_id); - // Tells the renderer to create a new RenderFrame. CreateFrame(CreateFrameParams params);
diff --git a/content/public/test/render_view_test.cc b/content/public/test/render_view_test.cc index c7edf26..b24af4e 100644 --- a/content/public/test/render_view_test.cc +++ b/content/public/test/render_view_test.cc
@@ -84,29 +84,24 @@ namespace { -// This class records, and then tears down all existing RenderViews. It's -// important to do this in two steps, since tearing down a RenderView will -// mutate the container that RenderView::ForEach() iterates over. class CloseMessageSendingRenderViewVisitor : public RenderViewVisitor { public: CloseMessageSendingRenderViewVisitor() = default; ~CloseMessageSendingRenderViewVisitor() override = default; - void CloseRenderViews() { - for (RenderView* render_view : live_render_views) { - RenderViewImpl* view_impl = static_cast<RenderViewImpl*>(render_view); - view_impl->Destroy(); - } - } - protected: bool Visit(RenderView* render_view) override { - live_render_views.push_back(render_view); + // Simulate the Widget receiving a close message. This should result on + // releasing the internal reference counts and destroying the internal + // state. + RenderWidget* render_widget = + static_cast<RenderViewImpl*>(render_view)->GetWidget(); + WidgetMsg_Close msg(render_widget->routing_id()); + render_widget->OnMessageReceived(msg); return true; } private: - std::vector<RenderView*> live_render_views; DISALLOW_COPY_AND_ASSIGN(CloseMessageSendingRenderViewVisitor); }; @@ -471,7 +466,6 @@ // opened by the test. CloseMessageSendingRenderViewVisitor closing_visitor; RenderView::ForEach(&closing_visitor); - closing_visitor.CloseRenderViews(); // |view_| is ref-counted and deletes itself during the RunUntilIdle() call // below.
diff --git a/content/renderer/input/widget_input_handler_manager.cc b/content/renderer/input/widget_input_handler_manager.cc index cc82b0f..a527fddc 100644 --- a/content/renderer/input/widget_input_handler_manager.cc +++ b/content/renderer/input/widget_input_handler_manager.cc
@@ -469,11 +469,6 @@ bool sync_compositing) { DCHECK(InputThreadTaskRunner()->BelongsToCurrentThread()); - // It is possible that the input_handle has already been destroyed before this - // Init() call was invoked. If so, early out. - if (!input_handler) - return; - // If there's no compositor thread (i.e. we're in a LayoutTest), force input // to go through the main thread. bool force_input_handling_on_main = !compositor_task_runner_;
diff --git a/content/renderer/render_thread_impl.cc b/content/renderer/render_thread_impl.cc index 24c8d97c..ee1544d 100644 --- a/content/renderer/render_thread_impl.cc +++ b/content/renderer/render_thread_impl.cc
@@ -2027,12 +2027,6 @@ GetWebMainThreadScheduler()->DefaultTaskRunner()); } -void RenderThreadImpl::DestroyView(int32_t view_id) { - RenderViewImpl* view = RenderViewImpl::FromRoutingID(view_id); - DCHECK(view); - view->Destroy(); -} - void RenderThreadImpl::CreateFrame(mojom::CreateFrameParamsPtr params) { CompositorDependencies* compositor_deps = this; service_manager::mojom::InterfaceProviderPtr interface_provider(
diff --git a/content/renderer/render_thread_impl.h b/content/renderer/render_thread_impl.h index 8128edd2..82e05e9a 100644 --- a/content/renderer/render_thread_impl.h +++ b/content/renderer/render_thread_impl.h
@@ -509,7 +509,6 @@ void CreateEmbedderRendererService( service_manager::mojom::ServiceRequest service_request) override; void CreateView(mojom::CreateViewParamsPtr params) override; - void DestroyView(int32_t view_id) override; void CreateFrame(mojom::CreateFrameParamsPtr params) override; void CreateFrameProxy( int32_t routing_id,
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc index c5a0403..118a7dc 100644 --- a/content/renderer/render_view_browsertest.cc +++ b/content/renderer/render_view_browsertest.cc
@@ -433,6 +433,17 @@ webview->MainFrame()->VisibleContentRect().width; } + // Closes a view created during the test, i.e. not the |view()|. Checks that + // the main frame is detached and deleted, and makes sure the view does not + // leak. + void CloseRenderWidget(RenderWidget* widget) { + WidgetMsg_Close msg(widget->routing_id()); + widget->OnMessageReceived(msg); + + // WidgetMsg_Close posts a task to do the actual closing. Let that run. + base::RunLoop().RunUntilIdle(); + } + private: std::unique_ptr<MockKeyboard> mock_keyboard_; }; @@ -855,6 +866,9 @@ ->BeginNavigation(std::move(popup_navigation_info)); EXPECT_TRUE(render_thread_->sink().GetUniqueMessageMatching( FrameHostMsg_OpenURL::ID)); + + RenderWidget* render_widget = new_view->GetWidget(); + CloseRenderWidget(render_widget); } class AlwaysForkingRenderViewTest : public RenderViewImplTest {
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc index 1a91981..035801fb 100644 --- a/content/renderer/render_view_impl.cc +++ b/content/renderer/render_view_impl.cc
@@ -476,8 +476,14 @@ scoped_refptr<base::SingleThreadTaskRunner> task_runner) { DCHECK(RenderThread::IsMainThread()); + // RenderView used to inherit from RenderWidget. Creating a delegate + // interface and explicitly passing ownership of ourselves to the + // RenderWidget preserves the lifetime semantics. This is a stepping + // stone to having RenderWidget creation taken out of the RenderViewImpl + // constructor. See the corresponding explicit reset() of the delegate + // in the ~RenderWidget(). Also, I hate inheritance. render_widget_ = render_widget; - GetWidget()->set_delegate(this); + GetWidget()->set_delegate(base::WrapUnique(this)); RenderThread::Get()->AddRoute(routing_id_, this); #if defined(OS_ANDROID) @@ -1047,12 +1053,6 @@ return render_view; } -void RenderViewImpl::Destroy() { - GetWidget()->PrepareForClose(); - GetWidget()->Close(); - delete this; -} - // static void RenderViewImpl::InstallCreateHook(RenderViewImpl* ( *create_render_view_impl)(CompositorDependencies* compositor_deps,
diff --git a/content/renderer/render_view_impl.h b/content/renderer/render_view_impl.h index 3e525b05..caf21b45 100644 --- a/content/renderer/render_view_impl.h +++ b/content/renderer/render_view_impl.h
@@ -103,6 +103,8 @@ public RenderWidgetDelegate, public RenderView { public: + ~RenderViewImpl() override; + // Creates a new RenderView. Note that if the original opener has been closed, // |params.window_was_created_with_opener| will be true and // |params.opener_frame_route_id| will be MSG_ROUTING_NONE. @@ -117,11 +119,6 @@ RenderWidget::ShowCallback show_callback, scoped_refptr<base::SingleThreadTaskRunner> task_runner); - // Instances of this object are created by and destroyed by the browser - // process. This method must be called exactly once by the IPC subsystem when - // the browser wishes the object to be destroyed. - void Destroy(); - // Used by web_test_support to hook into the creation of RenderViewImpls. static void InstallCreateHook(RenderViewImpl* (*create_render_view_impl)( CompositorDependencies* compositor_deps, @@ -296,7 +293,6 @@ protected: RenderViewImpl(CompositorDependencies* compositor_deps, const mojom::CreateViewParams& params); - ~RenderViewImpl() override; // Called when Page visibility is changed, to update the View/Page in blink. // This is separate from the IPC handlers as tests may call this and need to
diff --git a/content/renderer/render_widget.cc b/content/renderer/render_widget.cc index 67b20423..799cc64 100644 --- a/content/renderer/render_widget.cc +++ b/content/renderer/render_widget.cc
@@ -447,7 +447,6 @@ : routing_id_(widget_routing_id), compositor_deps_(compositor_deps), webwidget_internal_(nullptr), - delegate_(nullptr), auto_resize_mode_(false), is_hidden_(hidden), compositor_never_visible_(never_visible), @@ -525,13 +524,7 @@ void RenderWidget::CloseForFrame() { DCHECK(for_child_local_root_frame_); - PrepareForClose(); - - // The RenderWidget may be deattached from JS, which in turn may be called - // in a re-entrant context. We cannot synchronously destroy the object, so we - // post a task to do so later. - GetCleanupTaskRunner()->PostNonNestableTask( - FROM_HERE, base::BindOnce(&RenderWidget::Close, this)); + OnClose(); } void RenderWidget::Init(ShowCallback show_callback, WebWidget* web_widget) { @@ -577,9 +570,8 @@ mouse_lock_dispatcher_.reset(new RenderWidgetMouseLockDispatcher(this)); RenderThread::Get()->AddRoute(routing_id_, this); - // Take a reference. This object is either owned by the browser process, or by - // RenderFrame. Both will eventually call Close() when they wish to destroy - // this object. + // Take a reference on behalf of the RenderThread. This will be balanced + // when we receive WidgetMsg_Close. AddRef(); } @@ -712,14 +704,6 @@ } void RenderWidget::OnClose() { - // It is always safe to synchronously destroy this object from an IPC message. - // That's because the IPC message is asynchronous, which means it can never be - // called from a nested context. - PrepareForClose(); - Close(); -} - -void RenderWidget::PrepareForClose() { DCHECK(RenderThread::IsMainThread()); if (closing_) return; @@ -747,6 +731,23 @@ // already-deleted frame. CloseWebWidget(); } + // If there is a Send call on the stack, then it could be dangerous to close + // now. Post a task that only gets invoked when there are no nested message + // loops. + // + // The asynchronous Close() takes an owning reference to |this| keeping the + // object alive beyond the Release() below. It is the last reference to this + // object. + // + // TODO(https://crbug.com/545684): The actual lifetime for RenderWidget + // seems to be single-owner. It is either owned by "IPC" events (popup, + // mainframe, and fullscreen), or a RenderFrame. If Close() self-deleting, + // all the ref-counting mess could be removed. + GetCleanupTaskRunner()->PostNonNestableTask( + FROM_HERE, base::BindOnce(&RenderWidget::Close, this)); + + // Balances the AddRef taken when we called AddRoute. + Release(); } void RenderWidget::OnSynchronizeVisualProperties( @@ -1952,9 +1953,6 @@ // Note the ACK is a control message going to the RenderProcessHost. RenderThread::Get()->Send(new WidgetHostMsg_Close_ACK(routing_id())); closed_ = true; - - // Balances the AddRef in Init. - Release(); } void RenderWidget::CloseWebWidget() {
diff --git a/content/renderer/render_widget.h b/content/renderer/render_widget.h index 8f9dcda..72ab6536 100644 --- a/content/renderer/render_widget.h +++ b/content/renderer/render_widget.h
@@ -215,13 +215,16 @@ void InitForChildLocalRoot(blink::WebFrameWidget* web_frame_widget); // Sets a delegate to handle certain RenderWidget operations that need an - // escape to the RenderView. - void set_delegate(RenderWidgetDelegate* delegate) { + // escape to the RenderView. Also take ownership until RenderWidget lifetime + // has been reassociated with the RenderFrame. This is only set on Widgets + // that are associated with the RenderView. + // TODO(ajwong): Do not have RenderWidget own the delegate. + void set_delegate(std::unique_ptr<RenderWidgetDelegate> delegate) { DCHECK(!delegate_); - delegate_ = delegate; + delegate_ = std::move(delegate); } - RenderWidgetDelegate* delegate() const { return delegate_; } + RenderWidgetDelegate* delegate() const { return delegate_.get(); } // Returns the RenderWidget for the given routing ID. static RenderWidget* FromRoutingID(int32_t routing_id); @@ -229,15 +232,6 @@ // Closes a RenderWidget that was created by |CreateForFrame|. void CloseForFrame(); - // 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. - virtual void Close(); - int32_t routing_id() const { return routing_id_; } CompositorDependencies* compositor_deps() const { return compositor_deps_; } @@ -703,6 +697,9 @@ mojom::WidgetRequest widget_request); ~RenderWidget() override; + // Close the underlying WebWidget and stop the compositor. + virtual void Close(); + // Notify subclasses that we initiated the paint operation. virtual void DidInitiatePaint() {} @@ -937,9 +934,7 @@ blink::WebWidget* webwidget_internal_; // The delegate for this object which is just a RenderViewImpl. - // This member is non-null if and only if the RenderWidget is associated with - // a RenderViewImpl. - RenderWidgetDelegate* delegate_; + std::unique_ptr<RenderWidgetDelegate> delegate_; // This is lazily constructed and must not outlive webwidget_. std::unique_ptr<LayerTreeView> layer_tree_view_;
diff --git a/content/renderer/render_widget_unittest.cc b/content/renderer/render_widget_unittest.cc index f3db3ef..86699f8c 100644 --- a/content/renderer/render_widget_unittest.cc +++ b/content/renderer/render_widget_unittest.cc
@@ -226,6 +226,7 @@ protected: ~InteractiveRenderWidget() override { + Close(); webwidget_internal_ = nullptr; } @@ -269,18 +270,13 @@ // testing::Test implementation. void SetUp() override { widget_ = new InteractiveRenderWidget(&compositor_deps_); + // RenderWidget::Init does an AddRef that's balanced by a browser-initiated + // Close IPC. That Close will never happen in this test, so do a Release + // here to ensure |widget_| is properly freed. + widget_->Release(); + DCHECK(widget_->HasOneRef()); } - void DestroyWidget() { - if (widget_) { - widget_->PrepareForClose(); - widget_->Close(); - widget_.reset(); - } - } - - void TearDown() override { DestroyWidget(); } - InteractiveRenderWidget* widget() const { return widget_.get(); } const base::HistogramTester& histogram_tester() const { @@ -505,6 +501,11 @@ // testing::Test implementation. void SetUp() override { widget_ = new PopupRenderWidget(&compositor_deps_); + // RenderWidget::Init does an AddRef that's balanced by a browser-initiated + // Close IPC. That Close will never happen in this test, so do a Release + // here to ensure |widget_| is properly freed. + widget_->Release(); + DCHECK(widget_->HasOneRef()); } PopupRenderWidget* widget() const { return widget_.get(); } @@ -578,9 +579,7 @@ EXPECT_FALSE(layer_tree_host->is_external_pinch_gesture_active_for_testing()); // Repeat with a 'mainframe' widget. - std::unique_ptr<StubRenderWidgetDelegate> delegate = - std::make_unique<StubRenderWidgetDelegate>(); - widget()->set_delegate(delegate.get()); + widget()->set_delegate(std::make_unique<StubRenderWidgetDelegate>()); visual_properties.is_pinch_gesture_active = true; widget()->OnSynchronizeVisualProperties(visual_properties); // We do not expect the |is_pinch_gesture_active| value to propagate to the @@ -589,7 +588,6 @@ // not a pinch gesture is active, and so we shouldn't propagate this // information to the layer tree for a main-frame's widget. EXPECT_FALSE(layer_tree_host->is_external_pinch_gesture_active_for_testing()); - DestroyWidget(); } TEST_F(RenderWidgetPopupUnittest, EmulatingPopupRect) { @@ -618,11 +616,10 @@ scoped_refptr<PopupRenderWidget> parent_widget( new PopupRenderWidget(&compositor_deps_)); + parent_widget->Release(); // Balance Init(). // Emulation only happens for RenderWidgets with a delegate. - std::unique_ptr<StubRenderWidgetDelegate> delegate = - std::make_unique<StubRenderWidgetDelegate>(); - parent_widget->set_delegate(delegate.get()); + parent_widget->set_delegate(std::make_unique<StubRenderWidgetDelegate>()); // Setup emulation on the |parent_widget|. parent_widget->OnSynchronizeVisualProperties(visual_properties);