Make sure the spare renderer gets properly cleaned up during shutdown.

Ensuring proper cleanup of the spare during browser shutdown
============================================================

To properly cleanup the spare renderer we need to:

1. Make sure it is subject to the fast shutdown path (even if it is
   still starting).  This way the spare will be subject to the
   fast shutdown initiated from chrome/browser/browser_shutdown.cc:

      void OnShutdownStarting(ShutdownType type) {
        ...
        // Call FastShutdown on all of the RenderProcessHosts.  This will be
        // a no-op in some cases, so we still need to go through the normal
        // shutdown path for the ones that didn't exit here.
        g_shutdown_num_processes = 0;
        g_shutdown_num_processes_slow = 0;
        for (content::RenderProcessHost::iterator i(
                content::RenderProcessHost::AllHostsIterator());
             !i.IsAtEnd(); i.Advance()) {
          ++g_shutdown_num_processes;
          if (!i.GetCurrentValue()->FastShutdownIfPossible())
            ++g_shutdown_num_processes_slow;
        }
        ...

2. Make sure the SpareRenderProcessHostManager calls CleanUp on the
   spare for which RenderProcessHostObserver::RenderProcessExited is
   called.


Enabling fast shutdown for currently-starting renderers
=======================================================

This CL removes the code that avoids fast shutdown for renderers that
are currently starting.  This code comes from
https://codereview.chromium.org/397031/patch/2029/2041 by jam@.

I feel confident that fast shutdown will be handled correctly for
renderers that are currently starting.  During the fast shutdown
RenderProcessHostImpl::ProcessDied will be called and reset
|child_process_launcher_| - this will correctly terminate the process
regardless of whether the process 1) has already been started or 2) is
in currently starting:

1. If the process has already been launched, then the destructor of
   ChildProcessLauncher will ensure process termination by eventually
   calling ForceNormalProcessTerminationSync.  This platform-specific
   method will kill the process.

2. If the process is being started, then ChildProcessLauncherHelper will
   eventually (once the process really starts) get the
   PostLaunchOnClientThread callback.  In the PostLaunchOnClientThread
   method we will check if the weak pointer to |child_process_launcher_|
   is still valid and if not, we will also terminate the process via
   ForceNormalProcessTerminationAsync.

   I realize that the above explanation depends on
   PostLaunchOnClientThread method being actually called.  I understand
   that this call might not happen if the browser process crashes.  The
   problem of ensuring child process termination in this case seems like
   an orthogonal, separate problem.

Supporting fast shutdown for currently-starting renderers means that
RenderProcessHostObserver::RenderProcessExited may be called earlier
(it is called by ProcessDied which is used on the fast shutdown path).
I think that such an earlier call is okay - I've tweaked the comments
for RenderProcessHostObserver::RenderProcessReady accordingly (removing
the mention of RenderProcessExited callback).


NOT enabling fast shutdown for not-yet-started or crashed renderers
===================================================================

This CL retains the behavior of NOT enabling fast shutdown for
not-yet-started or crashed renderers.  Such change would break tests
like ChromeRenderProcessHostTest.CloseAllTabsDuringProcessDied
on mac_chromium_rel_ng:

    [render_process_host_impl.cc(3743)]
    Check failed: !within_process_died_observer_.
        base::debug::StackTrace::StackTrace(...)
        logging::LogMessage::~LogMessage()
        content::RenderProcessHostImpl::ProcessDied(...)
        content::RenderProcessHostImpl::FastShutdownIfPossible(...)
        CloseWebContentses(...)
        TabStripModel::InternalCloseTabs(...)
        TabStripModel::CloseAllTabs()
        WindowDestroyer::RenderProcessGone(...)
        content::WebContentsImpl::RenderViewTerminated(...)
        content::RenderProcessHostImpl::ProcessDied(...)
        non-virtual thunk to content::RenderProcessHostImpl::OnChannelError()
        ...

