Always keep a ChannelProxy alive in RenderProcessHostImpl

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 665 matching lines @@
 RenderProcessHostImpl::RenderProcessHostImpl(
     BrowserContext* browser_context,
     StoragePartitionImpl* storage_partition_impl,
     bool is_for_guests_only)
     : fast_shutdown_started_(false),
       deleting_soon_(false),
 #ifndef NDEBUG
       is_self_deleted_(false),
 #endif
       pending_views_(0),
-      child_token_(mojo::edk::GenerateRandomToken()),
       service_worker_ref_count_(0),
       shared_worker_ref_count_(0),
       is_worker_ref_count_disabled_(false),
       route_provider_binding_(this),
       associated_interface_provider_bindings_(
           mojo::BindingSetDispatchMode::WITH_CONTEXT),
       visible_widgets_(0),
       is_process_backgrounded_(false),
-      is_initialized_(false),
       id_(ChildProcessHostImpl::GenerateChildProcessUniqueId()),
       browser_context_(browser_context),
       storage_partition_impl_(storage_partition_impl),
       sudden_termination_allowed_(true),
       ignore_input_events_(false),
       is_for_guests_only_(is_for_guests_only),
       gpu_observer_registered_(false),
       delayed_cleanup_needed_(false),
       within_process_died_observer_(false),
       power_monitor_broadcaster_(this),
@@ skipping 40 matching lines @@
   // single process modes, the global attachment broker is the privileged
   // attachment broker, rather than an unprivileged attachment broker.
 #if defined(OS_MACOSX)
   IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded(
       MachBroker::GetInstance());
 #else
   IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded();
 #endif  // defined(OS_MACOSX)
 #endif  // USE_ATTACHMENT_BROKER
 
-  scoped_refptr<base::SequencedTaskRunner> io_task_runner =
-      BrowserThread::GetTaskRunnerForThread(BrowserThread::IO);
-  shell::Connector* connector =
-      BrowserContext::GetConnectorFor(browser_context_);
-  // Some embedders may not initialize Mojo or the shell connector for a browser
-  // context (e.g. Android WebView)... so just fall back to the per-process
-  // connector.
-  if (!connector) {
-    // Additionally, some test code may not initialize the process-wide
-    // ServiceManagerConnection prior to this point. This class of test code
-    // doesn't care about render processes so we can initialize a dummy one.
-    if (!ServiceManagerConnection::GetForProcess()) {
-      shell::mojom::ServiceRequest request = mojo::GetProxy(&test_service_);
-      ServiceManagerConnection::SetForProcess(ServiceManagerConnection::Create(
-          std::move(request), io_task_runner));
-    }
-    connector = ServiceManagerConnection::GetForProcess()->GetConnector();
-  }
-  child_connection_.reset(new ChildConnection(
-      kRendererServiceName,
-      base::StringPrintf("%d_%d", id_, instance_id_++), child_token_, connector,
-      io_task_runner));
+  InitializeChannelProxy();
 }
 
 // static
 void RenderProcessHostImpl::ShutDownInProcessRenderer() {
   DCHECK(g_run_renderer_in_process_);
 
   switch (g_all_hosts.Pointer()->size()) {
     case 0:
       return;
     case 1: {
@@ skipping 33 matching lines @@
 
   if (gpu_observer_registered_) {
     ui::GpuSwitchingManager::GetInstance()->RemoveObserver(this);
     gpu_observer_registered_ = false;
   }
 
 #if USE_ATTACHMENT_BROKER
   IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
       channel_.get());
 #endif
-  // We may have some unsent messages at this point, but that's OK.
-  channel_.reset();
-  queued_messages_ = MessageQueue{};
 
   UnregisterHost(GetID());
 
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableGpuShaderDiskCache)) {
     BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                             base::Bind(&RemoveShaderInfo, GetID()));
   }
 }
 
-void RenderProcessHostImpl::EnableSendQueue() {
-  is_initialized_ = false;

[ncarter (slow), 2016/10/12 23:32:42]

I'd never thought deeply about this function before, but wow, it seems super
weird that when we create a RVHImpl for an already-launched
RenderProcessHostImpl, we would pause the channel. I don't think is_initialized_
would ever transition back to true in that case in the old code (since Init()
would return true). What a mess!

[Ken Rockot(use gerrit already), 2016/10/12 23:56:05]

Yes, RPHI is full of surprises :)

