Fix RPHI message queueing during process death

Please see https://crbug.com/658759 for more context.

The TL;DR is I broke some subtle behavior in
https://crrev.com/425358, and this fixes it. Here's what happens
in ProcessDied, in this order:

  1. We reset channel_ so that future Send() calls throw away
     messages. Note that at this point, any Channel-associated
     interfaces remain valid but are intentionally dropping
     all messages as well.

  2. We fire various notifications about the death of the process.

  3. We re-initialize channel_, allowing subsequent Send() calls
     and associated interface calls to be queued and eventually
     delivered to the new process once it's restarted.

The subtlety here is that if a RenderViewHostImpl is created at any
point during step (2) above, we must re-initialize channel_ and
start allowing messages to be queued on it. Formerly this was
accomplished by EnableSendQueue() setting is_initialized_ to
false. This CL establishes the same behavior by restoring
EnableSendQueue() to essentially have the same effect by re-
initializing the channel earlier.

This is likely not the precise behavior we want, since it seems like
this can cause messages intended for the old process to end up getting
delivered to the new process, but it is the behavior we already had in
place before https://crrev.com/425358 and we apparently rely on it.

BUG=658759

Review-Url: https://codereview.chromium.org/2446543004
Cr-Commit-Position: refs/heads/master@{#427257}

content/browser/renderer_host/render_process_host_impl.cc

@@ -931,6 +931,11 @@ bool RenderProcessHostImpl::Init() {
   return true;
 }
 
+void RenderProcessHostImpl::EnableSendQueue() {
+  if (!channel_)
+    InitializeChannelProxy();
+}
+
 void RenderProcessHostImpl::InitializeChannelProxy() {
   // Generate a token used to identify the new child process.
   child_token_ = mojo::edk::GenerateRandomToken();
@@ -975,15 +980,7 @@ void RenderProcessHostImpl::InitializeChannelProxy() {
       IPC::ChannelMojo::CreateServerFactory(
           bootstrap.PassInterface().PassHandle(), io_task_runner);
 
-#if USE_ATTACHMENT_BROKER
-  if (channel_) {
-    IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
-        channel_.get());
-  }
-#endif
-
-  channel_.reset();
-  channel_connected_ = false;
+  ResetChannelProxy();
 
   // Do NOT expand ifdef or run time condition checks here! Synchronous
   // IPCs from browser process are banned. It is only narrowly allowed
@@ -1026,6 +1023,18 @@ void RenderProcessHostImpl::InitializeChannelProxy() {
   channel_->Pause();
 }
 
+void RenderProcessHostImpl::ResetChannelProxy() {
+  if (!channel_)
+    return;
+
+#if USE_ATTACHMENT_BROKER
+  IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
+      channel_.get());
+#endif
+  channel_.reset();
+  channel_connected_ = false;
+}
+
 void RenderProcessHostImpl::CreateMessageFilters() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   AddFilter(new ResourceSchedulerFilter(GetID()));
@@ -2171,17 +2180,12 @@ void RenderProcessHostImpl::Cleanup() {
   base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
   deleting_soon_ = true;
 
-#if USE_ATTACHMENT_BROKER
-  IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
-      channel_.get());
-#endif
-
   // It's important not to wait for the DeleteTask to delete the channel
   // proxy. Kill it off now. That way, in case the profile is going away, the
   // rest of the objects attached to this RenderProcessHost start going
   // away first, since deleting the channel proxy will post a
   // OnChannelClosed() to IPC::ChannelProxy::Context on the IO thread.
-  channel_.reset();
+  ResetChannelProxy();
 
   // The following members should be cleared in ProcessDied() as well!
   message_port_message_filter_ = NULL;
@@ -2668,11 +2672,7 @@ void RenderProcessHostImpl::ProcessDied(bool already_dead,
 
   child_process_launcher_.reset();
   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();
-  renderer_interface_.reset();
+  ResetChannelProxy();
 
   UpdateProcessPriority();
   DCHECK(!is_process_backgrounded_);
@@ -2699,7 +2699,9 @@ void RenderProcessHostImpl::ProcessDied(bool already_dead,
   // 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();
+  //
+  // Note that this may have already been called by one of the above observers
+  EnableSendQueue();
 
   // It's possible that one of the calls out to the observers might have caused
   // this object to be no longer needed.

content/browser/renderer_host/render_process_host_impl.h

@@ -120,6 +120,7 @@ class CONTENT_EXPORT RenderProcessHostImpl
 
   // RenderProcessHost implementation (public portion).
   bool Init() override;
+  void EnableSendQueue() override;
   int GetNextRoutingID() override;
   void AddRoute(int32_t routing_id, IPC::Listener* listener) override;
   void RemoveRoute(int32_t routing_id) override;
@@ -320,6 +321,10 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // to the next child process launched for this host, if any.
   void InitializeChannelProxy();
 
+  // Resets |channel_|, removing it from the attachment broker if necessary.
+  // Always call this in lieu of directly resetting |channel_|.
+  void ResetChannelProxy();
+
   // Creates and adds the IO thread message filters.
   void CreateMessageFilters();
 

content/browser/renderer_host/render_view_host_impl.cc

@@ -272,6 +272,12 @@ RenderViewHostImpl::RenderViewHostImpl(
 
   GetProcess()->AddObserver(this);
 
+  // New views may be created during RenderProcessHost::ProcessDied(), within a
+  // brief window where the internal ChannelProxy is null. This ensures that the
+  // ChannelProxy is re-initialized in such cases so that subsequent messages
+  // make their way to the new renderer once its restarted.
+  GetProcess()->EnableSendQueue();
+
   if (ResourceDispatcherHostImpl::Get()) {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,

content/public/browser/render_process_host.h

@@ -85,6 +85,12 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,
   // the process has been created, it should just call Init().
   virtual bool Init() = 0;
 
+  // Ensures that a Channel exists and is at least queueing outgoing messages
+  // if there isn't a render process connected to it yet. This may be used to
+  // ensure that in the event of a renderer crash and restart, subsequent
+  // messages sent via Send() will eventually reach the new process.
+  virtual void EnableSendQueue() = 0;
+
   // Gets the next available routing id.
   virtual int GetNextRoutingID() = 0;
 

content/public/test/mock_render_process_host.cc

@@ -101,6 +101,8 @@ bool MockRenderProcessHost::Init() {
   return true;
 }
 
+void MockRenderProcessHost::EnableSendQueue() {}
+
 int MockRenderProcessHost::GetNextRoutingID() {
   return ++prev_routing_id_;
 }

content/public/test/mock_render_process_host.h

@@ -45,6 +45,7 @@ class MockRenderProcessHost : public RenderProcessHost {
 
   // RenderProcessHost implementation (public portion).
   bool Init() override;
+  void EnableSendQueue() override;
   int GetNextRoutingID() override;
   void AddRoute(int32_t routing_id, IPC::Listener* listener) override;
   void RemoveRoute(int32_t routing_id) override;