Better ownership of RenderWidgetHostImpl in RenderFrameHost.

For RenderWidgetHostImpl created on behalf of the browser process,
replace raw pointers by std::unique_ptr.

Currently, the owners of the RenderWidgetHostImpl are:
 1. The RenderViewHostImpl. In this case the main RenderFrameHostImpl
    "borrows" the RenderWidgetHostImpl from it.
 2. The local-root RenderFrameHostImpl.

For RenderWidgetHostImpl created on behalf of the renderer process, they
continue to use raw pointers and to self-destroy via IPC.

Bug: 545684, 950969
Change-Id: If08e8634d7b329cae42b2101307a54750bfb61f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1559186Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659022}

content/browser/frame_host/render_frame_host_impl.cc

@@ -769,7 +769,6 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
       frame_tree_(frame_tree),
       frame_tree_node_(frame_tree_node),
       parent_(nullptr),
-      render_widget_host_(nullptr),
       routing_id_(routing_id),
       is_waiting_for_swapout_ack_(false),
       render_frame_created_(false),
@@ -837,12 +836,29 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
     mojom::WidgetPtr widget;
     GetRemoteInterfaces()->GetInterface(&widget);
 
-    // TODO(avi): Once RenderViewHostImpl has-a RenderWidgetHostImpl, the main
-    // render frame should probably start owning the RenderWidgetHostImpl,
-    // so this logic checking for an already existing RWHI should be removed.
-    // https://crbug.com/545684
-    render_widget_host_ =
-        RenderWidgetHostImpl::FromID(GetProcess()->GetID(), widget_routing_id);
+    if (!parent_) {
+      // For main frames, the RenderWidgetHost is owned by the RenderViewHost.
+      // TODO(https://crbug.com/545684): Once RenderViewHostImpl has-a
+      // RenderWidgetHostImpl, the main render frame should probably start
+      // owning the RenderWidgetHostImpl itself.
+      DCHECK(GetLocalRenderWidgetHost());
+      DCHECK_EQ(GetLocalRenderWidgetHost()->GetRoutingID(), widget_routing_id);
+      DCHECK(!GetLocalRenderWidgetHost()->owned_by_render_frame_host());
+
+      // Make the RenderWidgetHostImpl able to call the mojo Widget interface
+      // (implemented by the RenderWidgetImpl).
+      GetLocalRenderWidgetHost()->SetWidget(std::move(widget));
+    } else {
+      // For subframes, the RenderFrameHost directly creates and owns its
+      // RenderWidgetHost.
+      DCHECK_EQ(nullptr, GetLocalRenderWidgetHost());
+      DCHECK_EQ(nullptr, RenderWidgetHostImpl::FromID(GetProcess()->GetID(),
+                                                      widget_routing_id));
+      owned_render_widget_host_ = RenderWidgetHostFactory::Create(
+          frame_tree_->render_widget_delegate(), GetProcess(),
+          widget_routing_id, std::move(widget), hidden);
+      owned_render_widget_host_->set_owned_by_render_frame_host(true);
+    }
 
     mojom::WidgetInputHandlerAssociatedPtr widget_handler;
     mojom::WidgetInputHandlerHostRequest host_request;
@@ -852,22 +868,12 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
       frame_input_handler_->GetWidgetInputHandler(
           mojo::MakeRequest(&widget_handler), std::move(host));
     }
-    if (!render_widget_host_) {
-      DCHECK(frame_tree_node->parent());
-      render_widget_host_ = RenderWidgetHostFactory::Create(
-          frame_tree_->render_widget_delegate(), GetProcess(),
-          widget_routing_id, std::move(widget), hidden);
-      render_widget_host_->set_owned_by_render_frame_host(true);
-    } else {
-      DCHECK(!render_widget_host_->owned_by_render_frame_host());
-      render_widget_host_->SetWidget(std::move(widget));
-    }
     if (!frame_tree_node_->parent())
-      render_widget_host_->SetIntersectsViewport(true);
-    render_widget_host_->SetFrameDepth(frame_tree_node_->depth());
-    render_widget_host_->SetWidgetInputHandler(std::move(widget_handler),
-                                               std::move(host_request));
-    render_widget_host_->input_router()->SetFrameTreeNodeId(
+      GetLocalRenderWidgetHost()->SetIntersectsViewport(true);
+    GetLocalRenderWidgetHost()->SetFrameDepth(frame_tree_node_->depth());
+    GetLocalRenderWidgetHost()->SetWidgetInputHandler(std::move(widget_handler),
+                                                      std::move(host_request));
+    GetLocalRenderWidgetHost()->input_router()->SetFrameTreeNodeId(
         frame_tree_node_->frame_tree_node_id());
   }
   ResetFeaturePolicy();
@@ -994,11 +1000,11 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   for (auto& iter : visual_state_callbacks_)
     std::move(iter.second).Run(false);
 
