Only initialize DBusThreadManager exactly once.

Also only shutdown initialized services in ChromeBrowserMainParts to reduce warning spam.

BUG=159854
For chrome/browser:
TBR=sky@chromium.org



Review URL: https://chromiumcodereview.appspot.com/11332005

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

10 files changed, 69 insertions, 55 deletions

diff --git a/chrome/browser/chrome_browser_main_linux.cc b/chrome/browser/chrome_browser_main_linux.cc
index 04a5f88..e764919 100644
--- a/chrome/browser/chrome_browser_main_linux.cc
+++ b/chrome/browser/chrome_browser_main_linux.cc
@@ -81,11 +81,13 @@ bool IsCrashReportingEnabled(const PrefService* local_state) {
 
 ChromeBrowserMainPartsLinux::ChromeBrowserMainPartsLinux(
     const content::MainFunctionParams& parameters)
-    : ChromeBrowserMainPartsPosix(parameters) {
+    : ChromeBrowserMainPartsPosix(parameters),
+      did_pre_profile_init_(false) {
 }
 
 ChromeBrowserMainPartsLinux::~ChromeBrowserMainPartsLinux() {
-  chrome::MediaTransferProtocolManager::Shutdown();
+  if (did_pre_profile_init_)
+    chrome::MediaTransferProtocolManager::Shutdown();
 }
 
 void ChromeBrowserMainPartsLinux::PreProfileInit() {
@@ -115,6 +117,8 @@ void ChromeBrowserMainPartsLinux::PreProfileInit() {
 
   chrome::MediaTransferProtocolManager::Initialize();
 
+  did_pre_profile_init_ = true;
+
   ChromeBrowserMainPartsPosix::PreProfileInit();
 }
 
diff --git a/chrome/browser/chrome_browser_main_linux.h b/chrome/browser/chrome_browser_main_linux.h
index 962bfbb..bcd99a6 100644
--- a/chrome/browser/chrome_browser_main_linux.h
+++ b/chrome/browser/chrome_browser_main_linux.h
@@ -39,15 +39,16 @@ class ChromeBrowserMainPartsLinux : public ChromeBrowserMainPartsPosix {
 
  private:
 #if defined(OS_CHROMEOS)
+  // TODO(stevenjb): Move these to ChromeBrowserMainChromeos.
   chromeos::VersionLoader cros_version_loader_;
   CancelableRequestConsumer cros_consumer_;
 #else
   scoped_refptr<chrome::RemovableDeviceNotificationsLinux>
       removable_device_notifications_linux_;
 #endif
-
   scoped_ptr<chrome::MediaTransferProtocolDeviceObserverLinux>
       media_transfer_protocol_device_observer_;
+  bool did_pre_profile_init_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsLinux);
 };
diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
index 707d38d..1256c9e 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
@@ -205,14 +205,21 @@ void OptionallyRunChromeOSLoginManager(const CommandLine& parsed_command_line,
 
 ChromeBrowserMainPartsChromeos::ChromeBrowserMainPartsChromeos(
     const content::MainFunctionParams& parameters)
-    : ChromeBrowserMainPartsLinux(parameters) {
+    : ChromeBrowserMainPartsLinux(parameters),
+      did_post_main_message_loop_start_(false) {
 }
 
 ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos() {
   if (chromeos::KioskModeSettings::Get()->IsKioskModeEnabled())
     chromeos::ShutdownKioskModeScreensaver();
-  cryptohome::AsyncMethodCaller::Shutdown();
-  chromeos::disks::DiskMountManager::Shutdown();
+
+  // PostMainMessageLoopStart() is not always called in tests. Calling
+  // Shutdown() on these can generate bogus WARNINGs or cause problems.
+  // TODO(stevenjb): Find a better way to do this for all Parts & stages.
+  if (did_post_main_message_loop_start_) {
+    cryptohome::AsyncMethodCaller::Shutdown();
+    chromeos::disks::DiskMountManager::Shutdown();
+  }
 
   // CrosLibrary is shut down before DBusThreadManager even though the former
   // is initialized before the latter becuase some of its libraries depend
@@ -222,10 +229,12 @@ ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos() {
   if (!parameters().ui_task && chromeos::CrosLibrary::Get())
     chromeos::CrosLibrary::Shutdown();
 
-  chromeos::input_method::InputMethodManager::Shutdown();
-
-  chromeos::CrosDBusService::Shutdown();
-  chromeos::DBusThreadManager::Shutdown();
+  if (did_post_main_message_loop_start_) {
+    chromeos::input_method::InputMethodManager::Shutdown();
+    chromeos::CrosDBusService::Shutdown();
+    // NOTE: This must only be called if Initialize() was called.
+    chromeos::DBusThreadManager::Shutdown();
+  }
 
   // To be precise, logout (browser shutdown) is not yet done, but the
   // remaining work is negligible, hence we say LogoutDone here.
@@ -290,6 +299,7 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopStart() {
   // Initialize DBusThreadManager for the browser. This must be done after
   // the main message loop is started, as it uses the message loop.
   chromeos::DBusThreadManager::Initialize();
+
   // Add observers for WallpaperManager. WallpaperManager is initialized before
   // DBusThreadManager.
   chromeos::WallpaperManager::Get()->AddObservers();
@@ -323,6 +333,8 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopStart() {
         GetXKeyboard()->SetNumLockEnabled(false);
   }
 
+  did_post_main_message_loop_start_ = true;
+
   ChromeBrowserMainPartsLinux::PostMainMessageLoopStart();
 }
 
diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.h b/chrome/browser/chromeos/chrome_browser_main_chromeos.h
index 073b65f..e6513ce 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.h
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.h
@@ -74,6 +74,7 @@ class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
   scoped_ptr<chromeos::ScreenDimmingObserver> screen_dimming_observer_;
   scoped_refptr<chromeos::RemovableDeviceNotificationsCros>
       removable_device_notifications_;
+  bool did_post_main_message_loop_start_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsChromeos);
 };
diff --git a/chromeos/dbus/dbus_thread_manager.cc b/chromeos/dbus/dbus_thread_manager.cc
index 53566ad..7ebd78d 100644
--- a/chromeos/dbus/dbus_thread_manager.cc
+++ b/chromeos/dbus/dbus_thread_manager.cc
@@ -49,6 +49,7 @@
 namespace chromeos {
 
 static DBusThreadManager* g_dbus_thread_manager = NULL;
+static bool g_dbus_thread_manager_set_for_testing = false;
 
 // The DBusThreadManager implementation used in production.
 class DBusThreadManagerImpl : public DBusThreadManager {
@@ -447,10 +448,12 @@ class DBusThreadManagerImpl : public DBusThreadManager {
 
 // static
 void DBusThreadManager::Initialize() {
-  if (g_dbus_thread_manager) {
-    LOG(WARNING) << "DBusThreadManager was already initialized";
+  // Ignore Initialize() if we set a test DBusThreadManager.
+  if (g_dbus_thread_manager_set_for_testing)
     return;
-  }
+  // If we initialize DBusThreadManager twice we may also be shutting it down
+  // early; do not allow that.
+  CHECK(g_dbus_thread_manager == NULL);
   // Determine whether we use stub or real client implementations.
   if (base::chromeos::IsRunningOnChromeOS()) {
     g_dbus_thread_manager =
@@ -466,29 +469,35 @@ void DBusThreadManager::Initialize() {
 // static
 void DBusThreadManager::InitializeForTesting(
     DBusThreadManager* dbus_thread_manager) {
-  if (g_dbus_thread_manager) {
-    LOG(WARNING) << "DBusThreadManager was already initialized";
-    return;
-  }
+  // If we initialize DBusThreadManager twice we may also be shutting it down
+  // early; do not allow that.
+  CHECK(g_dbus_thread_manager == NULL);
   CHECK(dbus_thread_manager);
   g_dbus_thread_manager = dbus_thread_manager;
+  g_dbus_thread_manager_set_for_testing = true;
   VLOG(1) << "DBusThreadManager initialized with test implementation";
 }
 
 // static
 void DBusThreadManager::InitializeWithStub() {
+  // If we initialize DBusThreadManager twice we may also be shutting it down
+  // early; do not allow that.
+  CHECK(g_dbus_thread_manager == NULL);
   g_dbus_thread_manager =
         new DBusThreadManagerImpl(STUB_DBUS_CLIENT_IMPLEMENTATION);
-    VLOG(1) << "DBusThreadManager initialized with stub implementation";
+  VLOG(1) << "DBusThreadManager initialized with stub implementation";
+}
+
+// static
+bool DBusThreadManager::IsInitialized() {
+  return g_dbus_thread_manager != NULL;
 }
 
 // static
 void DBusThreadManager::Shutdown() {
-  if (!g_dbus_thread_manager) {
-    // TODO(satorux): Make it a DCHECK() once it's ready.
-    LOG(WARNING) << "DBusThreadManager::Shutdown() called with NULL manager";
-    return;
-  }
+  // If we called InitializeForTesting, this may get called more than once.
+  // Ensure that we only shutdown DBusThreadManager once.
+  CHECK(g_dbus_thread_manager || g_dbus_thread_manager_set_for_testing);
   delete g_dbus_thread_manager;
   g_dbus_thread_manager = NULL;
   VLOG(1) << "DBusThreadManager Shutdown completed";
diff --git a/chromeos/dbus/dbus_thread_manager.h b/chromeos/dbus/dbus_thread_manager.h
index a246cc7..1944d5e 100644
--- a/chromeos/dbus/dbus_thread_manager.h
+++ b/chromeos/dbus/dbus_thread_manager.h
@@ -95,6 +95,10 @@ class CHROMEOS_EXPORT DBusThreadManager {
   // Initialize with stub implementations for tests based on stubs.
   static void InitializeWithStub();
 
+  // Returns true if DBusThreadManager has been initialized. Call this to
+  // avoid initializing + shutting down DBusThreadManager more than once.
+  static bool IsInitialized();
+
   // Destroys the global instance.
   static void Shutdown();
 
diff --git a/tools/heapcheck/suppressions.txt b/tools/heapcheck/suppressions.txt
index 01f7b97..93133c0 100644
--- a/tools/heapcheck/suppressions.txt
+++ b/tools/heapcheck/suppressions.txt
@@ -1644,10 +1644,3 @@
    fun:net::FileStream::Context::Orphan
    fun:net::FileStream::~FileStream
 }
-{
-   bug_159854
-   Heapcheck:Leak
-   ...
-   fun:chromeos::::ShillServiceClientStubImpl::GetObserverList
-   fun:chromeos::::ShillServiceClientStubImpl::NotifyObserversPropertyChanged
-}
diff --git a/tools/valgrind/memcheck/suppressions.txt b/tools/valgrind/memcheck/suppressions.txt
index 79ddcc2..6ade537 100644
--- a/tools/valgrind/memcheck/suppressions.txt
+++ b/tools/valgrind/memcheck/suppressions.txt
@@ -6101,13 +6101,3 @@
    fun:_ZN7WebCore11RenderBlock14styleDidChangeENS_15StyleDifferenceEPKNS_11RenderStyleE
    fun:_ZN7WebCore12RenderObject8setStyleEN3WTF10PassRefPtrINS_11RenderStyleEEE
 }
-{
-   bug_159854
-   Memcheck:Leak
-   fun:_Znw*
-   ...
-   fun:_ZNSt3mapIN4dbus10ObjectPathEP12ObserverListIN8chromeos28ShillPropertyChangedObserverELb0EESt4lessIS1_ESaISt4pairIKS1_S6_EEEixERSA_
-   fun:_ZN8chromeos12_GLOBAL__N_126ShillServiceClientStubImpl15GetObserverListERKN4dbus10ObjectPathE
-   fun:_ZN8chromeos12_GLOBAL__N_126ShillServiceClientStubImpl30NotifyObserversPropertyChangedERKN4dbus10ObjectPathERKSs
-}
-
diff --git a/ui/base/ime/text_input_test_support.cc b/ui/base/ime/text_input_test_support.cc
index 254b1bc..3d3bee5 100644
--- a/ui/base/ime/text_input_test_support.cc
+++ b/ui/base/ime/text_input_test_support.cc
@@ -8,23 +8,29 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #endif  // OS_CHROMEOS
 
-namespace ui {
-
-TextInputTestSupport::TextInputTestSupport() {
+#if defined(OS_CHROMEOS)
+namespace {
+bool dbus_thread_manager_was_initialized = false;
 }
+#endif  // OS_CHROMEOS
 
-TextInputTestSupport::~TextInputTestSupport() {
-}
+namespace ui {
 
 void TextInputTestSupport::Initialize() {
 #if defined(OS_CHROMEOS)
-  chromeos::DBusThreadManager::InitializeWithStub();
+  if (!chromeos::DBusThreadManager::IsInitialized()) {
+    chromeos::DBusThreadManager::InitializeWithStub();
+    dbus_thread_manager_was_initialized = true;
+  }
 #endif  // OS_CHROMEOS
 }
 
 void TextInputTestSupport::Shutdown() {
 #if defined(OS_CHROMEOS)
-  chromeos::DBusThreadManager::Shutdown();
+  if (dbus_thread_manager_was_initialized) {
+    chromeos::DBusThreadManager::Shutdown();
+    dbus_thread_manager_was_initialized = false;
+  }
 #endif  // OS_CHROMEOS
 }
 
diff --git a/ui/base/ime/text_input_test_support.h b/ui/base/ime/text_input_test_support.h
index 41e9eed..ad7a15f 100644
--- a/ui/base/ime/text_input_test_support.h
+++ b/ui/base/ime/text_input_test_support.h
@@ -11,17 +11,11 @@ namespace ui {
 
 class TextInputTestSupport {
  public:
-  TextInputTestSupport();
-  virtual ~TextInputTestSupport();
-
-  // Initialize DBusThreadManager for text input testing.
+  // Initialize DBusThreadManager for text input testing if necessary.
   static void Initialize();
 
-  // Shutdown DBusThreadManager.
+  // Shutdown DBusThreadManager if necessary.
   static void Shutdown();
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TextInputTestSupport);
 };
 
 }  // namespace ui