Additionally, supporting fast shutdown for previously-crashed renderers
would also introduce a risk that RenderProcessExited might be called
twice - once when the process crashes and a second time when the process
is subject to the fast shutdown path.


Proper clean-up of a killed spare RenderProcessHost
===================================================

SpareRenderProcessHostManager holds the only reference to a spare
RenderProcessHost* (which otherwise has no IPC routes and a zero
ref-count).  Before this CL, SpareRenderProcessHostManager would
respond to RenderProcessHostObserver::RenderProcessExited call by
dropping the only reference to the spare - this would result in a leak
of the RenderProcessHost object.  After this CL,
RenderProcessHost::Cleanup is called, ensuring proper destruction of the
object in case of a crash or in case of a fast shutdown.


Consistent termination status during fast shutdown
==================================================

To ensure consistent termination status during fast shutdown
(i.e.  TERMINATION_STATUS_STILL_RUNNING) we need to tweak
ChildProcessLauncher::GetChildTerminationStatus to account for processes
that have not yet started.  Without this change Windows would see the
default termination status set in the constructor of
ChildProcessLauncher which would lead to a change in prerender final
status seen in the following callstack:

        prerender::PrerenderContents::SetFinalStatus
        prerender::PrerenderContents::Destroy
        prerender::PrerenderContents::RenderProcessGone
        content::WebContentsImpl::RenderViewTerminated
        content::RenderProcessHostImpl::ProcessDied
        content::RenderProcessHostImpl::FastShutdownIfPossible
        browser_shutdown::OnShutdownStarting
        Browser::OnWindowClosing
        BrowserView::CanClose
        views::Widget::Close
        BrowserCloseManager::CloseBrowsers
        BrowserCloseManager::TryToCloseBrowsers
        chrome::CloseAllBrowsers
        chrome::AttemptExitInternal

and consequently would lead to failures of the following browser tests
on Windows:

    PrerenderBrowserTest.PrerenderQuickQuit
    PrerenderBrowserTest.PrerenderInfiniteLoop


Bug: 808114
Change-Id: I0e1cb47eac0b59866d87b5b3c76aae95e6223760
Reviewed-on: https://chromium-review.googlesource.com/994290Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549278}

chrome/browser/fast_shutdown_browsertest.cc

@@ -19,6 +19,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/test/browser_test_utils.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
@@ -74,3 +75,20 @@ IN_PROC_BROWSER_TEST_F(FastShutdown, DISABLED_SlowTermination) {
   EXPECT_EQ("unloaded=ohyeah", content::GetCookies(browser()->profile(), url));
 }
 #endif
+
+// Verifies that the spare renderer maintained by SpareRenderProcessHostManager
+// is correctly destroyed during browser shutdown.
+//
+// Prior to the CL that introduced the test below, there were some problems
+// encountered during the shutdown sequence specific to the //chrome layer.
+// Therefore, it is important that the test below is a //chrome-level test, even
+// though the test doesn't have any explicit dependencies on the //chrome layer.
+IN_PROC_BROWSER_TEST_F(FastShutdown, SpareRenderProcessHostDuringShutdown) {
+  content::RenderProcessHost::WarmupSpareRenderProcessHost(
+      browser()->profile());
+
+  // The verification is that there are no DCHECKs anywhere during test tear
+  // down (in particular that no DCHECKs are hit inside
+  // ProfileDestroyer::DestroyProfileWhenAppropriate when it tries to make sure
+  // that no renderers associated with the given Profile are still alive).
+}

content/browser/child_process_launcher.cc

@@ -103,8 +103,14 @@ base::TerminationStatus ChildProcessLauncher::GetChildTerminationStatus(
     bool known_dead,
     int* exit_code) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!process_.process.IsValid()) {
-    // Process is already gone, so return the cached termination status.
+    // Make sure to avoid using the default termination status if the process
+    // hasn't even started yet.
+    if (IsStarting())
+      termination_status_ = base::TERMINATION_STATUS_STILL_RUNNING;
+
+    // Process doesn't exist, so return the cached termination status.
     if (exit_code)
       *exit_code = exit_code_;
     return termination_status_;

content/browser/renderer_host/render_process_host_browsertest.cc

@@ -345,6 +345,18 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, SpareRendererOnProcessReuse) {
   SetBrowserClientForTesting(old_client);
 }
 
