Remove RenderProcessWillExit to fix duplicate routing ID crashes.

RenderProcessWillExit is dispatched to observers from
RPHI::ShutdownRequest during renderer-initiated process shutdown, when
the renderer process is allowed to continue shutting down but before
it does actually shut down.  This is routed through
SiteInstanceImpl::RenderProcessWillExit, which calls
RenderFrameHostManager::RenderProcessGone() to mark all proxies living
in that process as non-live.  Unfortunately, this might lead to
duplicate routing ID crashes if one of these proxies is reused and
recreated before the process goes away completely.  See
https://crbug.com/794625#c14 for full details.

This CL fixes these races by completely removing
RenderProcessWillExit.  The affected RenderFrameProxyHosts will still
be marked as non-live later when the process actually goes away, as
part of SiteInstanceImpl::RenderProcessExited(), called from either
RPHI::Cleanup() or RPHI::ProcessDied().  That is, today we try to mark
proxies as non-live twice, and arguably only the second path is really
needed.  RenderProcessWillExit also used to trigger
RenderFrameHostImpl::RenderProcessGone() to perform some navigation
cleanup work, but the same cleanup will still happen via the
RenderProcessExited path.

Bug: 794625, 575400
Change-Id: I6b6c776e179a1048c9634ad323d45302423246f1
Reviewed-on: https://chromium-review.googlesource.com/1155790Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580069}

content/browser/frame_host/render_frame_host_impl.h

@@ -836,6 +836,8 @@ class CONTENT_EXPORT RenderFrameHostImpl
                            ActiveSandboxFlagsRetainedAfterSwapOut);
   FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest,
                            LastCommittedURLRetainedAfterSwapOut);
+  FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest,
+                           RenderFrameProxyNotRecreatedDuringProcessShutdown);
   FRIEND_TEST_ALL_PREFIXES(SecurityExploitBrowserTest,
                            AttemptDuplicateRenderViewHost);
 

content/browser/renderer_host/render_process_host_impl.cc

@@ -701,12 +701,6 @@ class SpareRenderProcessHostManager : public RenderProcessHostObserver {
     }
   }
 
-  // RenderProcessHostObserver::RenderProcessWillExit is not overriden because:
-  // 1. This simplifies reasoning when Cleanup can be called.
-  // 2. In practice the spare shouldn't go through graceful shutdown.
-  // 3. Handling RenderProcessExited and RenderProcessHostDestroyed is
-  //    sufficient from correctness perspective.
-
   void RenderProcessExited(RenderProcessHost* host,
                            const ChildProcessTerminationInfo& info) override {
     if (host == spare_render_process_host_)
@@ -4042,11 +4036,6 @@ void RenderProcessHostImpl::ShutdownRequest() {
     return;
   }
 
-  // Notify any contents that might have swapped out renderers from this
-  // process. They should not attempt to swap them back in.
-  for (auto& observer : observers_) {
-    observer.RenderProcessWillExit(this);
-  }
   child_control_interface_->ProcessShutdown();
 }
 

content/browser/site_instance_impl.cc

@@ -569,13 +569,6 @@ void SiteInstanceImpl::RenderProcessHostDestroyed(RenderProcessHost* host) {
   process_ = nullptr;
 }
 
-void SiteInstanceImpl::RenderProcessWillExit(RenderProcessHost* host) {
-  // TODO(nick): http://crbug.com/575400 - RenderProcessWillExit might not serve
-  // any purpose here.
-  for (auto& observer : observers_)
-    observer.RenderProcessGone(this);
-}
-
 void SiteInstanceImpl::RenderProcessExited(
     RenderProcessHost* host,
     const ChildProcessTerminationInfo& info) {

content/browser/site_instance_impl.h

@@ -229,7 +229,6 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
 
   // RenderProcessHostObserver implementation.
   void RenderProcessHostDestroyed(RenderProcessHost* host) override;
-  void RenderProcessWillExit(RenderProcessHost* host) override;
   void RenderProcessExited(RenderProcessHost* host,
                            const ChildProcessTerminationInfo& info) override;
 

content/browser/site_per_process_browsertest.cc

@@ -7446,8 +7446,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   EXPECT_FALSE(rvh->is_swapped_out_);
 }
 
-// Helper class to wait for a ShutdownRequest message to arrive, in response to
-// which RenderProcessWillExit is called on observers by RenderProcessHost.
+// Helper class to wait for a ShutdownRequest message to arrive.
 class ShutdownObserver : public RenderProcessHostObserver {
  public:
   ShutdownObserver() : message_loop_runner_(new MessageLoopRunner) {}
@@ -12923,4 +12922,81 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
       popup_subframe->current_frame_host()->GetSiteInstance()->GetSiteURL());
 }
 
