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 0cda89d..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..ed6447b 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..8698fcc 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 6c83151..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..82e05e9 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 3e525b0..caf21b4 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);