-}
-
 bool RenderProcessHostImpl::Init() {
   // calling Init() more than once does nothing, this makes it more convenient
   // for the view host which may not be sure in some cases
-  if (channel_)
+  if (HasConnection())
     return true;
 
+  is_dead_ = false;
+
   base::CommandLine::StringType renderer_prefix;
   // A command prefix is something prepended to the command line of the spawned
   // process.
   const base::CommandLine& browser_command_line =
       *base::CommandLine::ForCurrentProcess();
   renderer_prefix =
       browser_command_line.GetSwitchValueNative(switches::kRendererCmdPrefix);
 
 #if defined(OS_LINUX)
   int flags = renderer_prefix.empty() ? ChildProcessHost::CHILD_ALLOW_SELF
                                       : ChildProcessHost::CHILD_NORMAL;
 #else
   int flags = ChildProcessHost::CHILD_NORMAL;
 #endif
 
   // Find the renderer before creating the channel so if this fails early we
   // return without creating the channel.
   base::FilePath renderer_path = ChildProcessHost::GetChildPath(flags);
   if (renderer_path.empty())
     return false;
 
-  channel_connected_ = false;
   sent_render_process_ready_ = false;
 
-  // Setup the IPC channel.
-  channel_ = CreateChannelProxy();
-
-  // Note that Channel send is effectively paused and unpaused at various points
-  // during startup, and existing code relies on a fragile relative message
-  // ordering resulting from some early messages being queued until process
-  // launch while others are sent immediately.
+  // Unpause the Channel briefly. This will be paused again below if we launch a
+  // real child process. Note that messages may be sent in the short window
+  // between now and then (e.g. in response to RenderProcessWillLaunch) and we
+  // depend on those messages being sent right away.
   //
-  // We acquire a few associated interface proxies here -- before the channel is
-  // paused -- to ensure that subsequent initialization messages on those
-  // interfaces behave properly. Specifically, this avoids the risk of an
-  // interface being requested while the Channel is paused, effectively
-  // blocking the transmission of a subsequent message on the interface which
-  // may be sent while the Channel is unpaused.
-  //
-  // See OnProcessLaunched() for some additional details of this somewhat
-  // surprising behavior.
-  channel_->GetRemoteAssociatedInterface(&remote_route_provider_);
-
-  std::unique_ptr<AssociatedInterfaceHolder<mojom::Renderer>> holder =
-      base::MakeUnique<AssociatedInterfaceHolder<mojom::Renderer>>();
-  channel_->GetRemoteAssociatedInterface(&holder->proxy());
-  SetUserData(kRendererInterfaceKeyName, holder.release());
+  // |channel_| must always be non-null here: either it was initialized in
+  // the constructor, or in the most recent call to ProcessDied().
+  DCHECK(channel_);
+  channel_->Unpause(false /* flush */);
 
   // Call the embedder first so that their IPC filters have priority.
   GetContentClient()->browser()->RenderProcessWillLaunch(this);
 
 #if !defined(OS_MACOSX)
   // Intentionally delay the hang monitor creation after the first renderer
   // is created. On Mac audio thread is the UI thread, a hang monitor is not
   // necessary or recommended.
   media::AudioManager::StartHangMonitorIfNeeded(
       BrowserThread::GetTaskRunnerForThread(BrowserThread::IO));
@@ skipping 55 matching lines @@
     // at this stage.
     child_process_launcher_.reset(new ChildProcessLauncher(
         new RendererSandboxedProcessLauncherDelegate(channel_.get()), cmd_line,
         GetID(), this, child_token_,
         base::Bind(&RenderProcessHostImpl::OnMojoError, id_)));
     channel_->Pause();
 
     fast_shutdown_started_ = false;
   }
 
-  // Push any pending messages to the channel now. Note that if the child
-  // process is still launching, the channel will be paused and outgoing
-  // messages will be queued internally by the channel.
-  while (!queued_messages_.empty()) {
-    channel_->Send(queued_messages_.front().release());
-    queued_messages_.pop();
-  }
-
   if (!gpu_observer_registered_) {
     gpu_observer_registered_ = true;
     ui::GpuSwitchingManager::GetInstance()->AddObserver(this);
   }
 
   power_monitor_broadcaster_.Init();
 
   is_initialized_ = true;
   init_time_ = base::TimeTicks::Now();
   return true;
 }
 