+// Ensure that when a process is about to be destroyed after the last active
+// frame in it goes away, an attempt to reuse a proxy in that process doesn't
+// result in a crash.  See https://crbug.com/794625.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       RenderFrameProxyNotRecreatedDuringProcessShutdown) {
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+
+  GURL popup_url(embedded_test_server()->GetURL(
+      "b.com", "/title1.html"));
+  Shell* new_shell = OpenPopup(root, popup_url, "foo");
+  FrameTreeNode* popup_root =
+      static_cast<WebContentsImpl*>(new_shell->web_contents())
+          ->GetFrameTree()
+          ->root();
+  auto* rfh = popup_root->current_frame_host();
+
+  // Disable the swapout timer to prevent flakiness.
+  rfh->DisableSwapOutTimerForTesting();
+
+  // This will be used to monitor that b.com process exits cleanly.
+  RenderProcessHostWatcher b_process_observer(
+      popup_root->current_frame_host()->GetProcess(),
+      RenderProcessHostWatcher::WATCH_FOR_HOST_DESTRUCTION);
+
+  // In the first tab, install a postMessage handler to navigate the popup to a
+  // hung b.com URL once the first message is received.
+  GURL hung_b_url(embedded_test_server()->GetURL("b.com", "/hung"));
+  TestNavigationManager manager(new_shell->web_contents(), hung_b_url);
+  EXPECT_TRUE(ExecuteScript(shell(), base::StringPrintf(R"(
+      window.done = false;
+      window.onmessage = () => {
+        if (!window.done) {
+          window.open('%s', 'foo');
+          window.done = true;
+        }
+      };)", hung_b_url.spec().c_str())));
+
+  // In the popup, install an unload handler to send a lot of postMessages to
+  // the opener.  This keeps the MessageLoop in the b.com process busy after
+  // navigating away from the current document.  In https://crbug.com/794625,
+  // this was needed so that a subsequent IPC to recreate a proxy arrives
+  // before the process fully shuts down.
+  EXPECT_TRUE(ExecuteScript(new_shell, R"(
+      window.onunload = () => {
+        for (var i=0; i<10000; i++)
+          opener.postMessage('hi','*');
+      })"));
+
+  // Navigate popup to a.com.  This swaps out the last active frame in the
+  // b.com process, and hence initiates process shutdown.
+  TestFrameNavigationObserver commit_observer(popup_root);
+  GURL another_a_url(embedded_test_server()->GetURL("a.com", "/title3.html"));
+  EXPECT_TRUE(ExecuteScript(
+      new_shell,
+      base::StringPrintf("location = '%s';", another_a_url.spec().c_str())));
+  commit_observer.WaitForCommit();
+
+  // At this point, popup's original RFH is pending deletion.
+  EXPECT_FALSE(rfh->is_active());
+
+  // When the opener receives a postMessage from the popup's unload handler, it
+  // should start a navigation back to b.com.  Wait for it.  This navigation
+  // creates a speculative RFH which reuses the proxy that was created as part
+  // of swapping out from |popup_url| to |another_a_url|.
+  EXPECT_TRUE(manager.WaitForRequestStart());
+
+  // Cancel the started navigation (to /hung) in the popup and make sure the
+  // b.com renderer process exits cleanly without a crash.  In
+  // https://crbug.com/794625, the crash was caused by trying to recreate the
+  // reused proxy, which had been incorrectly set as non-live.
+  popup_root->ResetNavigationRequest(false, false);
+  b_process_observer.Wait();
+  EXPECT_TRUE(b_process_observer.did_exit_normally());
+}
+
 }  // namespace content

content/public/browser/render_process_host_observer.h

@@ -27,13 +27,6 @@ class CONTENT_EXPORT RenderProcessHostObserver {
   // but may or may not be allowed.
   virtual void RenderProcessShutdownRequested(RenderProcessHost* host) {}
 
-  // This method is invoked when the process is going to exit and should not be
-  // used for further navigations. Note that this is a COURTESY callback, not
-  // guaranteed to be called for any particular process. Because this is the
-  // first step in an orderly shutdown of a render process, do not expect that
-  // a new render process will be hosted with this RenderProcessHost.
-  virtual void RenderProcessWillExit(RenderProcessHost* host) {}
-
   // This method is invoked when the process of the observed RenderProcessHost
   // exits (either normally or with a crash). To determine if the process closed
   // normally or crashed, examine the |status| parameter.