Cleanups in RenderWidget initialization/destruction code

Make Init() private, add an InitForPepperFullscreen() and
InitForMainFrame() that RenderWidgetFullscreenPepper and RenderViewImpl
and RenderFrameImpl make use of. Now every callsite declares its
intentions.

Remove comments about RenderWidget being self-referencing, it is not
ref counted any longer.

Use render_widget_ instead of GetWidget() in RenderViewImpl. It is
the same thing but it makes code more clear about where it is ending
up. Especially interesting is the lines:

  RenderWidget* closing_widget = render_widget_.get();
  closing_widget->Close(std::move(render_widget_));

Some cleanups in the RenderWidgetFullscreenPepper code around
construction and use of the new RenderWidget Init api. And also
in RenderFrameImpl.

R=avi@chromium.org

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

content/renderer/render_frame_impl.cc

@@ -1467,17 +1467,17 @@ RenderFrameImpl* RenderFrameImpl::CreateMainFrame(
       render_view->page_properties(), params->visual_properties.display_mode,
       /*is_undead=*/params->main_frame_routing_id == MSG_ROUTING_NONE,
       params->never_visible);
-  render_view->GetWidget()->set_delegate(render_view);
 
   RenderWidget* render_widget = render_view->GetWidget();
+  render_widget->set_delegate(render_view);
 
   // Non-owning pointer that is self-referencing and destroyed by calling
   // Close(). The RenderViewImpl has a RenderWidget already, but not a
   // WebFrameWidget, which is now attached here.
-  auto* web_frame_widget = blink::WebFrameWidget::CreateForMainFrame(
-      render_view->GetWidget(), web_frame);
+  auto* web_frame_widget =
+      blink::WebFrameWidget::CreateForMainFrame(render_widget, web_frame);
 
-  render_widget->Init(std::move(show_callback), web_frame_widget);
+  render_widget->InitForMainFrame(std::move(show_callback), web_frame_widget);
   render_view->AttachWebFrameWidget(web_frame_widget);
 
   render_widget->OnSynchronizeVisualProperties(params->visual_properties);

content/renderer/render_view_impl.cc

@@ -511,18 +511,12 @@ void RenderViewImpl::Initialize(
     render_widget_ = RenderWidget::CreateForFrame(
         params->main_frame_widget_routing_id, compositor_deps,
         page_properties(), params->visual_properties.display_mode,
-        /*is_undead=*/params->main_frame_routing_id == MSG_ROUTING_NONE,
-        params->never_visible);
-    GetWidget()->set_delegate(this);
-    // Note: The GetWidget->Init() call causes an AddRef() to itself meaning
-    // that IPC system has taken conceptual ownership of the object. Though it
-    // is tempting to have RenderView retain the RenderWidget(), this lifecycle
-    // only requires single ownership and adding scoped_refptr<RenderWidget>
-    // muddies this unnecessarily -- especially since this RenderWidget should
-    // ultimately be own by the main frame.
+        /*is_undead=*/true, params->never_visible);
+    render_widget_->set_delegate(this);
     // We intentionally pass in a null webwidget since it shouldn't be needed
     // for remote frames.