-std::unique_ptr<IPC::ChannelProxy> RenderProcessHostImpl::CreateChannelProxy() {
-  scoped_refptr<base::SingleThreadTaskRunner> runner =
+void RenderProcessHostImpl::InitializeChannelProxy() {
+  // Generate a token used to identify the new child process.
+  child_token_ = mojo::edk::GenerateRandomToken();
+
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner =
       BrowserThread::GetTaskRunnerForThread(BrowserThread::IO);
+
+  // Acquire a Connector which will route connections to a new instance of the
+  // renderer service.
+  shell::Connector* connector =
+      BrowserContext::GetConnectorFor(browser_context_);
+  if (!connector) {
+    // Note that some embedders (e.g. Android WebView) may not initialize a
+    // Connector per BrowserContext. In those cases we fall back to the
+    // browser-wide Connector.
+    if (!ServiceManagerConnection::GetForProcess()) {
+      // Additionally, some test code may not initialize the process-wide
+      // ServiceManagerConnection prior to this point. This class of test code
+      // doesn't care about render processes, so we can initialize a dummy
+      // connection.
+      shell::mojom::ServiceRequest request = mojo::GetProxy(&test_service_);
+      ServiceManagerConnection::SetForProcess(ServiceManagerConnection::Create(
+          std::move(request), io_task_runner));
+    }
+    connector = ServiceManagerConnection::GetForProcess()->GetConnector();
+  }
+
+  // Establish a ServiceManager connection for the new render service instance.
+  child_connection_.reset(new ChildConnection(
+      kRendererServiceName,
+      base::StringPrintf("%d_%d", id_, instance_id_++), child_token_, connector,

[ncarter (slow), 2016/10/12 23:32:42]

Is instance_id_ needed for correctness, if we generate a new child_token_ each
time? (I can see how a sequence number would be useful for debugging, and am
fine keeping it if that's why it's here; just curious if there's a subtle other
purpose)

[Ken Rockot(use gerrit already), 2016/10/12 23:56:05]

It is needed for correctness. These two identifiers are used at different,
independent layers of the system.

The token is used to identify the child process at the internal Mojo layer,
while the instance ID is used to disambiguate the render service instance from
the ServiceManager's perspective.

+      io_task_runner));
+
+  // Send an interface request to bootstrap the IPC::Channel. Note that this
+  // request will happily sit on the pipe until the process is launched and
+  // connected to the ServiceManager. We take the other end immediately and
+  // plug it into a new ChannelProxy.
   IPC::mojom::ChannelBootstrapPtr bootstrap;
   GetRemoteInterfaces()->GetInterface(&bootstrap);
   std::unique_ptr<IPC::ChannelFactory> channel_factory =
       IPC::ChannelMojo::CreateServerFactory(
-          bootstrap.PassInterface().PassHandle(), runner);
+          bootstrap.PassInterface().PassHandle(), io_task_runner);
 
-  std::unique_ptr<IPC::ChannelProxy> channel;
+#if USE_ATTACHMENT_BROKER
+  if (channel_) {
+    IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
+        channel_.get());
+  }
+#endif
+
+  channel_.reset();
+  channel_connected_ = false;
+
   // Do NOT expand ifdef or run time condition checks here! Synchronous
   // IPCs from browser process are banned. It is only narrowly allowed
   // for Android WebView to maintain backward compatibility.
   // See crbug.com/526842 for details.
 #if defined(OS_ANDROID)
-  if (GetContentClient()->UsingSynchronousCompositing())
-    channel = IPC::SyncChannel::Create(this, runner.get(), &never_signaled_);
+  if (GetContentClient()->UsingSynchronousCompositing()) {
+    channel_ = IPC::SyncChannel::Create(
+        this, io_task_runner.get(), &never_signaled_);
+  }
 #endif  // OS_ANDROID
-  if (!channel)
-    channel.reset(new IPC::ChannelProxy(this, runner.get()));
+  if (!channel_)
+    channel_.reset(new IPC::ChannelProxy(this, io_task_runner.get()));
 #if USE_ATTACHMENT_BROKER
   IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
-      channel.get(), runner);
+      channel_.get(), io_task_runner);
 #endif
-  channel->Init(std::move(channel_factory), true /* create_pipe_now */);
+  channel_->Init(std::move(channel_factory), true /* create_pipe_now */);
 
