Don't use nested message loop during EndSession

Previously during EndSession, a nested message loop and a task posted to
the FILE thread that posted a MessageLoop::Quit were used to communicate
that the FILE thread had completed outstanding writes. However, during
shutdown the GPU process might be terminated before the browser has run
through this code. In that case, the GPU process host might process the
"lost context" and try to re-establish (synchronously) a connection with
the GPU process. In this case, Windows can deny the launch of the gpu
process because the window station is terminating. This results in a
browser hang waiting for the GPU, which turns into a crash when the
watchdog timer kills the browser.

To avoid all this, use the same mechanism as Linux -- create a waitable
event and simply have the FILE thread signal it (meaning it's completed
previous writes). By not re-entering a UI-thread message loop, we avoid
having the GPU process host messages get processed.

As a side-benefit, this also means that GPU paint messages are not
processed, which (I think) will avoid the ugliness of sometimes having
sad tabs rendered during shutdown. This could happen because the
renderers are also asynchronously killed during shutdown and so it's a
race between painting the next frame and the browser window being
closed.

R=piman@chromium.org, cpu@chromium.org, darin@chromium.org
BUG=318527,47908,142501

Review URL: https://codereview.chromium.org/170023002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251922 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 18 insertions, 38 deletions

diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc
index 681515c..006099a 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -128,18 +128,11 @@
 static const int kUpdateCheckIntervalHours = 6;
 #endif
 
-#if defined(OS_WIN)
-// Attest to the fact that the call to the file thread to save preferences has
-// run, and it is safe to terminate.  This avoids the potential of some other
-// task prematurely terminating our waiting message loop by posting a
-// QuitTask().
-static bool g_end_session_file_thread_has_completed = false;
-#endif
-
-#if defined(USE_X11)
-// How long to wait for the File thread to complete during EndSession, on
-// Linux. We have a timeout here because we're unable to run the UI messageloop
-// and there's some deadlock risk. Our only option is to exit anyway.
+#if defined(USE_X11) || defined(OS_WIN)
+// How long to wait for the File thread to complete during EndSession, on Linux
+// and Windows. We have a timeout here because we're unable to run the UI
+// messageloop and there's some deadlock risk. Our only option is to exit
+// anyway.
 static const int kEndSessionTimeoutSeconds = 10;
 #endif
 
@@ -312,16 +305,7 @@ void BrowserProcessImpl::PostDestroyThreads() {
   io_thread_.reset();
 }
 
-#if defined(OS_WIN)
-// Send a QuitTask to the given MessageLoop when the (file) thread has processed
-// our (other) recent requests (to save preferences).
-// Change the boolean so that the receiving thread will know that we did indeed
-// send the QuitTask that terminated the message loop.
-static void PostQuit(base::MessageLoop* message_loop) {
-  g_end_session_file_thread_has_completed = true;
-  message_loop->PostTask(FROM_HERE, base::MessageLoop::QuitClosure());
-}
-#elif defined(USE_X11)
+#if defined(USE_X11) || defined(OS_WIN)
 static void Signal(base::WaitableEvent* event) {
   event->Signal();
 }
@@ -403,8 +387,18 @@ void BrowserProcessImpl::EndSession() {
   // We must write that the profile and metrics service shutdown cleanly,
   // otherwise on startup we'll think we crashed. So we block until done and
   // then proceed with normal shutdown.
-#if defined(USE_X11)
-  //  Can't run a local loop on linux. Instead create a waitable event.
+#if defined(USE_X11) || defined(OS_WIN)
+  // Create a waitable event to block on file writing being complete.
+  //
+  // On Windows, we previously posted a message to FILE and then ran a nested
+  // message loop, waiting for that message to be processed until quitting.
+  // However, doing so means that other messages will also be processed. In
+  // particular, if the GPU process host notices that the GPU has been killed
+  // during shutdown, it races exiting the nested loop with the process host
+  // blocking the message loop attempting to re-establish a connection to the
+  // GPU process synchronously. Because the system may not be allowing
+  // processes to launch, this can result in a hang. See
+  // http://crbug.com/318527.
   scoped_ptr<base::WaitableEvent> done_writing(
       new base::WaitableEvent(false, false));
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
@@ -415,20 +409,6 @@ void BrowserProcessImpl::EndSession() {
       base::TimeDelta::FromSeconds(kEndSessionTimeoutSeconds))) {
     ignore_result(done_writing.release());
   }
-
-#elif defined(OS_WIN)
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(PostQuit, base::MessageLoop::current()));
-  int quits_received = 0;
-  do {
-    base::MessageLoop::current()->Run();
-    ++quits_received;
-  } while (!g_end_session_file_thread_has_completed);
-  // If we did get extra quits, then we should re-post them to the message loop.
-  while (--quits_received > 0) {
-    base::MessageLoop::current()->PostTask(FROM_HERE,
-                                           base::MessageLoop::QuitClosure());
-  }
 #else
   NOTIMPLEMENTED();
 #endif