diff options
author | derat@chromium.org <derat@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-10-29 19:54:24 +0000 |
---|---|---|
committer | derat@chromium.org <derat@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-10-29 19:54:24 +0000 |
commit | eb5614468cee0ca2f6db9bf69dfa97bf942a0a64 (patch) | |
tree | e32860f5a3b204051e2ce85ee261ee298393700c | |
parent | 4859d067e10a262569b41a4558cb38f04d0bef54 (diff) | |
download | chromium_src-eb5614468cee0ca2f6db9bf69dfa97bf942a0a64.zip chromium_src-eb5614468cee0ca2f6db9bf69dfa97bf942a0a64.tar.gz chromium_src-eb5614468cee0ca2f6db9bf69dfa97bf942a0a64.tar.bz2 |
chromeos: Fix lock-before-suspend regression.
This fixes an uninitialized member variable in
ash::internal::PowerEventObserver that could result in the
screen not getting locked before the system goes to sleep.
BUG=312210
Review URL: https://codereview.chromium.org/49743004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@231622 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | ash/ash.gyp | 1 | ||||
-rw-r--r-- | ash/system/chromeos/power/power_event_observer.cc | 3 | ||||
-rw-r--r-- | ash/system/chromeos/power/power_event_observer.h | 6 | ||||
-rw-r--r-- | ash/system/chromeos/power/power_event_observer_unittest.cc | 72 | ||||
-rw-r--r-- | ash/test/ash_test_base.cc | 5 | ||||
-rw-r--r-- | ash/test/ash_test_base.h | 1 | ||||
-rw-r--r-- | ash/test/test_session_state_delegate.cc | 8 | ||||
-rw-r--r-- | ash/test/test_session_state_delegate.h | 6 | ||||
-rw-r--r-- | chromeos/dbus/fake_power_manager_client.cc | 4 | ||||
-rw-r--r-- | chromeos/dbus/fake_power_manager_client.h | 1 | ||||
-rw-r--r-- | chromeos/dbus/power_manager_client.cc | 26 | ||||
-rw-r--r-- | chromeos/dbus/power_manager_client.h | 4 |
12 files changed, 131 insertions, 6 deletions
diff --git a/ash/ash.gyp b/ash/ash.gyp index b8124d1..0e10994 100644 --- a/ash/ash.gyp +++ b/ash/ash.gyp @@ -784,6 +784,7 @@ 'shell_unittest.cc', 'system/chromeos/managed/tray_locally_managed_user_unittest.cc', 'system/chromeos/network/network_state_notifier_unittest.cc', + 'system/chromeos/power/power_event_observer_unittest.cc', 'system/chromeos/power/power_status_unittest.cc', 'system/chromeos/power/tray_power_unittest.cc', 'system/chromeos/screen_security/screen_tray_item_unittest.cc', diff --git a/ash/system/chromeos/power/power_event_observer.cc b/ash/system/chromeos/power/power_event_observer.cc index 2dc4a84..c99ae50 100644 --- a/ash/system/chromeos/power/power_event_observer.cc +++ b/ash/system/chromeos/power/power_event_observer.cc @@ -16,7 +16,8 @@ namespace ash { namespace internal { -PowerEventObserver::PowerEventObserver() { +PowerEventObserver::PowerEventObserver() + : screen_locked_(false) { chromeos::DBusThreadManager::Get()->GetPowerManagerClient()-> AddObserver(this); chromeos::DBusThreadManager::Get()->GetSessionManagerClient()-> diff --git a/ash/system/chromeos/power/power_event_observer.h b/ash/system/chromeos/power/power_event_observer.h index aeb89b9..b35cb1b 100644 --- a/ash/system/chromeos/power/power_event_observer.h +++ b/ash/system/chromeos/power/power_event_observer.h @@ -5,6 +5,7 @@ #ifndef ASH_SYSTEM_CHROMEOS_POWER_POWER_EVENT_OBSERVER_H_ #define ASH_SYSTEM_CHROMEOS_POWER_POWER_EVENT_OBSERVER_H_ +#include "ash/ash_export.h" #include "base/basictypes.h" #include "base/callback.h" #include "base/compiler_specific.h" @@ -15,8 +16,9 @@ namespace ash { namespace internal { // A class that observes power-management-related events. -class PowerEventObserver : public chromeos::PowerManagerClient::Observer, - public chromeos::SessionManagerClient::Observer { +class ASH_EXPORT PowerEventObserver + : public chromeos::PowerManagerClient::Observer, + public chromeos::SessionManagerClient::Observer { public: // This class registers/unregisters itself as an observer in ctor/dtor. PowerEventObserver(); diff --git a/ash/system/chromeos/power/power_event_observer_unittest.cc b/ash/system/chromeos/power/power_event_observer_unittest.cc new file mode 100644 index 0000000..a2282c3 --- /dev/null +++ b/ash/system/chromeos/power/power_event_observer_unittest.cc @@ -0,0 +1,72 @@ +// Copyright 2013 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. + +#include "ash/system/chromeos/power/power_event_observer.h" + +#include "ash/test/ash_test_base.h" +#include "base/memory/scoped_ptr.h" +#include "base/time/time.h" +#include "chromeos/dbus/dbus_thread_manager.h" +#include "chromeos/dbus/power_manager_client.h" + +namespace ash { +namespace internal { + +class PowerEventObserverTest : public test::AshTestBase { + public: + PowerEventObserverTest() {} + virtual ~PowerEventObserverTest() {} + + // test::AshTestBase::SetUp() overrides: + virtual void SetUp() OVERRIDE { + test::AshTestBase::SetUp(); + observer_.reset(new PowerEventObserver()); + } + + virtual void TearDown() OVERRIDE { + observer_.reset(); + test::AshTestBase::TearDown(); + } + + protected: + scoped_ptr<PowerEventObserver> observer_; + + private: + DISALLOW_COPY_AND_ASSIGN(PowerEventObserverTest); +}; + +TEST_F(PowerEventObserverTest, LockBeforeSuspend) { + chromeos::PowerManagerClient* client = + chromeos::DBusThreadManager::Get()->GetPowerManagerClient(); + ASSERT_EQ(0, client->GetNumPendingSuspendReadinessCallbacks()); + + // Check that the observer requests a suspend-readiness callback when it hears + // that the system is about to suspend. + SetCanLockScreen(true); + SetShouldLockScreenBeforeSuspending(true); + observer_->SuspendImminent(); + EXPECT_EQ(1, client->GetNumPendingSuspendReadinessCallbacks()); + + // It should run the callback when it hears that the screen is locked. + observer_->ScreenIsLocked(); + RunAllPendingInMessageLoop(); + EXPECT_EQ(0, client->GetNumPendingSuspendReadinessCallbacks()); + + // If the system is already locked, no callback should be requested. + observer_->SystemResumed(base::TimeDelta()); + observer_->ScreenIsUnlocked(); + observer_->ScreenIsLocked(); + observer_->SuspendImminent(); + EXPECT_EQ(0, client->GetNumPendingSuspendReadinessCallbacks()); + + // It also shouldn't request a callback if it isn't instructed to lock the + // screen. + observer_->SystemResumed(base::TimeDelta()); + SetShouldLockScreenBeforeSuspending(false); + observer_->SuspendImminent(); + EXPECT_EQ(0, client->GetNumPendingSuspendReadinessCallbacks()); +} + +} // namespace internal +} // namespace ash diff --git a/ash/test/ash_test_base.cc b/ash/test/ash_test_base.cc index c82ae75..945b7ca 100644 --- a/ash/test/ash_test_base.cc +++ b/ash/test/ash_test_base.cc @@ -293,6 +293,11 @@ void AshTestBase::SetCanLockScreen(bool can_lock_screen) { SetCanLockScreen(can_lock_screen); } +void AshTestBase::SetShouldLockScreenBeforeSuspending(bool should_lock) { + ash_test_helper_->test_shell_delegate()->test_session_state_delegate()-> + SetShouldLockScreenBeforeSuspending(should_lock); +} + void AshTestBase::SetUserAddingScreenRunning(bool user_adding_screen_running) { ash_test_helper_->test_shell_delegate()->test_session_state_delegate()-> SetUserAddingScreenRunning(user_adding_screen_running); diff --git a/ash/test/ash_test_base.h b/ash/test/ash_test_base.h index b88f26cc..7b5bb44 100644 --- a/ash/test/ash_test_base.h +++ b/ash/test/ash_test_base.h @@ -122,6 +122,7 @@ class AshTestBase : public testing::Test { void SetSessionStarted(bool session_started); void SetUserLoggedIn(bool user_logged_in); void SetCanLockScreen(bool can_lock_screen); + void SetShouldLockScreenBeforeSuspending(bool should_lock); void SetUserAddingScreenRunning(bool user_adding_screen_running); // Methods to emulate blocking and unblocking user session with given diff --git a/ash/test/test_session_state_delegate.cc b/ash/test/test_session_state_delegate.cc index e18f7d1..0373602 100644 --- a/ash/test/test_session_state_delegate.cc +++ b/ash/test/test_session_state_delegate.cc @@ -31,6 +31,7 @@ TestSessionStateDelegate::TestSessionStateDelegate() : has_active_user_(false), active_user_session_started_(false), can_lock_screen_(true), + should_lock_screen_before_suspending_(false), screen_locked_(false), user_adding_screen_running_(false), logged_in_users_(1) { @@ -61,7 +62,7 @@ bool TestSessionStateDelegate::IsScreenLocked() const { } bool TestSessionStateDelegate::ShouldLockScreenBeforeSuspending() const { - return false; + return should_lock_screen_before_suspending_; } void TestSessionStateDelegate::LockScreen() { @@ -101,6 +102,11 @@ void TestSessionStateDelegate::SetCanLockScreen(bool can_lock_screen) { can_lock_screen_ = can_lock_screen; } +void TestSessionStateDelegate::SetShouldLockScreenBeforeSuspending( + bool should_lock) { + should_lock_screen_before_suspending_ = should_lock; +} + void TestSessionStateDelegate::SetUserAddingScreenRunning( bool user_adding_screen_running) { user_adding_screen_running_ = user_adding_screen_running; diff --git a/ash/test/test_session_state_delegate.h b/ash/test/test_session_state_delegate.h index 105c0b1..553f1ea 100644 --- a/ash/test/test_session_state_delegate.h +++ b/ash/test/test_session_state_delegate.h @@ -69,6 +69,9 @@ class TestSessionStateDelegate : public SessionStateDelegate { // is an active user. void SetCanLockScreen(bool can_lock_screen); + // Updates |should_lock_screen_before_suspending_|. + void SetShouldLockScreenBeforeSuspending(bool should_lock); + // Updates the internal state that indicates whether user adding screen is // running now. void SetUserAddingScreenRunning(bool user_adding_screen_running); @@ -86,6 +89,9 @@ class TestSessionStateDelegate : public SessionStateDelegate { // when this is |true| and there is an active user. bool can_lock_screen_; + // Return value for ShouldLockScreenBeforeSuspending(). + bool should_lock_screen_before_suspending_; + // Whether the screen is currently locked. bool screen_locked_; diff --git a/chromeos/dbus/fake_power_manager_client.cc b/chromeos/dbus/fake_power_manager_client.cc index 3c70a6f..81ca360 100644 --- a/chromeos/dbus/fake_power_manager_client.cc +++ b/chromeos/dbus/fake_power_manager_client.cc @@ -51,6 +51,10 @@ base::Closure FakePowerManagerClient::GetSuspendReadinessCallback() { return base::Closure(); } +int FakePowerManagerClient::GetNumPendingSuspendReadinessCallbacks() { + return 0; +} + bool FakePowerManagerClient::HasObserver(Observer* observer) { return false; } diff --git a/chromeos/dbus/fake_power_manager_client.h b/chromeos/dbus/fake_power_manager_client.h index d89bb51..ae1adc5 100644 --- a/chromeos/dbus/fake_power_manager_client.h +++ b/chromeos/dbus/fake_power_manager_client.h @@ -47,6 +47,7 @@ class FakePowerManagerClient : public PowerManagerClient { const power_manager::PowerManagementPolicy& policy) OVERRIDE; virtual void SetIsProjecting(bool is_projecting) OVERRIDE; virtual base::Closure GetSuspendReadinessCallback() OVERRIDE; + virtual int GetNumPendingSuspendReadinessCallbacks() OVERRIDE; power_manager::PowerManagementPolicy& get_policy() { return policy_; } diff --git a/chromeos/dbus/power_manager_client.cc b/chromeos/dbus/power_manager_client.cc index 82b934a..c065c09 100644 --- a/chromeos/dbus/power_manager_client.cc +++ b/chromeos/dbus/power_manager_client.cc @@ -211,6 +211,10 @@ class PowerManagerClientImpl : public PowerManagerClient { weak_ptr_factory_.GetWeakPtr(), pending_suspend_id_); } + virtual int GetNumPendingSuspendReadinessCallbacks() OVERRIDE { + return num_pending_suspend_readiness_callbacks_; + } + protected: virtual void Init(dbus::Bus* bus) OVERRIDE { power_manager_proxy_ = bus->GetObjectProxy( @@ -641,11 +645,16 @@ class PowerManagerClientStubImpl : public PowerManagerClient { brightness_(50.0), pause_count_(2), cycle_count_(0), + num_pending_suspend_readiness_callbacks_(0), weak_ptr_factory_(this) {} virtual ~PowerManagerClientStubImpl() {} - // PowerManagerClient overrides + int num_pending_suspend_readiness_callbacks() const { + return num_pending_suspend_readiness_callbacks_; + } + + // PowerManagerClient overrides: virtual void Init(dbus::Bus* bus) OVERRIDE { if (CommandLine::ForCurrentProcess()->HasSwitch( chromeos::switches::kEnableStubInteractive)) { @@ -714,10 +723,19 @@ class PowerManagerClientStubImpl : public PowerManagerClient { const power_manager::PowerManagementPolicy& policy) OVERRIDE {} virtual void SetIsProjecting(bool is_projecting) OVERRIDE {} virtual base::Closure GetSuspendReadinessCallback() OVERRIDE { - return base::Closure(); + num_pending_suspend_readiness_callbacks_++; + return base::Bind(&PowerManagerClientStubImpl::HandleSuspendReadiness, + weak_ptr_factory_.GetWeakPtr()); + } + virtual int GetNumPendingSuspendReadinessCallbacks() OVERRIDE { + return num_pending_suspend_readiness_callbacks_; } private: + void HandleSuspendReadiness() { + num_pending_suspend_readiness_callbacks_--; + } + void UpdateStatus() { if (pause_count_ > 0) { pause_count_--; @@ -802,6 +820,10 @@ class PowerManagerClientStubImpl : public PowerManagerClient { base::RepeatingTimer<PowerManagerClientStubImpl> update_timer_; power_manager::PowerSupplyProperties props_; + // Number of callbacks returned by GetSuspendReadinessCallback() but not yet + // invoked. + int num_pending_suspend_readiness_callbacks_; + // Note: This should remain the last member so it'll be destroyed and // invalidate its weak pointers before any other members are destroyed. base::WeakPtrFactory<PowerManagerClientStubImpl> weak_ptr_factory_; diff --git a/chromeos/dbus/power_manager_client.h b/chromeos/dbus/power_manager_client.h index a24312b..5c957f4 100644 --- a/chromeos/dbus/power_manager_client.h +++ b/chromeos/dbus/power_manager_client.h @@ -143,6 +143,10 @@ class CHROMEOS_EXPORT PowerManagerClient : public DBusClient { // readiness for suspend. See Observer::SuspendImminent(). virtual base::Closure GetSuspendReadinessCallback() = 0; + // Returns the number of callbacks returned by GetSuspendReadinessCallback() + // for the current suspend attempt but not yet called. Used by tests. + virtual int GetNumPendingSuspendReadinessCallbacks() = 0; + // Creates the instance. static PowerManagerClient* Create(DBusClientImplementationType type); |