chromeos: Fix PowerStateOverride/D-Bus lifetime issues.

This makes PowerStateOverride observe DBusThreadManager's
lifetime so it can unregister its request if the manager is
shutting down.

BUG=154203,152429


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

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

10 files changed, 176 insertions, 20 deletions

diff --git a/chromeos/dbus/dbus_thread_manager.cc b/chromeos/dbus/dbus_thread_manager.cc
index de18de0..1f4a14d 100644
--- a/chromeos/dbus/dbus_thread_manager.cc
+++ b/chromeos/dbus/dbus_thread_manager.cc
@@ -8,6 +8,7 @@
 
 #include "base/chromeos/chromeos_version.h"
 #include "base/command_line.h"
+#include "base/observer_list.h"
 #include "base/threading/thread.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/bluetooth_adapter_client.h"
@@ -20,6 +21,7 @@
 #include "chromeos/dbus/cros_disks_client.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "chromeos/dbus/dbus_client_implementation_type.h"
+#include "chromeos/dbus/dbus_thread_manager_observer.h"
 #include "chromeos/dbus/debug_daemon_client.h"
 #include "chromeos/dbus/shill_device_client.h"
 #include "chromeos/dbus/shill_ipconfig_client.h"
@@ -152,6 +154,9 @@ class DBusThreadManagerImpl : public DBusThreadManager {
   }
 
   virtual ~DBusThreadManagerImpl() {
+    FOR_EACH_OBSERVER(DBusThreadManagerObserver, observers_,
+                      OnDBusThreadManagerDestroying(this));
+
     // Shut down the bus. During the browser shutdown, it's ok to shut down
     // the bus synchronously.
     system_bus_->ShutdownOnDBusThreadAndBlock();
@@ -170,6 +175,18 @@ class DBusThreadManagerImpl : public DBusThreadManager {
   }
 
   // DBusThreadManager override.
+  virtual void AddObserver(DBusThreadManagerObserver* observer) OVERRIDE {
+    DCHECK(observer);
+    observers_.AddObserver(observer);
+  }
+
+  // DBusThreadManager override.
+  virtual void RemoveObserver(DBusThreadManagerObserver* observer) OVERRIDE {
+    DCHECK(observer);
+    observers_.RemoveObserver(observer);
+  }
+
+  // DBusThreadManager override.
   virtual void InitIBusBus(const std::string &ibus_address) OVERRIDE {
     DCHECK(!ibus_bus_);
     dbus::Bus::Options ibus_bus_options;
@@ -426,6 +443,8 @@ class DBusThreadManagerImpl : public DBusThreadManager {
   std::map<dbus::ObjectPath, IBusEngineService*> ibus_engine_services_;
 
   std::string ibus_address_;
+
+  ObserverList<DBusThreadManagerObserver> observers_;
 };
 
 // static
diff --git a/chromeos/dbus/dbus_thread_manager.h b/chromeos/dbus/dbus_thread_manager.h
index 90bfe8a..31205a5 100644
--- a/chromeos/dbus/dbus_thread_manager.h
+++ b/chromeos/dbus/dbus_thread_manager.h
@@ -22,6 +22,8 @@ class ObjectPath;
 
 namespace chromeos {
 
+class DBusThreadManagerObserver;
+
 // Style Note: Clients are sorted by names.
 class BluetoothAdapterClient;
 class BluetoothDeviceClient;
@@ -95,6 +97,10 @@ class CHROMEOS_EXPORT DBusThreadManager {
   // Gets the global instance. Initialize() must be called first.
   static DBusThreadManager* Get();
 
+  // Adds or removes an observer.
+  virtual void AddObserver(DBusThreadManagerObserver* observer) = 0;
+  virtual void RemoveObserver(DBusThreadManagerObserver* observer) = 0;
+
   // Creates new IBusBus instance to communicate with ibus-daemon with specified
   // ibus address. Must be called before using ibus related clients.
   // TODO(nona): Support shutdown to enable dynamical ibus-daemon shutdown.
diff --git a/chromeos/dbus/dbus_thread_manager_observer.h b/chromeos/dbus/dbus_thread_manager_observer.h
new file mode 100644
index 0000000..57e54ff
--- /dev/null
+++ b/chromeos/dbus/dbus_thread_manager_observer.h
@@ -0,0 +1,31 @@
+// Copyright (c) 2012 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.
+
+#ifndef CHROMEOS_DBUS_DBUS_THREAD_MANAGER_OBSERVER_H_
+#define CHROMEOS_DBUS_DBUS_THREAD_MANAGER_OBSERVER_H_
+
+#include "base/basictypes.h"
+
+namespace chromeos {
+
+class DBusThreadManager;
+
+// Interface for classes that are interested in observing changes to the
+// DBusThreadManager.
+class DBusThreadManagerObserver {
+ public:
+  DBusThreadManagerObserver() {}
+  virtual ~DBusThreadManagerObserver() {}
+
+  // Called when |manager| is about to be destroyed.  |manager| is still usable
+  // at this point.
+  virtual void OnDBusThreadManagerDestroying(DBusThreadManager* manager) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DBusThreadManagerObserver);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_DBUS_DBUS_THREAD_MANAGER_OBSERVER_H_
diff --git a/chromeos/dbus/mock_dbus_thread_manager.cc b/chromeos/dbus/mock_dbus_thread_manager.cc
index c025a07..cf3778e 100644
--- a/chromeos/dbus/mock_dbus_thread_manager.cc
+++ b/chromeos/dbus/mock_dbus_thread_manager.cc
@@ -4,6 +4,7 @@
 
 #include "chromeos/dbus/mock_dbus_thread_manager.h"
 
+#include "chromeos/dbus/dbus_thread_manager_observer.h"
 #include "chromeos/dbus/mock_bluetooth_adapter_client.h"
 #include "chromeos/dbus/mock_bluetooth_device_client.h"
 #include "chromeos/dbus/mock_bluetooth_input_client.h"
@@ -182,6 +183,20 @@ MockDBusThreadManager::MockDBusThreadManager()
       .Times(AnyNumber());
 }
 
-MockDBusThreadManager::~MockDBusThreadManager() {}
+MockDBusThreadManager::~MockDBusThreadManager() {
+  FOR_EACH_OBSERVER(DBusThreadManagerObserver, observers_,
+                    OnDBusThreadManagerDestroying(this));
+}
+
+void MockDBusThreadManager::AddObserver(DBusThreadManagerObserver* observer) {
+  DCHECK(observer);
+  observers_.AddObserver(observer);
+}
+
+void MockDBusThreadManager::RemoveObserver(
+    DBusThreadManagerObserver* observer) {
+  DCHECK(observer);
+  observers_.RemoveObserver(observer);
+}
 
 }  // namespace chromeos
diff --git a/chromeos/dbus/mock_dbus_thread_manager.h b/chromeos/dbus/mock_dbus_thread_manager.h
index 10c9907..cc84c77 100644
--- a/chromeos/dbus/mock_dbus_thread_manager.h
+++ b/chromeos/dbus/mock_dbus_thread_manager.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/observer_list.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
@@ -18,6 +19,7 @@ class Bus;
 
 namespace chromeos {
 
+class DBusThreadManagerObserver;
 class MockBluetoothAdapterClient;
 class MockBluetoothDeviceClient;
 class MockBluetoothInputClient;
@@ -54,6 +56,8 @@ class MockDBusThreadManager : public DBusThreadManager {
   MockDBusThreadManager();
   virtual ~MockDBusThreadManager();
 
+  void AddObserver(DBusThreadManagerObserver* observer) OVERRIDE;
+  void RemoveObserver(DBusThreadManagerObserver* observer) OVERRIDE;
   MOCK_METHOD1(InitIBusBus, void(const std::string& ibus_address));
   MOCK_METHOD0(GetSystemBus, dbus::Bus*(void));
   MOCK_METHOD0(GetIBusBus, dbus::Bus*(void));
@@ -206,6 +210,8 @@ class MockDBusThreadManager : public DBusThreadManager {
   scoped_ptr<MockSpeechSynthesizerClient> mock_speech_synthesizer_client_;
   scoped_ptr<MockUpdateEngineClient> mock_update_engine_client_;
 
+  ObserverList<DBusThreadManagerObserver> observers_;
+
   DISALLOW_COPY_AND_ASSIGN(MockDBusThreadManager);
 };
 
diff --git a/chromeos/dbus/mock_dbus_thread_manager_without_gmock.cc b/chromeos/dbus/mock_dbus_thread_manager_without_gmock.cc
index a22e906..b9cfbf3 100644
--- a/chromeos/dbus/mock_dbus_thread_manager_without_gmock.cc
+++ b/chromeos/dbus/mock_dbus_thread_manager_without_gmock.cc
@@ -4,6 +4,7 @@
 
 #include "chromeos/dbus/mock_dbus_thread_manager_without_gmock.h"
 
+#include "chromeos/dbus/dbus_thread_manager_observer.h"
 #include "chromeos/dbus/ibus/mock_ibus_client.h"
 #include "chromeos/dbus/ibus/mock_ibus_engine_factory_service.h"
 #include "chromeos/dbus/ibus/mock_ibus_engine_service.h"
@@ -17,7 +18,22 @@ MockDBusThreadManagerWithoutGMock::MockDBusThreadManagerWithoutGMock()
     ibus_bus_(NULL) {
 }
 
-MockDBusThreadManagerWithoutGMock::~MockDBusThreadManagerWithoutGMock() {}
+MockDBusThreadManagerWithoutGMock::~MockDBusThreadManagerWithoutGMock() {
+  FOR_EACH_OBSERVER(DBusThreadManagerObserver, observers_,
+                    OnDBusThreadManagerDestroying(this));
+}
+
+void MockDBusThreadManagerWithoutGMock::AddObserver(
+    DBusThreadManagerObserver* observer) {
+  DCHECK(observer);
+  observers_.AddObserver(observer);
+}
+
+void MockDBusThreadManagerWithoutGMock::RemoveObserver(
+    DBusThreadManagerObserver* observer) {
+  DCHECK(observer);
+  observers_.RemoveObserver(observer);
+}
 
 void MockDBusThreadManagerWithoutGMock::InitIBusBus(
     const std::string& ibus_address) {
diff --git a/chromeos/dbus/mock_dbus_thread_manager_without_gmock.h b/chromeos/dbus/mock_dbus_thread_manager_without_gmock.h
index 8d54646..c820599 100644
--- a/chromeos/dbus/mock_dbus_thread_manager_without_gmock.h
+++ b/chromeos/dbus/mock_dbus_thread_manager_without_gmock.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/logging.h"
+#include "base/observer_list.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 
 namespace dbus {
@@ -17,6 +18,7 @@ class ObjectPath;
 
 namespace chromeos {
 
+class DBusThreadManagerObserver;
 class MockIBusClient;
 class MockIBusEngineFactoryService;
 class MockIBusEngineService;
@@ -30,6 +32,8 @@ class MockDBusThreadManagerWithoutGMock : public DBusThreadManager {
   MockDBusThreadManagerWithoutGMock();
   virtual ~MockDBusThreadManagerWithoutGMock();
 
+  virtual void AddObserver(DBusThreadManagerObserver* observer) OVERRIDE;
+  virtual void RemoveObserver(DBusThreadManagerObserver* observer) OVERRIDE;
   virtual void InitIBusBus(const std::string& ibus_address) OVERRIDE;
   virtual dbus::Bus* GetSystemBus() OVERRIDE;
   virtual dbus::Bus* GetIBusBus() OVERRIDE;
@@ -97,6 +101,9 @@ class MockDBusThreadManagerWithoutGMock : public DBusThreadManager {
   scoped_ptr<MockIBusEngineFactoryService> mock_ibus_engine_factory_service_;
 
   dbus::Bus* ibus_bus_;
+
+  ObserverList<DBusThreadManagerObserver> observers_;
+
   DISALLOW_COPY_AND_ASSIGN(MockDBusThreadManagerWithoutGMock);
 };
 
diff --git a/chromeos/power/power_state_override.cc b/chromeos/power/power_state_override.cc
index 2383f80..7c07771 100644
--- a/chromeos/power/power_state_override.cc
+++ b/chromeos/power/power_state_override.cc
@@ -28,6 +28,7 @@ namespace chromeos {
 PowerStateOverride::PowerStateOverride(Mode mode)
     : override_types_(0),
       request_id_(0),
+      dbus_thread_manager_(DBusThreadManager::Get()),
       weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   switch (mode) {
     case BLOCK_DISPLAY_SLEEP:
@@ -42,6 +43,8 @@ PowerStateOverride::PowerStateOverride(Mode mode)
       NOTREACHED() << "Unhandled mode " << mode;
   }
 
+  dbus_thread_manager_->AddObserver(this);
+
   // request_id_ = 0 will create a new override request.
   CallRequestPowerStateOverrides();
 
@@ -52,12 +55,16 @@ PowerStateOverride::PowerStateOverride(Mode mode)
 }
 
 PowerStateOverride::~PowerStateOverride() {
-  heartbeat_.Stop();
+  if (dbus_thread_manager_)
+    dbus_thread_manager_->RemoveObserver(this);
+  CancelRequest();
+}
 
-  PowerManagerClient* power_manager =
-      DBusThreadManager::Get()->GetPowerManagerClient();
-  if (power_manager)
-    power_manager->CancelPowerStateOverrides(request_id_);
+void PowerStateOverride::OnDBusThreadManagerDestroying(
+    DBusThreadManager* manager) {
+  DCHECK_EQ(manager, dbus_thread_manager_);
+  CancelRequest();
+  dbus_thread_manager_ = NULL;
 }
 
 void PowerStateOverride::SetRequestId(uint32 request_id) {
@@ -65,16 +72,22 @@ void PowerStateOverride::SetRequestId(uint32 request_id) {
 }
 
 void PowerStateOverride::CallRequestPowerStateOverrides() {
-  PowerManagerClient* power_manager =
-      DBusThreadManager::Get()->GetPowerManagerClient();
-  if (power_manager) {
-    power_manager->RequestPowerStateOverrides(
-        request_id_,
-        base::TimeDelta::FromSeconds(
-            kHeartbeatTimeInSecs + kRequestSlackInSecs),
-        override_types_,
-        base::Bind(&PowerStateOverride::SetRequestId,
-                   weak_ptr_factory_.GetWeakPtr()));
+  DCHECK(dbus_thread_manager_);
+  dbus_thread_manager_->GetPowerManagerClient()->RequestPowerStateOverrides(
+      request_id_,
+      base::TimeDelta::FromSeconds(
+          kHeartbeatTimeInSecs + kRequestSlackInSecs),
+      override_types_,
+      base::Bind(&PowerStateOverride::SetRequestId,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PowerStateOverride::CancelRequest() {
+  if (request_id_) {
+    DCHECK(dbus_thread_manager_);
+    dbus_thread_manager_->GetPowerManagerClient()->
+        CancelPowerStateOverrides(request_id_);
+    request_id_ = 0;
   }
 }
 
diff --git a/chromeos/power/power_state_override.h b/chromeos/power/power_state_override.h
index 4e2fab9..8a180ea 100644
--- a/chromeos/power/power_state_override.h
+++ b/chromeos/power/power_state_override.h
@@ -9,12 +9,15 @@
 #include "base/memory/weak_ptr.h"
 #include "base/timer.h"
 #include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/dbus_thread_manager_observer.h"
 
 namespace chromeos {
 
+class DBusThreadManager;
+
 // This class overrides the current power state on the machine, disabling
 // a set of power management features.
-class CHROMEOS_EXPORT PowerStateOverride {
+class CHROMEOS_EXPORT PowerStateOverride : public DBusThreadManagerObserver {
  public:
   enum Mode {
     // Blocks the screen from being dimmed or blanked due to user inactivity.
@@ -27,7 +30,11 @@ class CHROMEOS_EXPORT PowerStateOverride {
   };
 
   explicit PowerStateOverride(Mode mode);
-  ~PowerStateOverride();
+  virtual ~PowerStateOverride();
+
+  // DBusThreadManagerObserver implementation:
+  virtual void OnDBusThreadManagerDestroying(DBusThreadManager* manager)
+      OVERRIDE;
 
  private:
   // Callback from RequestPowerStateOverride which receives our request_id.
@@ -38,6 +45,10 @@ class CHROMEOS_EXPORT PowerStateOverride {
   // since the last request has just been completed at that point.
   void CallRequestPowerStateOverrides();
 
+  // Asks the power manager to cancel |request_id_| and sets it to zero.
+  // Does nothing if it's already zero.
+  void CancelRequest();
+
   // Bitmap containing requested override types from
   // PowerManagerClient::PowerStateOverrideType.
   uint32 override_types_;
@@ -49,6 +60,8 @@ class CHROMEOS_EXPORT PowerStateOverride {
   // override.
   base::RepeatingTimer<PowerStateOverride> heartbeat_;
 
+  DBusThreadManager* dbus_thread_manager_;  // not owned
+
   base::WeakPtrFactory<PowerStateOverride> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PowerStateOverride);
diff --git a/chromeos/power/power_state_override_unittest.cc b/chromeos/power/power_state_override_unittest.cc
index 3deaab9..cba0feb 100644
--- a/chromeos/power/power_state_override_unittest.cc
+++ b/chromeos/power/power_state_override_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "chromeos/power/power_state_override.h"
 
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "chromeos/dbus/mock_power_manager_client.h"
 #include "chromeos/dbus/mock_dbus_thread_manager.h"
@@ -17,6 +19,9 @@ namespace chromeos {
 
 class PowerStateOverrideTest : public testing::Test {
  protected:
+  PowerStateOverrideTest() : dbus_thread_manager_shut_down_(false) {}
+  virtual ~PowerStateOverrideTest() {}
+
   virtual void SetUp() {
     MockDBusThreadManager* dbus_manager = new MockDBusThreadManager;
     DBusThreadManager::InitializeForTesting(dbus_manager);
@@ -24,14 +29,24 @@ class PowerStateOverrideTest : public testing::Test {
   }
 
   virtual void TearDown() {
-    DBusThreadManager::Shutdown();
+    if (!dbus_thread_manager_shut_down_)
+      DBusThreadManager::Shutdown();
   }
 
  protected:
+  // Shuts down the DBusThreadManager prematurely.
+  void ShutDownDBusThreadManager() {
+    DBusThreadManager::Shutdown();
+    dbus_thread_manager_shut_down_ = true;
+  }
+
   // Needed for PowerStateOverride's timer.
   MessageLoop message_loop_;
 
   MockPowerManagerClient* power_manager_client_;  // not owned
+
+  // Has the singleton DBusThreadManager already been manually shut down?
+  bool dbus_thread_manager_shut_down_;
 };
 
 TEST_F(PowerStateOverrideTest, AddAndRemoveOverrides) {
@@ -73,4 +88,19 @@ TEST_F(PowerStateOverrideTest, AddAndRemoveOverrides) {
   override.reset();
 }
 
+TEST_F(PowerStateOverrideTest, DBusThreadManagerShutDown) {
+  const uint32 kRequestId = 10;
+  chromeos::PowerStateRequestIdCallback request_id_callback;
+  EXPECT_CALL(*power_manager_client_, RequestPowerStateOverrides(0, _, _, _))
+      .WillOnce(SaveArg<3>(&request_id_callback));
+  scoped_ptr<PowerStateOverride> override(
+      new PowerStateOverride(PowerStateOverride::BLOCK_DISPLAY_SLEEP));
+  request_id_callback.Run(kRequestId);
+
+  // When the DBusThreadManager is about to be shut down, PowerStateOverride
+  // should cancel its request.
+  EXPECT_CALL(*power_manager_client_, CancelPowerStateOverrides(kRequestId));
+  ShutDownDBusThreadManager();
+}
+
 }  // namespace chromeos