Revert 59147 - Allow overriding of X error functions

BUG=50006 (and various other reports)
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.

There is a high ranking crash report on both linux and chromeos that happens whenever X sends an error to chrome. This change causes us to log and continue when we get a regular error from X. When we get an IO error, indicating X is gone, we attempt to shut down gracefully.

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

TBR=davemoore@chromium.org
Review URL: http://codereview.chromium.org/3332019

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

16 files changed, 24 insertions, 135 deletions

diff --git a/chrome/app/chrome_dll_main.cc b/chrome/app/chrome_dll_main.cc
index 19ab29e..02f554d 100644
--- a/chrome/app/chrome_dll_main.cc
+++ b/chrome/app/chrome_dll_main.cc
@@ -913,7 +913,7 @@ int ChromeMain(int argc, char** argv) {
     gtk_init(&argc, &argv);
     SetUpGLibLogHandler();
 
-    x11_util::SetDefaultX11ErrorHandlers();
+    x11_util::SetX11ErrorHandlers();
 #endif  // defined(OS_LINUX)
 
     rv = BrowserMain(main_params);
diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index 72a0b75..60d4620 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();
+    BrowserList::CloseAllBrowsers(true);
 
   return num_browsers == 0 ? YES : NO;
 }
diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index 31e09cc..0ce399d 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -223,15 +223,7 @@ void BrowserList::RemoveObserver(BrowserList::Observer* observer) {
 }
 
 // static