-    GetWidget()->Init(std::move(show_callback), nullptr);
+    render_widget_->InitForMainFrame(std::move(show_callback),
+                                     /*web_frame_widget=*/nullptr);
 
     RenderFrameProxy::CreateFrameProxy(params->proxy_routing_id, GetRoutingID(),
                                        opener_frame, MSG_ROUTING_NONE,
@@ -1023,7 +1017,7 @@ RenderViewImpl* RenderViewImpl::Create(
 }
 
 void RenderViewImpl::Destroy() {
-  GetWidget()->PrepareForClose();
+  render_widget_->PrepareForClose();
 
   webview_->Close();
   // The webview_ is already destroyed by the time we get here, remove any
@@ -1031,7 +1025,12 @@ void RenderViewImpl::Destroy() {
   g_view_map.Get().erase(webview_);
   webview_ = nullptr;
 
-  GetWidget()->Close(std::move(render_widget_));
+  // 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_));
 
   g_routing_id_view_map.Get().erase(GetRoutingID());
 
@@ -1069,7 +1068,7 @@ bool RenderViewImpl::RenderWidgetWillHandleMouseEventForWidget(
     const blink::WebMouseEvent& event) {
   // If the mouse is locked, only the current owner of the mouse lock can
   // process mouse events.
-  return GetWidget()->mouse_lock_dispatcher()->WillHandleMouseEvent(event);
+  return render_widget_->mouse_lock_dispatcher()->WillHandleMouseEvent(event);
 }
 
 void RenderViewImpl::SetActiveForWidget(bool active) {
@@ -1415,7 +1414,7 @@ WebView* RenderViewImpl::CreateView(
                      base::Unretained(creator_frame), opened_by_user_gesture);
 
   RenderViewImpl* view = RenderViewImpl::Create(
-      GetWidget()->compositor_deps(), std::move(view_params),
+      render_widget_->compositor_deps(), std::move(view_params),
       std::move(show_callback),
       creator->GetTaskRunner(blink::TaskType::kInternalDefault));
 
@@ -1448,7 +1447,7 @@ blink::WebPagePopup* RenderViewImpl::CreatePopup(
   // state off it for the popup.
   // TODO(crbug.com/419087): This should probably be using the local root's
   // RenderWidget for the frame making the popup.
-  RenderWidget* view_render_widget = GetWidget();
+  RenderWidget* view_render_widget = render_widget_.get();
 
   RenderWidget* popup_widget = RenderWidget::CreateForPopup(
       widget_routing_id, view_render_widget->compositor_deps(),
@@ -1512,14 +1511,14 @@ base::StringPiece RenderViewImpl::GetSessionStorageNamespaceId() {
 
 void RenderViewImpl::PrintPage(WebLocalFrame* frame) {
   RenderFrameImpl::FromWebFrame(frame)->ScriptedPrint(
-      GetWidget()->input_handler().handling_input_event());
+      render_widget_->input_handler().handling_input_event());
 }
 
 void RenderViewImpl::AttachWebFrameWidget(blink::WebFrameWidget* frame_widget) {
   // The previous WebFrameWidget must already be detached by CloseForFrame().
   DCHECK(!frame_widget_);
   frame_widget_ = frame_widget;
-  GetWidget()->SetWebWidgetInternal(frame_widget);
+  render_widget_->SetWebWidgetInternal(frame_widget);
 
   // Initialization for the WebFrameWidget that should only occur for the main
   // frame, and that uses types not allowed in blink. This should maybe be
@@ -1534,13 +1533,14 @@ void RenderViewImpl::DetachWebFrameWidget() {
   // We should detach when making the RenderWidget undead so we don't expect it
   // to be undead already. But when it is recycled for a provisional frame, then
   // we can detach when closing the provisional frame.
-  DCHECK(GetWidget()->IsUndeadOrProvisional() || GetWidget()->is_closing());
+  DCHECK(render_widget_->IsUndeadOrProvisional() ||
+         render_widget_->is_closing());
   DCHECK(frame_widget_);
   frame_widget_->Close();
   frame_widget_ = nullptr;
 
   // This just clears the webwidget_internal_ member from RenderWidget.
-  GetWidget()->SetWebWidgetInternal(nullptr);
+  render_widget_->SetWebWidgetInternal(nullptr);
 }
 
 void RenderViewImpl::SetHostZoomLevel(const GURL& url, double zoom_level) {
@@ -1768,9 +1768,8 @@ void RenderViewImpl::UpdateBrowserControlsState(
   TRACE_EVENT_INSTANT1("renderer", "is_animated", TRACE_EVENT_SCOPE_THREAD,
                        "animated", animate);
 
-  if (GetWidget() && GetWidget()->layer_tree_view()) {
-    GetWidget()
-        ->layer_tree_view()
+  if (render_widget_ && render_widget_->layer_tree_view()) {
+    render_widget_->layer_tree_view()
         ->layer_tree_host()
         ->UpdateBrowserControlsState(ContentToCc(constraints),
                                      ContentToCc(current), animate);
@@ -1814,11 +1813,11 @@ bool RenderViewImpl::CanUpdateLayout() {
 
 void RenderViewImpl::SetEditCommandForNextKeyEvent(const std::string& name,
                                                    const std::string& value) {
-  GetWidget()->SetEditCommandForNextKeyEvent(name, value);
+  render_widget_->SetEditCommandForNextKeyEvent(name, value);
 }
 
 void RenderViewImpl::ClearEditCommands() {
-  GetWidget()->ClearEditCommands();
+  render_widget_->ClearEditCommands();
 }
 
 const std::string& RenderViewImpl::GetAcceptLanguages() {
@@ -1886,7 +1885,7 @@ bool RenderViewImpl::Send(IPC::Message* message) {
   CHECK_NE(message->routing_id(), MSG_ROUTING_NONE);
 
   // Don't send any messages after the browser has told us to close.
-  if (GetWidget()->is_closing()) {
+  if (render_widget_->is_closing()) {
     delete message;
     return false;
   }
@@ -1910,7 +1909,7 @@ int RenderViewImpl::GetRoutingID() {
 }
 
 gfx::Size RenderViewImpl::GetSize() {
-  return GetWidget()->size();
+  return render_widget_->size();
 }
 
 float RenderViewImpl::GetDeviceScaleFactor() {
@@ -2175,7 +2174,7 @@ void RenderViewImpl::PageImportanceSignalsChanged() {
 }
 
 void RenderViewImpl::DidAutoResize(const blink::WebSize& newSize) {
-  GetWidget()->DidAutoResize(newSize);
+  render_widget_->DidAutoResize(newSize);
 }
 
 void RenderViewImpl::DidFocus(blink::WebLocalFrame* calling_frame) {
@@ -2259,16 +2258,16 @@ void RenderViewImpl::SetFocusAndActivateForTesting(bool enable) {
   if (webview()->MainFrame()->IsWebRemoteFrame())
     return;
 
-  if (enable == GetWidget()->has_focus())
+  if (enable == render_widget_->has_focus())
     return;
 
   if (enable) {
     SetActiveForWidget(true);
     // Fake an IPC message so go through the IPC handler.
-    GetWidget()->OnSetFocus(true);
+    render_widget_->OnSetFocus(true);
   } else {
     // Fake an IPC message so go through the IPC handler.
-    GetWidget()->OnSetFocus(false);
+    render_widget_->OnSetFocus(false);
     SetActiveForWidget(false);
   }
 }

content/renderer/render_widget.cc

@@ -509,11 +509,20 @@ void RenderWidget::InitForPopup(ShowCallback show_callback,
   Init(std::move(show_callback), web_page_popup);
 }
 
+void RenderWidget::InitForPepperFullscreen(ShowCallback show_callback,
+                                           blink::WebWidget* web_widget) {
+  pepper_fullscreen_ = true;
+  Init(std::move(show_callback), web_widget);
+}
+
+void RenderWidget::InitForMainFrame(ShowCallback show_callback,
+                                    blink::WebFrameWidget* web_frame_widget) {
+  Init(std::move(show_callback), web_frame_widget);
+}
+
 void RenderWidget::InitForChildLocalRoot(
     blink::WebFrameWidget* web_frame_widget) {
   for_child_local_root_frame_ = true;
-  // Init() increments the reference count on |this|, making it
-  // self-referencing.
   Init(base::NullCallback(), web_frame_widget);
 }
 

content/renderer/render_widget.h

@@ -221,16 +221,23 @@ class CONTENT_EXPORT RenderWidget
       mojo::PendingReceiver<mojom::Widget> widget_receiver);
 
   // Initialize a new RenderWidget for a popup. The |show_callback| is called
-  // when RenderWidget::Show() happens. This method increments the reference
-  // count on the RenderWidget, making it self-referencing, which is then
-  // release when a WidgetMsg_Close IPC is received.
+  // when RenderWidget::Show() happens.
   void InitForPopup(ShowCallback show_callback,
                     blink::WebPagePopup* web_page_popup);
 
+  // Initialize a new RenderWidget for pepper fullscreen. The |show_callback| is
+  // called when RenderWidget::Show() happens.
+  void InitForPepperFullscreen(ShowCallback show_callback,
+                               blink::WebWidget* web_widget);
+
+  // Initialize a new RenderWidget that will be attached to a RenderFrame (via
+  // the WebFrameWidget), for a frame that is a main frame.
+  void InitForMainFrame(ShowCallback show_callback,
+                        blink::WebFrameWidget* web_frame_widget);
+
   // Initialize a new RenderWidget that will be attached to a RenderFrame (via
   // the WebFrameWidget), for a frame that is a local root, but not the main
-  // frame. This method increments the reference count on the RenderWidget,
-  // making it self-referencing, which can be released by calling Close().
+  // frame.
   void InitForChildLocalRoot(blink::WebFrameWidget* web_frame_widget);
 
   // Sets a delegate to handle certain RenderWidget operations that need an
@@ -691,11 +698,6 @@ class CONTENT_EXPORT RenderWidget
     in_synchronous_composite_for_testing_ = in;
   }
 
-  // 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.
-  void Init(ShowCallback show_callback, blink::WebWidget* web_widget);
-
   base::WeakPtr<RenderWidget> AsWeakPtr();
 
   // TODO(https://crbug.com/995981): Eventually, the lifetime of RenderWidget
@@ -708,14 +710,6 @@ class CONTENT_EXPORT RenderWidget
   // Notify subclasses that we initiated the paint operation.
   virtual void DidInitiatePaint() {}
 
-  // RenderWidgets are created for frames, popups and pepper fullscreen. In the
-  // former case, the caller frame takes ownership and eventually passes the
-  // unique_ptr back in Close(). In the latter cases, the browser process takes
-  // ownership via IPC.  These booleans exist to allow us to confirm than an IPC
-  // message to kill the render widget is coming for a popup or fullscreen.
-  bool popup_ = false;
-  bool pepper_fullscreen_ = false;
-
  private:
   // Friend RefCounted so that the dtor can be non-public. Using this class
   // without ref-counting is an error.
@@ -735,6 +729,11 @@ class CONTENT_EXPORT RenderWidget
 
   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.
+  void Init(ShowCallback show_callback, blink::WebWidget* web_widget);
+
   // Creates the compositor, but leaves it in a stopped state, where it will
   // not set up IPC channels or begin trying to produce frames until started
   // via StartStopCompositor().
@@ -1129,6 +1128,13 @@ class CONTENT_EXPORT RenderWidget
   // that are not for a frame (eg popups) and excludes the widget for the main
   // frame (which is attached to the RenderViewImpl).
   bool for_child_local_root_frame_ = false;
+  // RenderWidgets are created for frames, popups and pepper fullscreen. In the
+  // former case, the caller frame takes ownership and eventually passes the
+  // unique_ptr back in Close(). In the latter cases, the browser process takes
+  // ownership via IPC.  These booleans exist to allow us to confirm than an IPC
+  // message to kill the render widget is coming for a popup or fullscreen.
+  bool popup_ = false;
+  bool pepper_fullscreen_ = false;
 
   // A callback into the creator/opener of this widget, to be executed when
   // WebWidgetClient::Show() occurs.

content/renderer/render_widget_fullscreen_pepper.cc

@@ -284,8 +284,8 @@ RenderWidgetFullscreenPepper* RenderWidgetFullscreenPepper::Create(
   RenderWidgetFullscreenPepper* widget = new RenderWidgetFullscreenPepper(
       routing_id, compositor_deps, page_properties, plugin,
       std::move(widget_receiver));
-  widget->Init(std::move(show_callback),
-               new PepperWidget(widget, local_main_frame_url));
+  widget->InitForPepperFullscreen(
+      std::move(show_callback), new PepperWidget(widget, local_main_frame_url));
   return widget;
 }
 
@@ -298,16 +298,14 @@ RenderWidgetFullscreenPepper::RenderWidgetFullscreenPepper(
     : RenderWidget(routing_id,
                    compositor_deps,
                    page_properties,
-                   blink::kWebDisplayModeUndefined,
-                   false,
-                   false,
-                   false,
+                   /*display_mode=*/blink::kWebDisplayModeUndefined,
+                   /*is_undead=*/false,
+                   /*hidden=*/false,
+                   /*never_visible=*/false,
                    std::move(widget_receiver)),
       plugin_(plugin),
-      layer_(nullptr),
-      mouse_lock_dispatcher_(new FullscreenMouseLockDispatcher(this)) {
-  pepper_fullscreen_ = true;
-}
+      mouse_lock_dispatcher_(
+          std::make_unique<FullscreenMouseLockDispatcher>(this)) {}
 
 RenderWidgetFullscreenPepper::~RenderWidgetFullscreenPepper() {
 }

content/renderer/render_widget_fullscreen_pepper.h

@@ -79,7 +79,7 @@ class RenderWidgetFullscreenPepper : public RenderWidget,
   // The plugin instance this widget wraps.
   PepperPluginInstanceImpl* plugin_;
 
-  cc::Layer* layer_;
+  cc::Layer* layer_ = nullptr;
 
   std::unique_ptr<MouseLockDispatcher> mouse_lock_dispatcher_;