Avoid unbounded queueing of IPC messages for crashed renderers.

If a renderer process crashes, then we retain the associated
RenderProcessHostImpl object (to retain the old render_process_host_id
that may have been stored elsewhere in the system).  Before this CL this
had an undesired interaction with the ability of RenderProcessHostImpl
to queue IPCs between the call to Init and the time when a renderer
process is actually spawned (disclaimer - in practice the queueing has a
slightly/undesirably longer timeline).

Before this CL, RenderProcessHostImpl::ProcessDied(...) would call
EnableSendQueue().  This meant that an unbounded number of IPC messages
may be accumulated in
ChannelAssociatedGroupController::outgoing_messages_ (until the crashed
process is restarted).

After this CL, RenderProcessHostImpl::ProcessDied(...) no longer calls
EnableSendQueue().
- Short-term: subsystems that require queueing of IPCs should
  call RPH::EnableSendQueue() themselves (e.g. like RenderViewHostImpl's
  constructor does).
- Long-term: we should discard IPCs until RenderProcessHostImpl::Init
  is called - queueing should only take place in the short time between
  calling RenderProcessHostImpl::Init and the time the renderer process
  is actually launched.

There is a small risk that this CL will introduce crashes similar to the
ones seen in https://crbug.com/658759.

Bug: 813045
Change-Id: I838bf7ea443633ba0f2314d2de5c83891ec9ac5f
Reviewed-on: https://chromium-review.googlesource.com/1073621Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562491}

content/browser/renderer_host/render_process_host_browsertest.cc

@@ -31,6 +31,7 @@
 #include "content/shell/browser/shell_browser_main_parts.h"
 #include "content/shell/browser/shell_content_browser_client.h"
 #include "content/test/test_content_browser_client.h"
+#include "ipc/ipc_mojo_bootstrap.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/media_switches.h"
 #include "media/base/test_data_util.h"
@@ -1051,5 +1052,62 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, FastShutdownForStartingProcess) {
   EXPECT_TRUE(process->FastShutdownIfPossible());
 }
 
+// Regression test for one part of https://crbug.com/813045.
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
+                       NoUnboundedQueueingOfIpcs_CrashedProcess) {
+  // Navigate to a random page (this guarantees that a renderer had to be
+  // spawned to render the page).
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL main_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+
+  // Kill the renderer process.
+  RenderProcessHost* process =
+      shell()->web_contents()->GetMainFrame()->GetProcess();
+  RenderProcessHostWatcher crash_observer(
+      process, RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  EXPECT_TRUE(process->Shutdown(0));
+  crash_observer.Wait();
+
+  // Repeatedly send an IPC to the killed renderer process.
+  //
+  // This approximates a scenario where (over a long period of time) Chromium
+  // tries send an IPC to all renderers about environment changes (for example,
+  // see calls to RendererInterface::OnNetworkQualityChanged mentioned in
+  // https://crbug.com/813045#c17).
+  constexpr size_t kNumberOfTestIterations =
+      IPC::MojoBootstrap::kMaxOutgoingMessagesSizeForTesting + 2;
+  for (size_t i = 0; i < kNumberOfTestIterations; i++) {
+    // Verify that the process is still dead.
+    EXPECT_FALSE(process->HasConnection());
+    EXPECT_FALSE(process->GetChannel());
+    EXPECT_FALSE(process->GetProcess().IsValid());
+    EXPECT_EQ(base::kNullProcessHandle, process->GetProcess().Handle());
+
+    // Attempt to send an IPC message to the dead process.
+    std::string user_agent = base::StringPrintf("to-be-discarded-%zu", i);
+    if (process->GetRendererInterface())
+      process->GetRendererInterface()->SetUserAgent(user_agent);
+  }
+
+  // The main test verification is that the loop above didn't hit the CHECK in
+  // ipc/ipc_mojo_bootstrap.cc:
+  //
+  //      CHECK_LE(outgoing_messages_.size(),
+  //               MojoBootstrap::kMaxOutgoingMessagesSizeForTesting);
+  //
+  // No messages should be accumulated/queued for a crashed renderer process.
+  // Instead, such messages should be discarded.
+  //
+  // If the messages weren't discarded, then the SetUserAgent should hit another
+  // DCHECK in RenderThreadImpl::SetUserAgent (DCHECK(user_agent_.IsNull()))
+  // after reloading the page.
+  ReloadBlockUntilNavigationsComplete(shell(), 1);
+  EXPECT_EQ(process, shell()->web_contents()->GetMainFrame()->GetProcess());
+  EXPECT_EQ(process->GetID(),
+            shell()->web_contents()->GetMainFrame()->GetProcess()->GetID());
+  EXPECT_TRUE(process->HasConnection());
+}
+
 }  // namespace
 }  // namespace content

content/browser/renderer_host/render_process_host_impl.cc

@@ -3854,13 +3854,6 @@ void RenderProcessHostImpl::ProcessDied(
     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.
-  //
-  // 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.
   if (delayed_cleanup_needed_)

ipc/ipc_mojo_bootstrap.cc

@@ -640,11 +640,12 @@ class ChannelAssociatedGroupController
           base::AutoLock lock(outgoing_messages_lock_);
           outgoing_messages_.emplace_back(std::move(*message));
 
-          // TODO(https://crbug.com/813045): Remove this. Typically this queue
-          // won't exceed something like 50 messages even on slow devices. If
-          // the massive leaks we see can be attributed to this queue, it would
-          // have to be quite a bit larger.
-          CHECK_LE(outgoing_messages_.size(), 100000u);
+          // TODO(https://crbug.com/813045): Change this to a DCHECK. Typically
+          // this queue won't exceed something like 50 messages even on slow
+          // devices - higher numbers probably indicate that IPC messages are
+          // leaked.
+          CHECK_LE(outgoing_messages_.size(),
+                   MojoBootstrap::kMaxOutgoingMessagesSizeForTesting);
         }
         return true;
       }

ipc/ipc_mojo_bootstrap.h

@@ -56,6 +56,8 @@ class COMPONENT_EXPORT(IPC) MojoBootstrap {
   virtual void Flush() = 0;
 
   virtual mojo::AssociatedGroup* GetAssociatedGroup() = 0;
+
+  enum { kMaxOutgoingMessagesSizeForTesting = 100000u };
 };
 
 }  // namespace IPC