-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
+void BrowserList::CloseAllBrowsers(bool use_post) {
   // Tell everyone that we are shutting down.
   browser_shutdown::SetTryingToQuit(true);
 
@@ -241,7 +233,7 @@ void BrowserList::CloseAllBrowsers() {
 
   // 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 (force_exit || browsers_.empty()) {
+  if (browsers_.empty()) {
     NotificationService::current()->Notify(NotificationType::APP_TERMINATING,
                                            NotificationService::AllSources(),
                                            NotificationService::NoDetails());
@@ -284,10 +276,10 @@ void BrowserList::CloseAllBrowsersAndExit() {
 
 #if !defined(OS_MACOSX)
   // On most platforms, closing all windows causes the application to exit.
-  CloseAllBrowsers();
+  CloseAllBrowsers(true);
 #else
   // On the Mac, the application continues to run once all windows are closed.
-  // Terminate will result in a CloseAllBrowsers() call, and once (and if)
+  // Terminate will result in a CloseAllBrowsers(true) call, and once (and if)
   // that is done, will cause the application to exit cleanly.
   chrome_browser_application_mac::Terminate();
 #endif
@@ -311,7 +303,8 @@ void BrowserList::WindowsSessionEnding() {
   // Write important data first.
   g_browser_process->EndSession();
 
-  BrowserList::CloseAllBrowsers();
+  // Close all the browsers.
+  BrowserList::CloseAllBrowsers(false);
 
   // Send out notification. This is used during testing so that the test harness
   // can properly shutdown before we exit.
@@ -327,8 +320,6 @@ 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
@@ -367,7 +358,7 @@ void BrowserList::EndKeepAlive() {
     // (MessageLoop::current() == null).
     if (browsers_.empty() && !browser_shutdown::IsTryingToQuit() &&
         MessageLoop::current())
-      CloseAllBrowsers();
+      CloseAllBrowsers(true);
   }
 }
 
diff --git a/chrome/browser/browser_list.h b/chrome/browser/browser_list.h
index dcde7fb..9e52aba 100644
--- a/chrome/browser/browser_list.h
+++ b/chrome/browser/browser_list.h
@@ -96,17 +96,18 @@ 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 7a358be..46d3b0c 100644
--- a/chrome/browser/browser_main.cc
+++ b/chrome/browser/browser_main.cc
@@ -100,7 +100,6 @@
 #endif
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
-#include "chrome/browser/browser_main_gtk.h"
 #include "chrome/browser/gtk/gtk_util.h"
 #endif
 
@@ -1199,10 +1198,6 @@ 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 25147c8d..520c652 100644
--- a/chrome/browser/browser_main_gtk.cc
+++ b/chrome/browser/browser_main_gtk.cc
@@ -4,12 +4,8 @@
 
 #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"
@@ -18,30 +14,6 @@
 #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() {
 }
 
@@ -74,11 +46,3 @@ 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
deleted file mode 100644
index 234ff7a..0000000
--- a/chrome/browser/browser_main_gtk.h
+++ /dev/null
@@ -1,15 +0,0 @@
-// 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 682c491..539bf57 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -11,7 +11,6 @@
 #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"
@@ -75,13 +74,6 @@
 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),
@@ -223,16 +215,10 @@ 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());
@@ -254,9 +240,9 @@ unsigned int BrowserProcessImpl::ReleaseModule() {
 }
 
 void BrowserProcessImpl::EndSession() {
-#if defined(OS_WIN) || defined(USE_X11)
+#if defined(OS_WIN)
   // Notify we are going away.
-  shutdown_event_->Signal();
+  ::SetEvent(shutdown_event_->handle());
 #endif
 
   // Mark all the profiles as clean.
@@ -278,20 +264,9 @@ 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.
-  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,
+  g_browser_process->file_thread()->message_loop()->PostTask(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 7824149..7268404 100644
--- a/chrome/browser/browser_shutdown.cc
+++ b/chrome/browser/browser_shutdown.cc
@@ -289,12 +289,4 @@ 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 0df2919..c2e38b5 100644
--- a/chrome/browser/browser_shutdown.h
+++ b/chrome/browser/browser_shutdown.h
@@ -66,11 +66,6 @@ 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 899b88c..536ba9b 100644
--- a/chrome/browser/chromeos/tab_closeable_state_watcher.cc
+++ b/chrome/browser/chromeos/tab_closeable_state_watcher.cc
@@ -6,7 +6,6 @@
 
 #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"
@@ -76,8 +75,7 @@ TabCloseableStateWatcher::TabCloseableStateWatcher()
 
 TabCloseableStateWatcher::~TabCloseableStateWatcher() {
   BrowserList::RemoveObserver(this);
-  if (!browser_shutdown::ShuttingDownWithoutClosingBrowsers())
-    DCHECK(tabstrip_watchers_.empty());
+  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 7f04be4..4465aaa 100644
--- a/chrome/browser/gtk/bookmark_bar_gtk.cc
+++ b/chrome/browser/gtk/bookmark_bar_gtk.cc
@@ -12,7 +12,6 @@
 #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"
@@ -412,8 +411,7 @@ 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.
-  if (!browser_shutdown::ShuttingDownWithoutClosingBrowsers())
-    NOTREACHED();
+  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 f3ab049..4aeab9c 100644
--- a/chrome/browser/views/bookmark_bar_view.cc
+++ b/chrome/browser/views/bookmark_bar_view.cc
@@ -19,7 +19,6 @@
 #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"
@@ -969,11 +968,9 @@ void BookmarkBarView::Loaded(BookmarkModel* model) {
 }
 
 void BookmarkBarView::BookmarkModelBeingDeleted(BookmarkModel* model) {
-  // In normal shutdown The bookmark model should never be deleted before us.
-  // When X exits suddenly though, it can happen, This code exists
+  // The bookmark model should never be deleted before us. This code exists
   // to check for regressions in shutdown code and not crash.
-  if (!browser_shutdown::ShuttingDownWithoutClosingBrowsers())
-    NOTREACHED();
+  NOTREACHED();
 
   // Do minimal cleanup, presumably we'll be deleted shortly.
   NotifyModelChanged();
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index 613a903..07da92f 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -293,7 +293,6 @@
         'browser/browser_list.h',
         'browser/browser_main.cc',
         'browser/browser_main_gtk.cc',
-        'browser/browser_main_gtk.h',
         'browser/browser_main_mac.mm',
         'browser/browser_main_posix.cc',
         'browser/browser_main_posix.h',
diff --git a/chrome/gpu/gpu_thread.cc b/chrome/gpu/gpu_thread.cc
index f1e5148..a949e3b 100644
--- a/chrome/gpu/gpu_thread.cc
+++ b/chrome/gpu/gpu_thread.cc
@@ -26,7 +26,6 @@
 #endif
 
 #if defined(OS_LINUX)
-#include "app/x11_util.h"
 #include <gtk/gtk.h>
 #endif
 
@@ -58,7 +57,7 @@ GpuThread::GpuThread() {
     for (size_t i = 0; i < args.size(); ++i) {
       free(argv[i]);
     }
-    x11_util::SetDefaultX11ErrorHandlers();
+    x11_util::SetX11ErrorHandlers();
   }
 #endif
 }
diff --git a/chrome/plugin/plugin_thread.cc b/chrome/plugin/plugin_thread.cc
index 8e2b3ba..1bd9903 100644
--- a/chrome/plugin/plugin_thread.cc
+++ b/chrome/plugin/plugin_thread.cc
@@ -87,7 +87,7 @@ PluginThread::PluginThread()
     }
   }
 
-  x11_util::SetDefaultX11ErrorHandlers();
+  x11_util::SetX11ErrorHandlers();
 #endif
 
   PatchNPNFunctions();