+// Verifies that the spare renderer maintained by SpareRenderProcessHostManager
+// is correctly destroyed during browser shutdown.  This test is an analogue
+// to the //chrome-layer FastShutdown.SpareRenderProcessHost test.
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
+                       SpareRenderProcessHostDuringShutdown) {
+  content::RenderProcessHost::WarmupSpareRenderProcessHost(
+      shell()->web_contents()->GetBrowserContext());
+
+  // The verification is that there are no DCHECKs anywhere during test tear
+  // down.
+}
+
 class ShellCloser : public RenderProcessHostObserver {
  public:
   ShellCloser(Shell* shell, std::string* logging_string)
@@ -812,5 +824,14 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
     rph->RemoveObserver(this);
 }
 
+// This test verifies that a fast shutdown is possible for a starting process.
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, FastShutdownForStartingProcess) {
+  RenderProcessHost* process = RenderProcessHostImpl::CreateRenderProcessHost(
+      ShellContentBrowserClient::Get()->browser_context(), nullptr, nullptr,
+      false /* is_for_guests_only */);
+  process->Init();
+  EXPECT_TRUE(process->FastShutdownIfPossible());
+}
+
 }  // namespace
 }  // namespace content

content/browser/renderer_host/render_process_host_impl.cc

@@ -643,8 +643,16 @@ class SpareRenderProcessHostManager : public RenderProcessHostObserver {
   // Gracefully remove and cleanup a spare RenderProcessHost if it exists.
   void CleanupSpareRenderProcessHost() {
     if (spare_render_process_host_) {
+      // Stop observing the process, to avoid getting notifications as a
+      // consequence of the Cleanup call below - such notification could call
+      // back into CleanupSpareRenderProcessHost leading to stack overflow.
+      spare_render_process_host_->RemoveObserver(this);
+
+      // Make sure the RenderProcessHost object gets destroyed.
       spare_render_process_host_->Cleanup();
-      DropSpareRenderProcessHost(spare_render_process_host_);
+
+      // Drop reference to the RenderProcessHost object.
+      spare_render_process_host_ = nullptr;
     }
   }
 
@@ -653,15 +661,17 @@ class SpareRenderProcessHostManager : public RenderProcessHostObserver {
   }
 
  private:
-  // RenderProcessHostObserver
-  void RenderProcessWillExit(RenderProcessHost* host) override {
-    DropSpareRenderProcessHost(host);
-  }
+  // 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,
-                           base::TerminationStatus unused_status,
+                           base::TerminationStatus status,
                            int unused_exit_code) override {
-    DropSpareRenderProcessHost(host);
+    if (host == spare_render_process_host_)
+      CleanupSpareRenderProcessHost();
   }
 
   void RenderProcessHostDestroyed(RenderProcessHost* host) override {
@@ -2907,8 +2917,7 @@ bool RenderProcessHostImpl::FastShutdownIfPossible(size_t page_count,
   if (run_renderer_in_process())
     return false;  // Single process mode never shuts down the renderer.
 
-  if (!child_process_launcher_.get() || child_process_launcher_->IsStarting() ||
-      !GetHandle())
+  if (!child_process_launcher_.get())
     return false;  // Render process hasn't started or is probably crashed.
 
   // Test if there's an unload listener.

content/public/browser/render_process_host_observer.h

@@ -19,7 +19,7 @@ class CONTENT_EXPORT RenderProcessHostObserver {
  public:
   // This method is invoked when the process was launched and the channel was
   // connected. This is the earliest time it is safe to call Shutdown on the
-  // RenderProcessHost and get RenderProcessExited notifications.
+  // RenderProcessHost.
   virtual void RenderProcessReady(RenderProcessHost* host) {}
 
   // This method is invoked when the process when the process could shut down