GitLab

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}