This is a second attempt of http://codereview.chromium.org/3175038

It failed the Vista Perf UI tests. This is because those tests close the browser upon an error. And they always get an error when the session is closed in the middle of the test.

The new changes are in chrome/browser/automation/testing_automation_provider.cc

BUG=50006
TEST=Run chrome under nested window manager using Xephyr (see
http://code.google.com/p/chromium/wiki/LayoutTestsLinux)

use --enable-logging=stderr --log-level=0
kill xephyr
examine log. You should see
X IO Error detected
followed (not necessarily immediately) by
successfully saved /tmp/tx/Default/Preferences
successfully saved /tmp/tx/Local State
successfully saved /tmp/tx/Local State
successfully saved /tmp/tx/Default/Preferences
along with no crash.

BUG=
TEST=

Review URL: http://codereview.chromium.org/3364019

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

13 files changed, 133 insertions, 22 deletions

diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index 60d4620..72a0b75 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -246,7 +246,7 @@ void RecordLastRunAppBundlePath() {
   // Initiate a shutdown (via BrowserList::CloseAllBrowsers()) if we aren't
   // already shutting down.
   if (!browser_shutdown::IsTryingToQuit())
-    BrowserList::CloseAllBrowsers(true);
+    BrowserList::CloseAllBrowsers();
 
   return num_browsers == 0 ? YES : NO;
 }
diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc
index a2264e9..998f322 100644
--- a/chrome/browser/automation/testing_automation_provider.cc
+++ b/chrome/browser/automation/testing_automation_provider.cc
@@ -29,6 +29,7 @@
 #include "chrome/browser/bookmarks/bookmark_storage.h"
 #include "chrome/browser/blocked_popup_container.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/browser_window.h"
 #include "chrome/browser/debugger/devtools_manager.h"
 #include "chrome/browser/download/download_prefs.h"
@@ -419,7 +420,8 @@ void TestingAutomationProvider::OnMessageReceived(
 }
 
 void TestingAutomationProvider::OnChannelError() {
-  BrowserList::CloseAllBrowsersAndExit();
+  if (browser_shutdown::GetShutdownType() == browser_shutdown::NOT_VALID)
+    BrowserList::CloseAllBrowsersAndExit();
   AutomationProvider::OnChannelError();
 }
 
diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index 0ce399d..31e09cc 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -223,7 +223,15 @@ void BrowserList::RemoveObserver(BrowserList::Observer* observer) {
 }
 
 // static
-void BrowserList::CloseAllBrowsers(bool use_post) {
+void BrowserList::CloseAllBrowsers() {
+  bool session_ending =
+      browser_shutdown::GetShutdownType() == browser_shutdown::END_SESSION;
+  bool use_post = !session_ending;
+  bool force_exit = false;
+#if defined(USE_X11)
+  if (session_ending)
+    force_exit = true;
+#endif
   // Tell everyone that we are shutting down.
   browser_shutdown::SetTryingToQuit(true);
 
@@ -233,7 +241,7 @@ void BrowserList::CloseAllBrowsers(bool use_post) {
 
   // If there are no browsers, send the APP_TERMINATING action here. Otherwise,
   // it will be sent by RemoveBrowser() when the last browser has closed.
-  if (browsers_.empty()) {
+  if (force_exit || browsers_.empty()) {
     NotificationService::current()->Notify(NotificationType::APP_TERMINATING,
                                            NotificationService::AllSources(),
                                            NotificationService::NoDetails());
@@ -276,10 +284,10 @@ void BrowserList::CloseAllBrowsersAndExit() {
 
 #if !defined(OS_MACOSX)
   // On most platforms, closing all windows causes the application to exit.
-  CloseAllBrowsers(true);
+  CloseAllBrowsers();
 #else
   // On the Mac, the application continues to run once all windows are closed.
-  // Terminate will result in a CloseAllBrowsers(true) call, and once (and if)
+  // Terminate will result in a CloseAllBrowsers() call, and once (and if)
   // that is done, will cause the application to exit cleanly.
   chrome_browser_application_mac::Terminate();
 #endif
@@ -303,8 +311,7 @@ void BrowserList::WindowsSessionEnding() {
   // Write important data first.
   g_browser_process->EndSession();
 
-  // Close all the browsers.
-  BrowserList::CloseAllBrowsers(false);
+  BrowserList::CloseAllBrowsers();
 
   // Send out notification. This is used during testing so that the test harness
   // can properly shutdown before we exit.
@@ -320,6 +327,8 @@ void BrowserList::WindowsSessionEnding() {
   // At this point the message loop is still running yet we've shut everything
   // down. If any messages are processed we'll likely crash. Exit now.
   ExitProcess(ResultCodes::NORMAL_EXIT);
+#elif defined(OS_LINUX)
+  _exit(ResultCodes::NORMAL_EXIT);
 #else
   NOTIMPLEMENTED();
 #endif
@@ -358,7 +367,7 @@ void BrowserList::EndKeepAlive() {
     // (MessageLoop::current() == null).
     if (browsers_.empty() && !browser_shutdown::IsTryingToQuit() &&
         MessageLoop::current())
-      CloseAllBrowsers(true);
+      CloseAllBrowsers();
   }
 }
 
diff --git a/chrome/browser/browser_list.h b/chrome/browser/browser_list.h
index 9e52aba..dcde7fb 100644
--- a/chrome/browser/browser_list.h
+++ b/chrome/browser/browser_list.h
@@ -96,18 +96,17 @@ class BrowserList {
   // 2. An update exe is present in the install folder.
   static bool CanRestartForUpdate();
 
-  // Closes all browsers. If use_post is true the windows are closed by way of
-  // posting a WM_CLOSE message, otherwise the windows are closed directly. In
-  // almost all cases you'll want to use true, the one exception is ending
-  // the session. use_post should only be false when invoked from end session.
-  static void CloseAllBrowsers(bool use_post);
-
   // Closes all browsers and exits.  This is equivalent to
   // CloseAllBrowsers(true) on platforms where the application exits when no
   // more windows are remaining.  On other platforms (the Mac), this will
   // additionally exit the application.
   static void CloseAllBrowsersAndExit();
 
+  // Closes all browsers. If the session is ending the windows are closed
+  // directly. Otherwise the windows are closed by way of posting a WM_CLOSE
+  // message.
+  static void CloseAllBrowsers();
+
   // Begins shutdown of the application when the Windows session is ending.
   static void WindowsSessionEnding();
 
diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc
index d3f6fe0..da28fb1 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -100,6 +100,7 @@
 #endif
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
+#include "chrome/browser/browser_main_gtk.h"
 #include "chrome/browser/gtk/gtk_util.h"
 #endif
 
@@ -1198,6 +1199,10 @@ int BrowserMain(const MainFunctionParams& parameters) {
   }
 #endif
 
+#if defined(USE_X11)
+  SetBrowserX11ErrorHandlers();
+#endif
+
   Profile* profile = CreateProfile(parameters, user_data_dir);
   if (!profile)
     return ResultCodes::NORMAL_EXIT;
diff --git a/chrome/browser/browser_main_gtk.cc b/chrome/browser/browser_main_gtk.cc
index 520c652..25147c8d 100644
--- a/chrome/browser/browser_main_gtk.cc
+++ b/chrome/browser/browser_main_gtk.cc
@@ -4,8 +4,12 @@
 
 #include "chrome/browser/browser_main.h"
 
+#include "app/x11_util.h"
+#include "app/x11_util_internal.h"
 #include "base/command_line.h"
 #include "base/debug_util.h"
+#include "chrome/browser/browser_list.h"
+#include "chrome/browser/browser_main_gtk.h"
 #include "chrome/browser/browser_main_win.h"
 #include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/common/result_codes.h"
@@ -14,6 +18,30 @@
 #include "chrome/app/breakpad_linux.h"
 #endif
 
+namespace {
+
+// Indicates that we're currently responding to an IO error (by shutting down).
+bool g_in_x11_io_error_handler = false;
+
+int BrowserX11ErrorHandler(Display* d, XErrorEvent* error) {
+  if (!g_in_x11_io_error_handler)
+    LOG(ERROR) << x11_util::GetErrorEventDescription(error);
+  return 0;
+}
+
+int BrowserX11IOErrorHandler(Display* d) {
+  // If there's an IO error it likely means the X server has gone away
+  if (!g_in_x11_io_error_handler) {
+    g_in_x11_io_error_handler = true;
+    LOG(ERROR) << "X IO Error detected";
+    BrowserList::WindowsSessionEnding();
+  }
+
+  return 0;
+}
+
+}  // namespace
+
 void DidEndMainMessageLoop() {
 }
 
@@ -46,3 +74,11 @@ bool CheckMachineLevelInstall() {
 
 void PrepareRestartOnCrashEnviroment(const CommandLine &parsed_command_line) {
 }
+
+void SetBrowserX11ErrorHandlers() {
+  // Set up error handlers to make sure profile gets written if X server
+  // goes away.
+  x11_util::SetX11ErrorHandlers(
+      BrowserX11ErrorHandler,
+      BrowserX11IOErrorHandler);
+}
diff --git a/chrome/browser/browser_main_gtk.h b/chrome/browser/browser_main_gtk.h
new file mode 100644
index 0000000..234ff7a
--- /dev/null
+++ b/chrome/browser/browser_main_gtk.h
@@ -0,0 +1,15 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Contains functions used by BrowserMain() that are gtk-specific.
+
+#ifndef CHROME_BROWSER_BROWSER_MAIN_GTK_H_
+#define CHROME_BROWSER_BROWSER_MAIN_GTK_H_
+#pragma once
+
+// Installs the X11 error handlers for the browser process. This will
+// allow us to exit cleanly if X exits before us.
+void SetBrowserX11ErrorHandlers();
+
+#endif  // CHROME_BROWSER_BROWSER_MAIN_GTK_H_
diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc
index 539bf57..682c491 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -11,6 +11,7 @@
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
+#include "base/task.h"
 #include "base/thread.h"
 #include "base/waitable_event.h"
 #include "chrome/browser/appcache/chrome_appcache_service.h"
@@ -74,6 +75,13 @@
 static const int kUpdateCheckIntervalHours = 6;
 #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.
+static const int kEndSessionTimeoutSeconds = 10;
+#endif
+
 BrowserProcessImpl::BrowserProcessImpl(const CommandLine& command_line)
     : created_resource_dispatcher_host_(false),
       created_metrics_service_(false),
@@ -215,10 +223,16 @@ BrowserProcessImpl::~BrowserProcessImpl() {
   g_browser_process = NULL;
 }
 
+#if defined(OS_WIN)
 // Send a QuitTask to the given MessageLoop.
 static void PostQuit(MessageLoop* message_loop) {
   message_loop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
 }
+#elif defined(USE_X11)
+static void Signal(base::WaitableEvent* event) {
+  event->Signal();
+}
+#endif
 
 unsigned int BrowserProcessImpl::AddRefModule() {
   DCHECK(CalledOnValidThread());
@@ -240,9 +254,9 @@ unsigned int BrowserProcessImpl::ReleaseModule() {
 }
 
 void BrowserProcessImpl::EndSession() {
-#if defined(OS_WIN)
+#if defined(OS_WIN) || defined(USE_X11)
   // Notify we are going away.
-  ::SetEvent(shutdown_event_->handle());
+  shutdown_event_->Signal();
 #endif
 
   // Mark all the profiles as clean.
@@ -264,9 +278,20 @@ 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.
-  g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
+#if defined(USE_X11)
+  //  Can't run a local loop on linux. Instead create a waitable event.
+  base::WaitableEvent done_writing(false, false);
+  ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
+      NewRunnableFunction(Signal, &done_writing));
+  done_writing.TimedWait(
+      base::TimeDelta::FromSeconds(kEndSessionTimeoutSeconds));
+#elif defined(OS_WIN)
+  ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
       NewRunnableFunction(PostQuit, MessageLoop::current()));
   MessageLoop::current()->Run();
+#else
+  NOTIMPLEMENTED();
+#endif
 }
 
 ResourceDispatcherHost* BrowserProcessImpl::resource_dispatcher_host() {
diff --git a/chrome/browser/browser_shutdown.cc b/chrome/browser/browser_shutdown.cc
index 7268404..7824149 100644
--- a/chrome/browser/browser_shutdown.cc
+++ b/chrome/browser/browser_shutdown.cc
@@ -289,4 +289,12 @@ bool IsTryingToQuit() {
   return g_trying_to_quit;
 }
 
+bool ShuttingDownWithoutClosingBrowsers() {
+#if defined(USE_X11)
+  if (GetShutdownType() == browser_shutdown::END_SESSION)
+    return true;
+#endif
+  return false;
+}
+
 }  // namespace browser_shutdown
diff --git a/chrome/browser/browser_shutdown.h b/chrome/browser/browser_shutdown.h
index c2e38b5..0df2919 100644
--- a/chrome/browser/browser_shutdown.h
+++ b/chrome/browser/browser_shutdown.h
@@ -66,6 +66,11 @@ void SetTryingToQuit(bool quitting);
 // General accessor.
 bool IsTryingToQuit();
 
+// This is true on X during an END_SESSION, when we can no longer depend
+// on the X server to be running. As a result we don't explicitly close the
+// browser windows, which can lead to conditions which would fail checks.
+bool ShuttingDownWithoutClosingBrowsers();
+
 }  // namespace browser_shutdown
 
 #endif  // CHROME_BROWSER_BROWSER_SHUTDOWN_H__
diff --git a/chrome/browser/chromeos/tab_closeable_state_watcher.cc b/chrome/browser/chromeos/tab_closeable_state_watcher.cc
index 536ba9b..899b88c 100644
--- a/chrome/browser/chromeos/tab_closeable_state_watcher.cc
+++ b/chrome/browser/chromeos/tab_closeable_state_watcher.cc
@@ -6,6 +6,7 @@
 
 #include "base/command_line.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
@@ -75,7 +76,8 @@ TabCloseableStateWatcher::TabCloseableStateWatcher()
 
 TabCloseableStateWatcher::~TabCloseableStateWatcher() {
   BrowserList::RemoveObserver(this);
-  DCHECK(tabstrip_watchers_.empty());
+  if (!browser_shutdown::ShuttingDownWithoutClosingBrowsers())
+    DCHECK(tabstrip_watchers_.empty());
 }
 
 bool TabCloseableStateWatcher::CanCloseTab(const Browser* browser) const {
diff --git a/chrome/browser/gtk/bookmark_bar_gtk.cc b/chrome/browser/gtk/bookmark_bar_gtk.cc
index 4465aaa..7f04be4 100644
--- a/chrome/browser/gtk/bookmark_bar_gtk.cc
+++ b/chrome/browser/gtk/bookmark_bar_gtk.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/bookmarks/bookmark_drag_data.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
+#include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/gtk/bookmark_menu_controller_gtk.h"
 #include "chrome/browser/gtk/bookmark_tree_model.h"
@@ -411,7 +412,8 @@ void BookmarkBarGtk::Loaded(BookmarkModel* model) {
 void BookmarkBarGtk::BookmarkModelBeingDeleted(BookmarkModel* model) {
   // The bookmark model should never be deleted before us. This code exists
   // to check for regressions in shutdown code and not crash.
-  NOTREACHED();
+  if (!browser_shutdown::ShuttingDownWithoutClosingBrowsers())
+    NOTREACHED();
 
   // Do minimal cleanup, presumably we'll be deleted shortly.
   model_->RemoveObserver(this);
diff --git a/chrome/browser/views/bookmark_bar_view.cc b/chrome/browser/views/bookmark_bar_view.cc
index 4aeab9c..f3ab049 100644
--- a/chrome/browser/views/bookmark_bar_view.cc
+++ b/chrome/browser/views/bookmark_bar_view.cc
@@ -19,6 +19,7 @@
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/browser.h"
+#include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/importer/importer_data_types.h"
 #include "chrome/browser/metrics/user_metrics.h"
@@ -968,9 +969,11 @@ void BookmarkBarView::Loaded(BookmarkModel* model) {
 }
 
 void BookmarkBarView::BookmarkModelBeingDeleted(BookmarkModel* model) {
-  // The bookmark model should never be deleted before us. This code exists
+  // In normal shutdown The bookmark model should never be deleted before us.
+  // When X exits suddenly though, it can happen, This code exists
   // to check for regressions in shutdown code and not crash.
-  NOTREACHED();
+  if (!browser_shutdown::ShuttingDownWithoutClosingBrowsers())
+    NOTREACHED();
 
   // Do minimal cleanup, presumably we'll be deleted shortly.
   NotifyModelChanged();