-  return channel;
+  // Note that Channel send is effectively paused and unpaused at various points
+  // during startup, and existing code relies on a fragile relative message
+  // ordering resulting from some early messages being queued until process
+  // launch while others are sent immediately. See https://goo.gl/REW75h for
+  // details.
+  //
+  // We acquire a few associated interface proxies here -- before the channel is
+  // paused -- to ensure that subsequent initialization messages on those
+  // interfaces behave properly. Specifically, this avoids the risk of an
+  // interface being requested while the Channel is paused, which could
+  // effectively and undesirably block the transmission of a subsequent message
+  // on that interface while the Channel is unpaused.
+  //
+  // See OnProcessLaunched() for some additional details of this somewhat
+  // surprising behavior.
+  channel_->GetRemoteAssociatedInterface(&remote_route_provider_);
+
+  std::unique_ptr<AssociatedInterfaceHolder<mojom::Renderer>> holder =
+      base::MakeUnique<AssociatedInterfaceHolder<mojom::Renderer>>();
+  channel_->GetRemoteAssociatedInterface(&holder->proxy());
+  SetUserData(kRendererInterfaceKeyName, holder.release());
+
+  // We start the Channel in a paused state. It will be briefly unpaused again
+  // in Init() if applicable, before process launch is initiated.
+  channel_->Pause();
 }
 
 void RenderProcessHostImpl::CreateMessageFilters() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   AddFilter(new ResourceSchedulerFilter(GetID()));
   MediaInternals* media_internals = MediaInternals::GetInstance();
   // Add BrowserPluginMessageFilter to ensure it gets the first stab at messages
   // from guests.
   scoped_refptr<BrowserPluginMessageFilter> bp_message_filter(
       new BrowserPluginMessageFilter(GetID()));
@@ skipping 909 matching lines @@
 
   ProcessDied(false /* already_dead */, nullptr);
   return true;
 }
 
 bool RenderProcessHostImpl::Send(IPC::Message* msg) {
   TRACE_EVENT0("renderer_host", "RenderProcessHostImpl::Send");
 
   std::unique_ptr<IPC::Message> message(msg);
 
+  // |channel_| is only null after Cleanup(), at which point we don't care about
+  // delivering any messages.
+  if (!channel_)
+    return false;
+
 #if !defined(OS_ANDROID)
   DCHECK(!message->is_sync());
 #endif
 
-  if (!channel_) {
-#if defined(OS_ANDROID)
-    if (message->is_sync())
-      return false;
-#endif
-    if (!is_initialized_) {
-      queued_messages_.emplace(std::move(message));
-      return true;
-    }
-    return false;
-  }
-
 #if defined(OS_ANDROID)
   if (child_process_launcher_.get() && child_process_launcher_->IsStarting() &&
       message->is_sync()) {
     return false;
   }
 #endif
 
   return channel_->Send(message.release());
 }
 
@@ skipping 97 matching lines @@
 bool RenderProcessHostImpl::InSameStoragePartition(
     StoragePartition* partition) const {
   return storage_partition_impl_ == partition;
 }
 
 int RenderProcessHostImpl::GetID() const {
   return id_;
 }
 
 bool RenderProcessHostImpl::HasConnection() const {
-  return channel_.get() != NULL;
+  return is_initialized_ && !is_dead_;
 }
 
 void RenderProcessHostImpl::SetIgnoreInputEvents(bool ignore_input_events) {
   ignore_input_events_ = ignore_input_events;
 }
 
 bool RenderProcessHostImpl::IgnoreInputEvents() const {
   return ignore_input_events_;
 }
 
@@ skipping 41 matching lines @@
     UMA_HISTOGRAM_COUNTS("Render.Workers.MaxWorkerCountInRendererProcess",
                           max_worker_count_);
   }
 
   // We cannot clean up twice; if this fails, there is an issue with our
   // control flow.
   DCHECK(!deleting_soon_);
 
   DCHECK_EQ(0, pending_views_);
 