-  if (render_widget_host_ &&
-      render_widget_host_->owned_by_render_frame_host()) {
-    // Shutdown causes the RenderWidgetHost to delete itself.
-    render_widget_host_->ShutdownAndDestroyWidget(true);
-  }
+  // Note: The RenderWidgetHost of the main frame is owned by the RenderViewHost
+  // instead. In this case the RenderViewHost is responsible for shutting down
+  // its RenderViewHost.
+  if (owned_render_widget_host_)
+    owned_render_widget_host_->ShutdownAndDestroyWidget(false);
 
   // Notify the FrameTree that this RFH is going away, allowing it to shut down
   // the corresponding RenderViewHost if it is no longer needed.
@@ -1333,7 +1339,7 @@ PageVisibilityState RenderFrameHostImpl::GetVisibilityState() {
   // https://crbug.com/615867.
   RenderFrameHostImpl* frame = this;
   while (frame) {
-    if (frame->render_widget_host_)
+    if (frame->GetLocalRenderWidgetHost())
       break;
     frame = frame->GetParent();
   }
@@ -1567,10 +1573,8 @@ bool RenderFrameHostImpl::AccessibilityIsMainFrame() {
 void RenderFrameHostImpl::RenderProcessGone(SiteInstanceImpl* site_instance) {
   DCHECK_EQ(site_instance_.get(), site_instance);
 
-  if (render_widget_host_ &&
-      render_widget_host_->owned_by_render_frame_host()) {
-    render_widget_host_->RendererExited();
-  }
+  if (owned_render_widget_host_)
+    owned_render_widget_host_->RendererExited();
 
   // The renderer process is gone, so this frame can no longer be loading.
   if (GetNavigationHandle())
@@ -1710,9 +1714,10 @@ bool RenderFrameHostImpl::CreateRenderFrame(int previous_routing_id,
       frame_tree_node()->has_committed_real_load();
 
   params->widget_params = mojom::CreateFrameWidgetParams::New();
-  if (render_widget_host_) {
-    params->widget_params->routing_id = render_widget_host_->GetRoutingID();
-    params->widget_params->hidden = render_widget_host_->is_hidden();
+  if (GetLocalRenderWidgetHost()) {
+    params->widget_params->routing_id =
+        GetLocalRenderWidgetHost()->GetRoutingID();
+    params->widget_params->hidden = GetLocalRenderWidgetHost()->is_hidden();
   } else {
     // MSG_ROUTING_NONE will prevent a new RenderWidget from being created in
     // the renderer process.
@@ -1727,9 +1732,11 @@ bool RenderFrameHostImpl::CreateRenderFrame(int previous_routing_id,
   // TODO(avi): This will need to change to initialize a
   // RenderWidgetHostViewAura for the main frame once RenderViewHostImpl has-a
   // RenderWidgetHostImpl. https://crbug.com/545684
-  if (parent_routing_id != MSG_ROUTING_NONE && render_widget_host_) {
+  if (owned_render_widget_host_) {
+    DCHECK(parent_);
+    DCHECK_NE(parent_routing_id, MSG_ROUTING_NONE);
     RenderWidgetHostView* rwhv =
-        RenderWidgetHostViewChildFrame::Create(render_widget_host_);
+        RenderWidgetHostViewChildFrame::Create(owned_render_widget_host_.get());
     rwhv->Hide();
   }
 
@@ -1795,10 +1802,10 @@ void RenderFrameHostImpl::SetRenderFrameCreated(bool created) {
     }
   }
 
-  if (created && render_widget_host_) {
+  if (created && GetLocalRenderWidgetHost()) {
     mojom::WidgetPtr widget;
     GetRemoteInterfaces()->GetInterface(&widget);
-    render_widget_host_->SetWidget(std::move(widget));
+    GetLocalRenderWidgetHost()->SetWidget(std::move(widget));
 
     if (frame_input_handler_) {
       mojom::WidgetInputHandlerAssociatedPtr widget_handler;
@@ -1807,16 +1814,17 @@ void RenderFrameHostImpl::SetRenderFrameCreated(bool created) {
           mojo::MakeRequest(&host);
       frame_input_handler_->GetWidgetInputHandler(
           mojo::MakeRequest(&widget_handler), std::move(host));
-      render_widget_host_->SetWidgetInputHandler(std::move(widget_handler),
-                                                 std::move(host_request));
+      GetLocalRenderWidgetHost()->SetWidgetInputHandler(
+          std::move(widget_handler), std::move(host_request));
     }
-    render_widget_host_->input_router()->SetFrameTreeNodeId(
+    GetLocalRenderWidgetHost()->input_router()->SetFrameTreeNodeId(
         frame_tree_node_->frame_tree_node_id());
     viz::mojom::InputTargetClientPtr input_target_client;
     remote_interfaces_->GetInterface(&input_target_client);
     input_target_client_ = input_target_client.get();
-    render_widget_host_->SetInputTargetClient(std::move(input_target_client));
-    render_widget_host_->InitForFrame();
+    GetLocalRenderWidgetHost()->SetInputTargetClient(
+        std::move(input_target_client));
+    GetLocalRenderWidgetHost()->InitForFrame();
   }
 
   if (enabled_bindings_ && created) {
@@ -2275,8 +2283,8 @@ void RenderFrameHostImpl::OnUpdateState(const PageState& state) {
 RenderWidgetHostImpl* RenderFrameHostImpl::GetRenderWidgetHost() {
   RenderFrameHostImpl* frame = this;
   while (frame) {
-    if (frame->render_widget_host_)
-      return frame->render_widget_host_;
+    if (frame->GetLocalRenderWidgetHost())
+      return frame->GetLocalRenderWidgetHost();
     frame = frame->GetParent();
   }
 
@@ -6972,4 +6980,11 @@ RenderFrameHostImpl::EnsurePrefetchedSignedExchangeCache() {
   return prefetched_signed_exchange_cache_;
 }
 
+RenderWidgetHostImpl* RenderFrameHostImpl::GetLocalRenderWidgetHost() const {
+  if (!parent_)
+    return render_view_host_->GetWidget();
+  else
+    return owned_render_widget_host_.get();
+}
+
 }  // namespace content

content/browser/frame_host/render_frame_host_impl.h

@@ -435,7 +435,7 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // distinguished by owning a RenderWidgetHost, which manages input events
   // and painting for this frame and its contiguous local subtree in the
   // renderer process.
-  bool is_local_root() const { return !!render_widget_host_; }
+  bool is_local_root() const { return !!GetLocalRenderWidgetHost(); }
 
   // Returns the RenderWidgetHostImpl attached to this frame or the nearest
   // ancestor frame, which could potentially be the root. For most input
@@ -1592,6 +1592,15 @@ class CONTENT_EXPORT RenderFrameHostImpl
       FrameHostMsg_DidCommitProvisionalLoad_Params* validated_params,
       mojom::DidCommitProvisionalLoadInterfaceParamsPtr* interface_params);
 
+  // If this RenderFrameHost is a local root (i.e., either the main frame or a
+  // subframe in a different process than its parent), this returns the
+  // RenderWidgetHost corresponding to this frame. Otherwise this returns null.
+  // See also GetRenderWidgetHost(), which walks up the tree to find the nearest
+  // local root.
+  // Main frame: RenderWidgetHost is owned by the RenderViewHost.
+  // Subframe: RenderWidgetHost is owned by this RenderFrameHost.
+  RenderWidgetHostImpl* GetLocalRenderWidgetHost() const;
+
   // For now, RenderFrameHosts indirectly keep RenderViewHosts alive via a
   // refcount that calls Shutdown when it reaches zero.  This allows each
   // RenderFrameHostManager to just care about RenderFrameHosts, while ensuring
@@ -1649,13 +1658,11 @@ class CONTENT_EXPORT RenderFrameHostImpl
 
   std::map<uint64_t, VisualStateCallback> visual_state_callbacks_;
 
-  // RenderFrameHosts that need management of the rendering and input events
-  // for their frame subtrees require RenderWidgetHosts. This typically
-  // means frames that are rendered in different processes from their parent
-  // frames.
+  // Local root subframes directly own their RenderWidgetHost.
+  // Please see comments about the GetLocalRenderWidgetHost() function.
   // TODO(kenrb): Later this will also be used on the top-level frame, when
   // RenderFrameHost owns its RenderViewHost.
-  RenderWidgetHostImpl* render_widget_host_;
+  std::unique_ptr<RenderWidgetHostImpl> owned_render_widget_host_;
 
   const int routing_id_;
 

content/browser/renderer_host/input/mouse_latency_browsertest.cc

@@ -135,14 +135,14 @@ class TracingRenderWidgetHostFactory : public RenderWidgetHostFactory {
     RenderWidgetHostFactory::UnregisterFactory();
   }
 
-  RenderWidgetHostImpl* CreateRenderWidgetHost(
+  std::unique_ptr<RenderWidgetHostImpl> CreateRenderWidgetHost(
       RenderWidgetHostDelegate* delegate,
       RenderProcessHost* process,
       int32_t routing_id,
       mojom::WidgetPtr widget_interface,
       bool hidden) override {
-    return new TracingRenderWidgetHost(delegate, process, routing_id,
-                                       std::move(widget_interface), hidden);
+    return std::make_unique<TracingRenderWidgetHost>(
+        delegate, process, routing_id, std::move(widget_interface), hidden);
   }
 
  private:

content/browser/renderer_host/render_view_host_factory.cc

@@ -53,9 +53,8 @@ RenderViewHost* RenderViewHostFactory::Create(
   }
   return new RenderViewHostImpl(
       instance,
-      base::WrapUnique(RenderWidgetHostFactory::Create(
-          widget_delegate, instance->GetProcess(), widget_routing_id, nullptr,
-          hidden)),
+      RenderWidgetHostFactory::Create(widget_delegate, instance->GetProcess(),
+                                      widget_routing_id, nullptr, hidden),
       delegate, routing_id, main_frame_routing_id, swapped_out,
       true /* has_initialized_audio_host */);
 }

content/browser/renderer_host/render_widget_host_factory.cc

@@ -12,7 +12,7 @@ namespace content {
 RenderWidgetHostFactory* RenderWidgetHostFactory::factory_ = nullptr;
 
 // static
-RenderWidgetHostImpl* RenderWidgetHostFactory::Create(
+std::unique_ptr<RenderWidgetHostImpl> RenderWidgetHostFactory::Create(
     RenderWidgetHostDelegate* delegate,
     RenderProcessHost* process,
     int32_t routing_id,
@@ -22,8 +22,8 @@ RenderWidgetHostImpl* RenderWidgetHostFactory::Create(
     return factory_->CreateRenderWidgetHost(
         delegate, process, routing_id, std::move(widget_interface), hidden);
   }
-  return new RenderWidgetHostImpl(delegate, process, routing_id,
-                                  std::move(widget_interface), hidden);
+  return std::make_unique<RenderWidgetHostImpl>(
+      delegate, process, routing_id, std::move(widget_interface), hidden);
 }
 
 // static

content/browser/renderer_host/render_widget_host_factory.h

@@ -6,6 +6,7 @@
 #define CONTENT_BROWSER_RENDERER_HOST_RENDER_WIDGET_HOST_FACTORY_H_
 
 #include <stdint.h>
+#include <memory>
 
 #include "base/macros.h"
 #include "content/common/content_export.h"
@@ -24,11 +25,12 @@ class RenderWidgetHostFactory {
   // Creates a RenderWidgetHostImpl using the currently registered factory, or
   // the default one if no factory is registered. Ownership of the returned
   // pointer will be passed to the caller.
-  static RenderWidgetHostImpl* Create(RenderWidgetHostDelegate* delegate,
-                                      RenderProcessHost* process,
-                                      int32_t routing_id,
-                                      mojom::WidgetPtr widget_interface,
-                                      bool hidden);
+  static std::unique_ptr<RenderWidgetHostImpl> Create(
+      RenderWidgetHostDelegate* delegate,
+      RenderProcessHost* process,
+      int32_t routing_id,
+      mojom::WidgetPtr widget_interface,
+      bool hidden);
 
   // Returns true if there is currently a globally-registered factory.
   static bool has_factory() { return !!factory_; }
@@ -39,7 +41,7 @@ class RenderWidgetHostFactory {
 
   // You can derive from this class and specify an implementation for this
   // function to create a different kind of RenderWidgetHostImpl for testing.
-  virtual RenderWidgetHostImpl* CreateRenderWidgetHost(
+  virtual std::unique_ptr<RenderWidgetHostImpl> CreateRenderWidgetHost(
       RenderWidgetHostDelegate* delegate,
       RenderProcessHost* process,
       int32_t routing_id,

content/test/test_render_widget_host_factory.cc

@@ -16,14 +16,14 @@ TestRenderWidgetHostFactory::~TestRenderWidgetHostFactory() {
   RenderWidgetHostFactory::UnregisterFactory();
 }
 
-RenderWidgetHostImpl* TestRenderWidgetHostFactory::CreateRenderWidgetHost(
+std::unique_ptr<RenderWidgetHostImpl>
+TestRenderWidgetHostFactory::CreateRenderWidgetHost(
     RenderWidgetHostDelegate* delegate,
     RenderProcessHost* process,
     int32_t routing_id,
     mojom::WidgetPtr widget_interface,
     bool hidden) {
-  return TestRenderWidgetHost::Create(delegate, process, routing_id, hidden)
-      .release();
+  return TestRenderWidgetHost::Create(delegate, process, routing_id, hidden);
 }
 
 }  // namespace content

content/test/test_render_widget_host_factory.h

@@ -22,7 +22,7 @@ class TestRenderWidgetHostFactory : public RenderWidgetHostFactory {
   TestRenderWidgetHostFactory();
   ~TestRenderWidgetHostFactory() override;
 
-  RenderWidgetHostImpl* CreateRenderWidgetHost(
+  std::unique_ptr<RenderWidgetHostImpl> CreateRenderWidgetHost(
       RenderWidgetHostDelegate* delegate,
       RenderProcessHost* process,
       int32_t routing_id,