Remove 2-stage RenderWidget initialization

content/browser/renderer_host/render_view_host_impl.cc

Index: content/browser/renderer_host/render_view_host_impl.cc
diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc
index 7897b7a4c094ca5a7e5e52747dd712cba7a71024..4c2c1fac8b5880504604f97372ad5cbba1bfc173 100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -224,6 +224,7 @@ RenderViewHostImpl::RenderViewHostImpl(
       virtual_keyboard_requested_(false),
       is_focused_element_editable_(false),
       updating_web_preferences_(false),
+      render_view_ready_on_process_launch_(false),
       weak_factory_(this) {
   DCHECK(instance_.get());
   CHECK(delegate_);  // http://crbug.com/82827
@@ -351,6 +352,8 @@ bool RenderViewHostImpl::CreateRenderView(
     RenderFrameHostImpl::FromID(GetProcess()->GetID(), main_frame_routing_id_)
         ->SetRenderFrameCreated(true);
   }
+  SendScreenRects();
+  PostRenderViewReady();
 
   return true;
 }
@@ -582,6 +585,13 @@ void RenderViewHostImpl::RequestFindMatchRects(int current_version) {
 }
 #endif
 
+void RenderViewHostImpl::RenderProcessReady(RenderProcessHost* host) {
+  if (render_view_ready_on_process_launch_) {
+    render_view_ready_on_process_launch_ = false;
+    RenderViewReady();
+  }
+}
+
 void RenderViewHostImpl::RenderProcessExited(RenderProcessHost* host,
                                              base::TerminationStatus status,
                                              int exit_code) {
@@ -903,7 +913,6 @@ bool RenderViewHostImpl::OnMessageReceived(const IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(ViewHostMsg_ShowWidget, OnShowWidget)
     IPC_MESSAGE_HANDLER(ViewHostMsg_ShowFullscreenWidget,
                         OnShowFullscreenWidget)
-    IPC_MESSAGE_HANDLER(ViewHostMsg_RenderViewReady, OnRenderViewReady)
     IPC_MESSAGE_HANDLER(ViewHostMsg_UpdateState, OnUpdateState)
     IPC_MESSAGE_HANDLER(ViewHostMsg_UpdateTargetURL, OnUpdateTargetURL)
     IPC_MESSAGE_HANDLER(ViewHostMsg_Close, OnClose)
@@ -934,6 +943,7 @@ bool RenderViewHostImpl::OnMessageReceived(const IPC::Message& msg) {
 
 void RenderViewHostImpl::Init() {
   RenderWidgetHostImpl::Init();
+  PostRenderViewReady();
 }
 
 void RenderViewHostImpl::Shutdown() {
@@ -1022,13 +1032,6 @@ void RenderViewHostImpl::OnShowFullscreenWidget(int route_id) {
   Send(new ViewMsg_Move_ACK(route_id));
 }
 
-void RenderViewHostImpl::OnRenderViewReady() {
-  render_view_termination_status_ = base::TERMINATION_STATUS_STILL_RUNNING;
-  SendScreenRects();
-  WasResized();
-  delegate_->RenderViewReady(this);
-}
-
 void RenderViewHostImpl::OnRenderProcessGone(int status, int exit_code) {
   // Do nothing, otherwise RenderWidgetHostImpl will assume it is not a
   // RenderViewHostImpl and destroy itself.
@@ -1426,4 +1429,20 @@ void RenderViewHostImpl::SelectWordAroundCaret() {
   Send(new ViewMsg_SelectWordAroundCaret(GetRoutingID()));
 }
 
+void RenderViewHostImpl::PostRenderViewReady() {
+  if (GetProcess()->IsReady()) {
+    BrowserThread::PostTask(

[ncarter (slow), 2015/09/29 18:28:40]

How confident are you that this posted task won't be in a race with later IPCs
from the renderer?

The worry I have is that there might be some code somewhere that isn't expecting
any IPCs prior to
RenderViewReady/WCO::RenderViewReady()/NOTIFICATION_EXTENSION_HOST_CREATED/NOTIFICATION_WEB_CONTENTS_CONNECTED
etc.

I suppose we could test this by forcing the race outcome: e.g. adding a
sleep(1000) before the PostTask here. If browser_tests and content_browsertests
pass with that sleep() added, then I'm probably okay with calling this safe.

[piman, 2015/09/29 23:54:27]

So, it is already the case today that IPCs can be sent before
ViewHostMsg_RenderViewReady (e.g. before this patch, since we initialize the
compositor before sending ViewHostMsg_RenderViewReady, we need to do some IPCs
to the browser, e.g. to initialize the context).

Regardless, this patch only makes it better in that respect - instead of
UI->IO->Renderer IO->Renderer Main->Renderer IO->IO->UI->RenderViewReady, we do
UI->UI->RenderViewReady, i.e. RenderViewReady should happen no later in terms of
task ordering on the UI thread (This posts the task from the same function that
sends the message that would have triggered the ViewHostMsg_RenderViewReady on
the renderer side).
Now, of course, it is possible that this patch exposes existing races. I sent
https://codereview.chromium.org/1380573003/ to the trybots, which is the same
patch but changing this into a PostDelayedTask(..., 1000ms). It looks good so
far (modulo bots unable to check out the code), but will wait for completion
before landing.

[ncarter (slow), 2015/09/30 20:30:33]

Ah, okay. I had assumed that RenderViewReady was happening at some deterministic
sequence point in the IPC stream from a new renderer process, and I was worried
that we'd be introducing nondeterminism. If it was already nondeterministic,
then I'm much less worried about regression.

+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&RenderViewHostImpl::RenderViewReady,
+                   weak_factory_.GetWeakPtr()));

[ncarter (slow), 2015/09/29 18:28:40]

Also, what breaks if you just call RenderViewReady directly?

[piman, 2015/09/29 23:54:27]

There's at least a couple of places that I found, that basically don't expect
RenderViewReady to be called reentrantly.

In BrowserPluginGuest::OnWillAttachComplete, it will call RVHI::Init() (which
signals RenderViewReady), but in the callback expects to find the view on the
WebContentsImpl, whereas it's only set up later.

In the task manager, the TabContentsInformation expects in GetOwnership (which
is called indirectly from RenderViewReady) to find the WebContents in the list
of tabs in the browser. This ends up depending on the implementation of
chrome::Navigate, which does both triggering the navigation (indirectly calling
RenderViewHostImpl::CreateRenderView) and attaching the WebContents into the
list of tabs, but in various orders depending on the case.


It may be possible to fix those, but both of these are security sensitive and
way out of my area of expertise, so I prefer to keep this patch scoped, and not
introduce unexpected reentrancy, since in the end it feels like an extra
optimization.

[ncarter (slow), 2015/09/30 20:30:33]

This makes sense.

+  } else {
+    render_view_ready_on_process_launch_ = true;
+  }
+}
+
+void RenderViewHostImpl::RenderViewReady() {
+  delegate_->RenderViewReady(this);
+}
+
 }  // namespace content