-  // If |channel_| is still valid, the process associated with this
-  // RenderProcessHost is still alive. Notify all observers that the process
-  // has exited cleanly, even though it will be destroyed a bit later.
-  // Observers shouldn't rely on this process anymore.
-  if (channel_.get()) {
+  // If the process associated with this RenderProcessHost is still alive,
+  // notify all observers that the process has exited cleanly, even though it
+  // will be destroyed a bit later. Observers shouldn't rely on this process
+  // anymore.
+  if (!is_dead_) {

[ncarter (slow), 2016/10/12 23:32:42]

I'm trying to reason about why we need both is_dead_ and is_initialized_. Do we
really need both? Generally, as far as RPHImpl is concerned, everything that
happens on the initial normal launch should really also happen on re-launch
after crash.

[Ken Rockot(use gerrit already), 2016/10/12 23:56:05]

We may not need both. HasConnection() could *probably* just return !is_dead_,
and otherwise is_initialized_ is only used to conditionally clear the WebRTC
logging callback (yikes).

I left it here for now because it was easier for me to believe that the new
logic is strictly equivalent to the old logic, and didn't require me to figure
out if the webrtc logging thing really mattered.

     FOR_EACH_OBSERVER(
         RenderProcessHostObserver, observers_,
         RenderProcessExited(this, base::TERMINATION_STATUS_NORMAL_TERMINATION,
                             0));
   }
   FOR_EACH_OBSERVER(RenderProcessHostObserver, observers_,
                     RenderProcessHostDestroyed(this));
   NotificationService::current()->Notify(
       NOTIFICATION_RENDERER_PROCESS_TERMINATED,
       Source<RenderProcessHost>(this), NotificationService::NoDetails());
@@ skipping 509 matching lines @@
       // FrameHostMsg_RenderProcessGone with status
       // TERMINATION_STATUS_STILL_RUNNING, since this will break WebContentsImpl
       // logic.
       status = base::TERMINATION_STATUS_PROCESS_CRASHED;
     }
   }
 
   RendererClosedDetails details(status, exit_code);
 
   child_process_launcher_.reset();
-#if USE_ATTACHMENT_BROKER
-  IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
-      channel_.get());
-#endif
-  channel_.reset();
-  queued_messages_ = MessageQueue{};
+  is_dead_ = true;
 
   // Clear all cached associated interface proxies as well, since these are
   // effectively bound to the lifetime of the Channel.
   remote_route_provider_.reset();
   RemoveUserData(kRendererInterfaceKeyName);
 
   UpdateProcessPriority();
   DCHECK(!is_process_backgrounded_);
 
-  // RenderProcessExited observers and RenderProcessGone handlers might
-  // navigate or perform other actions that require a connection. Ensure that
-  // there is one before calling them.
-  child_token_ = mojo::edk::GenerateRandomToken();
-  shell::Connector* connector =
-      BrowserContext::GetConnectorFor(browser_context_);
-  if (!connector)
-    connector = ServiceManagerConnection::GetForProcess()->GetConnector();
-  child_connection_.reset(new ChildConnection(
-      kRendererServiceName,
-      base::StringPrintf("%d_%d", id_, instance_id_++), child_token_, connector,
-      BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)));
-
   within_process_died_observer_ = true;
   NotificationService::current()->Notify(
       NOTIFICATION_RENDERER_PROCESS_CLOSED, Source<RenderProcessHost>(this),
       Details<RendererClosedDetails>(&details));
   FOR_EACH_OBSERVER(RenderProcessHostObserver, observers_,
                     RenderProcessExited(this, status, exit_code));
   within_process_died_observer_ = false;
 
   message_port_message_filter_ = NULL;
 
-  DCHECK(!channel_);
   RemoveUserData(kSessionStorageHolderKey);
 
   IDMap<IPC::Listener>::iterator iter(&listeners_);
   while (!iter.IsAtEnd()) {
     iter.GetCurrentValue()->OnMessageReceived(FrameHostMsg_RenderProcessGone(
         iter.GetCurrentKey(), static_cast<int>(status), exit_code));
     iter.Advance();
   }
 
+  // Initialize a new ChannelProxy in case this host is re-used for a new
+  // process. This ensures that new messages can be sent on the host ASAP (even
+  // before Init()) and they'll eventually reach the new process.
+  InitializeChannelProxy();
+
   // It's possible that one of the calls out to the observers might have caused
   // this object to be no longer needed.
   if (delayed_cleanup_needed_)
     Cleanup();
 
   // This object is not deleted at this point and might be reused later.
   // TODO(darin): clean this up
 }
 
 size_t RenderProcessHost::GetActiveViewCount() {
@@ skipping 290 matching lines @@
   LOG(ERROR) << "Terminating render process for bad Mojo message: " << error;
 
   // The ReceivedBadMessage call below will trigger a DumpWithoutCrashing. Alias
   // enough information here so that we can determine what the bad message was.
   base::debug::Alias(&error);
   bad_message::ReceivedBadMessage(render_process_id,
                                   bad_message::RPH_MOJO_PROCESS_ERROR);
 }
 
 }  // namespace content