Remove 2-stage RenderWidget initialization

content/browser/renderer_host/render_process_host_impl.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Represents the browser side of the browser <--> renderer communication
 // channel. There will be one RenderProcessHost per renderer process.
 
 #include "content/browser/renderer_host/render_process_host_impl.h"
 
 #include <algorithm>
@@ skipping 1465 matching lines @@
 base::ProcessHandle RenderProcessHostImpl::GetHandle() const {
   if (run_renderer_in_process())
     return base::GetCurrentProcessHandle();
 
   if (!child_process_launcher_.get() || child_process_launcher_->IsStarting())
     return base::kNullProcessHandle;
 
   return child_process_launcher_->GetProcess().Handle();
 }
 
+bool RenderProcessHostImpl::IsReady() const {

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

It might be worthwhile to add a comment here to point out that the
channel-connection (which initializes GetPeerPID) and the
result-of-process-launch (GetHandle()) can happen in either ordering.

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

Done.

+  return GetHandle() && channel_ &&
+         (channel_->GetPeerPID() != base::kNullProcessId);
+}
+
 bool RenderProcessHostImpl::Shutdown(int exit_code, bool wait) {
   if (run_renderer_in_process())
     return false;  // Single process mode never shuts down the renderer.
 
 #if defined(OS_ANDROID)
   // Android requires a different approach for killing.
   StopChildProcess(GetHandle());
   return true;
 #else
   if (!child_process_launcher_.get() || child_process_launcher_->IsStarting())
@@ skipping 99 matching lines @@
       IPC::Message* reply = IPC::SyncMessage::GenerateReply(&msg);
       reply->set_reply_error();
       Send(reply);
     }
     return true;
   }
   return listener->OnMessageReceived(msg);
 }
 
 void RenderProcessHostImpl::OnChannelConnected(int32 peer_pid) {
+  if (IsReady()) {
+    // Send RenderProcessReady only if we already received the process handle.
+    FOR_EACH_OBSERVER(RenderProcessHostObserver,
+                      observers_,
+                      RenderProcessReady(this));
+  }
+
 #if defined(IPC_MESSAGE_LOG_ENABLED)
   Send(new ChildProcessMsg_SetIPCLoggingEnabled(
       IPC::Logging::GetInstance()->Enabled()));
 #endif
 
   tracked_objects::ThreadData::Status status =
       tracked_objects::ThreadData::status();
   Send(new ChildProcessMsg_SetProfilerStatus(status));
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
@@ skipping 782 matching lines @@
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile6(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465841 "
           "RenderProcessHostImpl::OnProcessLaunched::SendQueuedMessages"));
   while (!queued_messages_.empty()) {
     Send(queued_messages_.front());
     queued_messages_.pop();
   }
 
+  if (IsReady()) {
+    // Send RenderProcessReady only if the channel is already connected.
+    FOR_EACH_OBSERVER(RenderProcessHostObserver,
+                      observers_,
+                      RenderProcessReady(this));

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

The intent here, right, is to call RenderProcessReady from exactly one of
OnProcessLaunched or OnChannelConnected, whichever occurs last?

We were actually just working through the ChildProcessHost variant of this
pattern, http://crbug.com/484937. In my experiments on that bug, it seemed like
OnChannelConnected usually happened first -- do you know if we have adequate
test coverage of the other ordering?

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

Right, the contract is, RenderProcessReady is only sent once both of:
- GetHandle() returns a valid handle
- the channel is connected (channel_->GetPeerPid() is valid)
are true.
(this corresponds to IsReady() is true).

GetHandle() is valid once OnProcessLaunched() is called (as a posted task from
the process launcher thread)
The channel is known to be connected once OnChannelConnected() is called (as a
posted task from the IO thread, after the newly running process sends the hello
message).

The 2 posted tasks race with each other, but since they are separate tasks, only
one of the callback will observe the IsReady() transition from false to true.
Actually, here, I found that it was usually the opposite, GetHandle() being
valid but the channel was not yet connected (I had a patch originally that only
waited for the process to be launched, but one of the tests broke on Windows,
namely
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
, see ExtensionManagementApiEscalationTest /
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
as to why).

In a way it makes sense, because the LAUNCHER->UI hop is generally shorter than
the LAUNCHER->new process main->IO->UI multiple hop.

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

I think I had it somewhat backwards, but the behavior probably varies quite a
bit depending on the OS and number of cores. In my own testing, on Windows/z620,
I actually was having to sandbag the child process launcher to force the
channel-connection to win -- so that matches what you've seen. But in the
originally-reported bug (which was reported against nacl launching, on cros),
the channel-connection was winning on average. It may be that there are extra
process hops involved in nacl processes that delay the result of process
creation? Something something zygote process?

+  }

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

I like how this operates, but I'm wondering if we should be worried about it
evolving so that RenderProcessReady comes to be called twice?

We could protect against that by adding an explicit, redundant boolean member
(process_ready_) to protect against double notification, or else to make sure
that there's a DCHECK in one of the observers_ (maybe a test-only,
ContentBrowserSanityChecker style DCHECK?).

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

I added a DCHECK. I'm fairly confident that we call only once (see above why).

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

Great. I'm confident that it's correct now, but I worry that it could be a
little fragile as the code evolves.

+

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

FYI: an alternative approach discussed in bug 484937, is to just initialize
Handle() from |peer_pid| at OnChannelConnected time, and do the notification
exclusively from OnChannelConnected. This has the advantage of removing the
latency impact of waiting for the process-launch result, but it puts a little
more trust in |peer_pid| (though I'd hope that's okay since there's the
authentication secret).

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

I believe GetHandle and channel_->GetPeerPid return different things (they're a
different type on Windows). As much as it looks like we can extract the process
id from the process handle, I don't know that the opposite is true (not a
Windows expert here....)
I also think there may be places where we need the process handle before the
channel is connected. E.g. the task manager uses the process handle to do
things.

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

You definitely can go pid->handle on Windows, but it's a syscall to do so, and
it'll fail if the process has died in the meantime. Usually those failure cases
are a headache to deal with and it's a better practice to keep things as handles
all the way through. However, this here is the earliest possible moment.

In the ChildProcessHost case, we actually already open the handle from the pid,
just before calling OnChannelConnected:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/chi....
It was because we already had a handle right there, in the calling stack frame,
that I started wondering if we should just use that.

Definitely waiting for both results (as your patch does) feels like the
cleanest, safest approach. There are latency savings to be had only if it's
common for the channel to connect before we hear back from the launcher.

 #if defined(ENABLE_WEBRTC)
   // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/465841
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile7(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465841 RenderProcessHostImpl::OnProcessLaunched::EnableAec"));
   if (WebRTCInternals::GetInstance()->IsAudioDebugRecordingsEnabled()) {
     EnableAudioDebugRecordings(
         WebRTCInternals::GetInstance()->GetAudioDebugRecordingsFilePath());
   }
@@ skipping 145 matching lines @@
 void RenderProcessHostImpl::GetAudioOutputControllers(
     const GetAudioOutputControllersCallback& callback) const {
   audio_renderer_host()->GetOutputControllers(callback);
 }
 
 BluetoothDispatcherHost* RenderProcessHostImpl::GetBluetoothDispatcherHost() {
   return bluetooth_dispatcher_host_.get();
 }
 
 }  // namespace content