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

3 files changed, 74 insertions, 18 deletions

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