diff options
author | rkc@chromium.org <rkc@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-11-17 12:33:12 +0000 |
---|---|---|
committer | rkc@chromium.org <rkc@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-11-17 12:33:12 +0000 |
commit | f079d57f3ba5db51c3a5d16369903e683e0db195 (patch) | |
tree | 50b9a317b9d4c1a2a95c7ce0337194d7c60610a9 | |
parent | 06ccb7aefdde0e8af1e28af68cb9bb37bdbb858b (diff) | |
download | chromium_src-f079d57f3ba5db51c3a5d16369903e683e0db195.zip chromium_src-f079d57f3ba5db51c3a5d16369903e683e0db195.tar.gz chromium_src-f079d57f3ba5db51c3a5d16369903e683e0db195.tar.bz2 |
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
7 files changed, 43 insertions, 24 deletions
diff --git a/chrome/browser/chromeos/extensions/power/power_api_browsertest.cc b/chrome/browser/chromeos/extensions/power/power_api_browsertest.cc index f2896d9..99f5918 100644 --- a/chrome/browser/chromeos/extensions/power/power_api_browsertest.cc +++ b/chrome/browser/chromeos/extensions/power/power_api_browsertest.cc @@ -64,6 +64,8 @@ class PowerApiTest : public InProcessBrowserTest { bool boolean_value; result->GetAsBoolean(&boolean_value); EXPECT_EQ(boolean_value, true); + + MessageLoop::current()->RunUntilIdle(); } // Adds an expectation that RequestPowerStateOverrides() will be called once diff --git a/chrome/browser/chromeos/extensions/power/power_api_manager.cc b/chrome/browser/chromeos/extensions/power/power_api_manager.cc index 306a98b..f5627ab 100644 --- a/chrome/browser/chromeos/extensions/power/power_api_manager.cc +++ b/chrome/browser/chromeos/extensions/power/power_api_manager.cc @@ -37,7 +37,7 @@ void PowerApiManager::Observe(int type, UpdatePowerSettings(); } else if (type == chrome::NOTIFICATION_APP_TERMINATING) { // If the Chrome app is terminating, ensure we release our power overrides. - power_state_override_.reset(NULL); + power_state_override_ = NULL; } else { NOTREACHED() << "Unexpected notification " << type; } @@ -57,11 +57,11 @@ PowerApiManager::~PowerApiManager() {} void PowerApiManager::UpdatePowerSettings() { // If we have a wake lock and don't have the power state overriden. if (extension_ids_set_.size() && !power_state_override_.get()) { - power_state_override_.reset(new chromeos::PowerStateOverride( - chromeos::PowerStateOverride::BLOCK_DISPLAY_SLEEP)); + power_state_override_ = new chromeos::PowerStateOverride( + chromeos::PowerStateOverride::BLOCK_DISPLAY_SLEEP); // else, if we don't have any wake locks and do have a power override. } else if (extension_ids_set_.empty() && power_state_override_.get()) { - power_state_override_.reset(NULL); + power_state_override_ = NULL; } } diff --git a/chrome/browser/chromeos/extensions/power/power_api_manager.h b/chrome/browser/chromeos/extensions/power/power_api_manager.h index 30e25bf..cfaf3b9 100644 --- a/chrome/browser/chromeos/extensions/power/power_api_manager.h +++ b/chrome/browser/chromeos/extensions/power/power_api_manager.h @@ -51,7 +51,7 @@ class PowerApiManager : public content::NotificationObserver { content::NotificationRegistrar registrar_; - scoped_ptr<chromeos::PowerStateOverride> power_state_override_; + scoped_refptr<chromeos::PowerStateOverride> power_state_override_; // Set of extension IDs that have a keep awake lock. std::set<std::string> extension_ids_set_; 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 diff --git a/content/browser/power_save_blocker_chromeos.cc b/content/browser/power_save_blocker_chromeos.cc index afb0cc2..de9d07b 100644 --- a/content/browser/power_save_blocker_chromeos.cc +++ b/content/browser/power_save_blocker_chromeos.cc @@ -34,13 +34,13 @@ class PowerSaveBlocker::Delegate default: NOTREACHED() << "Unhandled block type " << type_; } - override_.reset(new chromeos::PowerStateOverride(mode)); + override_ = new chromeos::PowerStateOverride(mode); } // Resets the previously-created PowerStateOverride object. void RemoveBlock() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - override_.reset(); + override_ = NULL; } private: @@ -49,7 +49,7 @@ class PowerSaveBlocker::Delegate PowerSaveBlockerType type_; - scoped_ptr<chromeos::PowerStateOverride> override_; + scoped_refptr<chromeos::PowerStateOverride> override_; DISALLOW_COPY_AND_ASSIGN(Delegate); }; |