Make power state override refcounted.

In the case of power state override being created and immidiatelly destructed, we'll have a scenario in which the correct power state request won't get cancelled.
e.g.,
. User creates PowerStateOverride instance -> We call RequestPowerStateOverride
  - we wait for the callback from ChromeOS to get our request ID
. User deletes PowerStateOverride instance -> We call CancelPowerStateOverride with a 0 request ID
. We destruct, invalidating our weak pointers
. Callback from RequestPowerStateOverride never lands since the weakptr is invalidated
  - Power state override remains in effect for the next 10 minutes.


This has now changed to,
. User creates PowerStateOverride instance -> We call RequestPowerStateOverride, this increments our reference count to 2
  - we wait for the callback from ChromeOS to get our request ID
. User releases his refernce to PowerStateOverride instance -> We decrement our reference count to 1
. RequestPowerStateOverride returns, giving us our RequestID, and decrements our refernce count to 0
. Destructor gets called, which does a CancelPowerStateOverride with the correct request ID
  - The PowerStateOverride is cancelled


R=derat@chromium.org
BUG=151732


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

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

3 files changed, 33 insertions, 16 deletions

diff --git a/chromeos/power/power_state_override.cc b/chromeos/power/power_state_override.cc
index 7c07771..ab9af01 100644
--- a/chromeos/power/power_state_override.cc
+++ b/chromeos/power/power_state_override.cc
@@ -28,8 +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)) {
+      dbus_thread_manager_(DBusThreadManager::Get()) {
   switch (mode) {
     case BLOCK_DISPLAY_SLEEP:
       override_types_ |= (PowerManagerClient::DISABLE_IDLE_DIM |
@@ -46,11 +45,20 @@ PowerStateOverride::PowerStateOverride(Mode mode)
   dbus_thread_manager_->AddObserver(this);
 
   // request_id_ = 0 will create a new override request.
-  CallRequestPowerStateOverrides();
+  // We do a post task here to ensure that this request runs 'after' our
+  // constructor is done. If not, there is a possibility (though only in
+  // tests at the moment) that the power state override request executes
+  // and returns before the constructor has finished executing. This will
+  // cause an AddRef and a Release, the latter destructing our current
+  // instance even before the constructor has finished executing (as it does
+  // in the DownloadExtensionTest browsertests currently).
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&PowerStateOverride::CallRequestPowerStateOverrides, this));
 
   heartbeat_.Start(FROM_HERE,
                    base::TimeDelta::FromSeconds(kHeartbeatTimeInSecs),
-                   weak_ptr_factory_.GetWeakPtr(),
+                   this,
                    &PowerStateOverride::CallRequestPowerStateOverrides);
 }
 
@@ -78,8 +86,7 @@ void PowerStateOverride::CallRequestPowerStateOverrides() {
       base::TimeDelta::FromSeconds(
           kHeartbeatTimeInSecs + kRequestSlackInSecs),
       override_types_,
-      base::Bind(&PowerStateOverride::SetRequestId,
-                 weak_ptr_factory_.GetWeakPtr()));
+      base::Bind(&PowerStateOverride::SetRequestId, this));
 }
 
 void PowerStateOverride::CancelRequest() {
diff --git a/chromeos/power/power_state_override.h b/chromeos/power/power_state_override.h
index 8a180ea..86976ad 100644
--- a/chromeos/power/power_state_override.h
+++ b/chromeos/power/power_state_override.h
@@ -17,7 +17,9 @@ class DBusThreadManager;
 
 // This class overrides the current power state on the machine, disabling
 // a set of power management features.
-class CHROMEOS_EXPORT PowerStateOverride : public DBusThreadManagerObserver {
+class CHROMEOS_EXPORT PowerStateOverride
+    : public base::RefCountedThreadSafe<PowerStateOverride>,
+      public DBusThreadManagerObserver {
  public:
   enum Mode {
     // Blocks the screen from being dimmed or blanked due to user inactivity.
@@ -30,6 +32,12 @@ class CHROMEOS_EXPORT PowerStateOverride : public DBusThreadManagerObserver {
   };
 
   explicit PowerStateOverride(Mode mode);
+
+  // This destructor cancels the current power state override. There might be
+  // a very slight delay between the the last reference to an instance being
+  // released and the power state override being canceled. This is only in
+  // the case that the current instance has JUST been created and ChromeOS
+  // hasn't had a chance to service the initial power override request yet.
   virtual ~PowerStateOverride();
 
   // DBusThreadManagerObserver implementation:
@@ -62,8 +70,6 @@ class CHROMEOS_EXPORT PowerStateOverride : public DBusThreadManagerObserver {
 
   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 cba0feb..7b3c7da 100644
--- a/chromeos/power/power_state_override_unittest.cc
+++ b/chromeos/power/power_state_override_unittest.cc
@@ -68,24 +68,26 @@ TEST_F(PowerStateOverrideTest, AddAndRemoveOverrides) {
   EXPECT_CALL(*power_manager_client_,
               RequestPowerStateOverrides(0, _, kDisplayOverrides, _))
       .WillOnce(SaveArg<3>(&request_id_callback));
-  scoped_ptr<PowerStateOverride> override(
+  scoped_refptr<PowerStateOverride> override(
       new PowerStateOverride(PowerStateOverride::BLOCK_DISPLAY_SLEEP));
+  // Make sure our PostTask to the request power state overrides runs.
+  message_loop_.RunUntilIdle();
   request_id_callback.Run(kRequestId);
 
   // The request should be canceled when the PowerStateOverride is destroyed.
   EXPECT_CALL(*power_manager_client_, CancelPowerStateOverrides(kRequestId));
-  override.reset();
+  override = NULL;
 
   // Now send a request to just block the system from suspending.
-  Mock::VerifyAndClearExpectations(power_manager_client_);
   EXPECT_CALL(*power_manager_client_,
               RequestPowerStateOverrides(0, _, kSystemOverrides, _))
       .WillOnce(SaveArg<3>(&request_id_callback));
-  override.reset(
-      new PowerStateOverride(PowerStateOverride::BLOCK_SYSTEM_SUSPEND));
+  override = new PowerStateOverride(PowerStateOverride::BLOCK_SYSTEM_SUSPEND);
+  // Make sure our PostTask to the request power state overrides runs.
+  message_loop_.RunUntilIdle();
   request_id_callback.Run(kRequestId);
   EXPECT_CALL(*power_manager_client_, CancelPowerStateOverrides(kRequestId));
-  override.reset();
+  override = NULL;
 }
 
 TEST_F(PowerStateOverrideTest, DBusThreadManagerShutDown) {
@@ -93,8 +95,10 @@ TEST_F(PowerStateOverrideTest, DBusThreadManagerShutDown) {
   chromeos::PowerStateRequestIdCallback request_id_callback;
   EXPECT_CALL(*power_manager_client_, RequestPowerStateOverrides(0, _, _, _))
       .WillOnce(SaveArg<3>(&request_id_callback));
-  scoped_ptr<PowerStateOverride> override(
+  scoped_refptr<PowerStateOverride> override(
       new PowerStateOverride(PowerStateOverride::BLOCK_DISPLAY_SLEEP));
+  // Make sure our PostTask to the request power state overrides runs.
+  message_loop_.RunUntilIdle();
   request_id_callback.Run(kRequestId);
 
   // When the DBusThreadManager is about to be shut down, PowerStateOverride