diff options
author | stepco@chromium.org <stepco@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-09-07 00:42:21 +0000 |
---|---|---|
committer | stepco@chromium.org <stepco@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-09-07 00:42:21 +0000 |
commit | 02d8afa2d018c17cd560252561b125928bcb2152 (patch) | |
tree | 46fbed26aa10799528ee57224c4f3e7762d1f1ab | |
parent | 57dbc99bb94628569852954246429364a6d51caa (diff) | |
download | chromium_src-02d8afa2d018c17cd560252561b125928bcb2152.zip chromium_src-02d8afa2d018c17cd560252561b125928bcb2152.tar.gz chromium_src-02d8afa2d018c17cd560252561b125928bcb2152.tar.bz2 |
Fix policy invalidator lifecycle bugs for Android.
On Android, the user may disconnect from their Google account and reconnect multiple times within the same session. This case wasn't considered properly when developing the CloudPolicyInvalidator.
When the account is disconnected, the CloudPolicyCore is disconnected before the CloudPolicyStore is reset. When the store is reset, the invalidator unregisters for the policy object and makes a callback to indicate that invalidations are disabled. However, since the CloudPolicyCore is disconnected at this point, a crash is occurring because the code blindly attempts to use objects connected to the core.
Also, when the account becomes reconnected, the refresh scheduler is restarted. This caused the invalidator to be initialized again, which is unexpected.
The solution to these issues is to refactor the CloudPolicyInvalidator to support starting and stopping. The invalidator will listen to events published by the CloudPolicyCore to determine when to start and stop. The invalidator starts when the refresh scheduler starts and stops when it stops.
Also, when CloudPolicyInvalidator::Shutdown is called, outstanding weak pointers held in callbacks should be invalidated. This may not cause any issues since it is likely no messages are pumped on the thread in between the calls to Shutdown and the destructor, but should be fixed.
BUG=280345,272574,280536
Review URL: https://chromiumcodereview.appspot.com/23592017
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221837 0039d316-1c4b-4281-b951-d872f2087c98
16 files changed, 489 insertions, 467 deletions
diff --git a/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc b/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc index 9f50d06..d72429b 100644 --- a/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc +++ b/chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc @@ -239,7 +239,7 @@ void DeviceCloudPolicyManagerChromeOS::EnrollmentCompleted( EnrollmentStatus status) { if (status.status() == EnrollmentStatus::STATUS_SUCCESS) { core()->Connect(enrollment_handler_->ReleaseClient()); - StartRefreshScheduler(); + core()->StartRefreshScheduler(); core()->TrackRefreshDelayPref(local_state_, prefs::kDevicePolicyRefreshRate); attestation_policy_observer_.reset( @@ -260,7 +260,7 @@ void DeviceCloudPolicyManagerChromeOS::StartIfManaged() { store()->is_managed() && !service()) { core()->Connect(CreateClient()); - StartRefreshScheduler(); + core()->StartRefreshScheduler(); core()->TrackRefreshDelayPref(local_state_, prefs::kDevicePolicyRefreshRate); attestation_policy_observer_.reset( diff --git a/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc b/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc index 1c8c03d..ebfbfd4 100644 --- a/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc +++ b/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.cc @@ -343,7 +343,7 @@ void UserCloudPolicyManagerChromeOS::StartRefreshSchedulerIfReady() { return; } - StartRefreshScheduler(); + core()->StartRefreshScheduler(); core()->TrackRefreshDelayPref(local_state_, prefs::kUserPolicyRefreshRate); } diff --git a/chrome/browser/policy/cloud/DEPS b/chrome/browser/policy/cloud/DEPS index 184b2d2..62375e2 100644 --- a/chrome/browser/policy/cloud/DEPS +++ b/chrome/browser/policy/cloud/DEPS @@ -53,7 +53,6 @@ specific_include_rules = { r"cloud_policy_invalidator\.cc": [ "+chrome/browser/invalidation/invalidation_service.h", - "+chrome/browser/invalidation/invalidation_service_factory.h", "+chrome/common/chrome_switches.h", ], @@ -110,6 +109,7 @@ specific_include_rules = { r"user_cloud_policy_invalidator\.cc": [ "+chrome/browser/chrome_notification_types.h", + "+chrome/browser/invalidation/invalidation_service_factory.h", "+content/public/browser/notification_source.h", ], diff --git a/chrome/browser/policy/cloud/cloud_policy_core.cc b/chrome/browser/policy/cloud/cloud_policy_core.cc index 68dd175..52eebac 100644 --- a/chrome/browser/policy/cloud/cloud_policy_core.cc +++ b/chrome/browser/policy/cloud/cloud_policy_core.cc @@ -15,6 +15,8 @@ namespace policy { +CloudPolicyCore::Observer::~Observer() {} + CloudPolicyCore::CloudPolicyCore(const PolicyNamespaceKey& key, CloudPolicyStore* store) : policy_ns_key_(key), @@ -27,9 +29,12 @@ void CloudPolicyCore::Connect(scoped_ptr<CloudPolicyClient> client) { CHECK(client); client_ = client.Pass(); service_.reset(new CloudPolicyService(policy_ns_key_, client_.get(), store_)); + FOR_EACH_OBSERVER(Observer, observers_, OnCoreConnected(this)); } void CloudPolicyCore::Disconnect() { + if (client_) + FOR_EACH_OBSERVER(Observer, observers_, OnCoreDisconnecting(this)); refresh_delay_.reset(); refresh_scheduler_.reset(); service_.reset(); @@ -48,6 +53,7 @@ void CloudPolicyCore::StartRefreshScheduler() { client_.get(), store_, base::MessageLoop::current()->message_loop_proxy())); UpdateRefreshDelayFromPref(); + FOR_EACH_OBSERVER(Observer, observers_, OnRefreshSchedulerStarted(this)); } } @@ -62,6 +68,14 @@ void CloudPolicyCore::TrackRefreshDelayPref( UpdateRefreshDelayFromPref(); } +void CloudPolicyCore::AddObserver(CloudPolicyCore::Observer* observer) { + observers_.AddObserver(observer); +} + +void CloudPolicyCore::RemoveObserver(CloudPolicyCore::Observer* observer) { + observers_.RemoveObserver(observer); +} + void CloudPolicyCore::UpdateRefreshDelayFromPref() { if (refresh_scheduler_ && refresh_delay_) refresh_scheduler_->SetRefreshDelay(refresh_delay_->GetValue()); diff --git a/chrome/browser/policy/cloud/cloud_policy_core.h b/chrome/browser/policy/cloud/cloud_policy_core.h index 96135dc..6158ee4 100644 --- a/chrome/browser/policy/cloud/cloud_policy_core.h +++ b/chrome/browser/policy/cloud/cloud_policy_core.h @@ -9,6 +9,7 @@ #include "base/basictypes.h" #include "base/memory/scoped_ptr.h" +#include "base/observer_list.h" #include "base/prefs/pref_member.h" #include "chrome/browser/policy/cloud/cloud_policy_constants.h" @@ -29,6 +30,21 @@ class CloudPolicyStore; // CloudPolicyRefreshScheduler which triggers periodic refreshes. class CloudPolicyCore { public: + // Callbacks for policy core events. + class Observer { + public: + virtual ~Observer(); + + // Called after the core is connected. + virtual void OnCoreConnected(CloudPolicyCore* core) = 0; + + // Called after the refresh scheduler is started. + virtual void OnRefreshSchedulerStarted(CloudPolicyCore* core) = 0; + + // Called before the core is disconnected. + virtual void OnCoreDisconnecting(CloudPolicyCore* core) = 0; + }; + CloudPolicyCore(const PolicyNamespaceKey& policy_ns_key, CloudPolicyStore* store); ~CloudPolicyCore(); @@ -67,6 +83,12 @@ class CloudPolicyCore { void TrackRefreshDelayPref(PrefService* pref_service, const std::string& refresh_pref_name); + // Registers an observer to be notified of policy core events. + void AddObserver(Observer* observer); + + // Removes the specified observer. + void RemoveObserver(Observer* observer); + private: // Updates the refresh scheduler on refresh delay changes. void UpdateRefreshDelayFromPref(); @@ -77,6 +99,7 @@ class CloudPolicyCore { scoped_ptr<CloudPolicyService> service_; scoped_ptr<CloudPolicyRefreshScheduler> refresh_scheduler_; scoped_ptr<IntegerPrefMember> refresh_delay_; + ObserverList<Observer, true> observers_; DISALLOW_COPY_AND_ASSIGN(CloudPolicyCore); }; diff --git a/chrome/browser/policy/cloud/cloud_policy_core_unittest.cc b/chrome/browser/policy/cloud/cloud_policy_core_unittest.cc index 07de17c..47492f5 100644 --- a/chrome/browser/policy/cloud/cloud_policy_core_unittest.cc +++ b/chrome/browser/policy/cloud/cloud_policy_core_unittest.cc @@ -17,13 +17,47 @@ namespace policy { -class CloudPolicyCoreTest : public testing::Test { +class CloudPolicyCoreTest : public testing::Test, + public CloudPolicyCore::Observer { protected: CloudPolicyCoreTest() : core_(PolicyNamespaceKey(dm_protocol::kChromeUserPolicyType, std::string()), - &store_) { + &store_), + core_connected_callback_count_(0), + refresh_scheduler_started_callback_count_(0), + core_disconnecting_callback_count_(0), + bad_callback_count_(0) { chrome::RegisterLocalState(prefs_.registry()); + core_.AddObserver(this); + } + + virtual ~CloudPolicyCoreTest() { + core_.RemoveObserver(this); + } + + virtual void OnCoreConnected(CloudPolicyCore* core) OVERRIDE { + // Make sure core is connected at callback time. + if (core_.client()) + core_connected_callback_count_++; + else + bad_callback_count_++; + } + + virtual void OnRefreshSchedulerStarted(CloudPolicyCore* core) OVERRIDE { + // Make sure refresh scheduler is started at callback time. + if (core_.refresh_scheduler()) + refresh_scheduler_started_callback_count_++; + else + bad_callback_count_++; + } + + virtual void OnCoreDisconnecting(CloudPolicyCore* core) OVERRIDE { + // Make sure core is still connected at callback time. + if (core_.client()) + core_disconnecting_callback_count_++; + else + bad_callback_count_++; } base::MessageLoop loop_; @@ -32,6 +66,11 @@ class CloudPolicyCoreTest : public testing::Test { MockCloudPolicyStore store_; CloudPolicyCore core_; + int core_connected_callback_count_; + int refresh_scheduler_started_callback_count_; + int core_disconnecting_callback_count_; + int bad_callback_count_; + private: DISALLOW_COPY_AND_ASSIGN(CloudPolicyCoreTest); }; @@ -47,18 +86,28 @@ TEST_F(CloudPolicyCoreTest, ConnectAndDisconnect) { EXPECT_TRUE(core_.client()); EXPECT_TRUE(core_.service()); EXPECT_FALSE(core_.refresh_scheduler()); + EXPECT_EQ(1, core_connected_callback_count_); + EXPECT_EQ(0, refresh_scheduler_started_callback_count_); + EXPECT_EQ(0, core_disconnecting_callback_count_); // Disconnect() goes back to no client and service. core_.Disconnect(); EXPECT_FALSE(core_.client()); EXPECT_FALSE(core_.service()); EXPECT_FALSE(core_.refresh_scheduler()); + EXPECT_EQ(1, core_connected_callback_count_); + EXPECT_EQ(0, refresh_scheduler_started_callback_count_); + EXPECT_EQ(1, core_disconnecting_callback_count_); // Calling Disconnect() twice doesn't do bad things. core_.Disconnect(); EXPECT_FALSE(core_.client()); EXPECT_FALSE(core_.service()); EXPECT_FALSE(core_.refresh_scheduler()); + EXPECT_EQ(1, core_connected_callback_count_); + EXPECT_EQ(0, refresh_scheduler_started_callback_count_); + EXPECT_EQ(1, core_disconnecting_callback_count_); + EXPECT_EQ(0, bad_callback_count_); } TEST_F(CloudPolicyCoreTest, RefreshScheduler) { @@ -76,6 +125,11 @@ TEST_F(CloudPolicyCoreTest, RefreshScheduler) { prefs_.ClearPref(prefs::kUserPolicyRefreshRate); EXPECT_EQ(default_refresh_delay, core_.refresh_scheduler()->refresh_delay()); + + EXPECT_EQ(1, core_connected_callback_count_); + EXPECT_EQ(1, refresh_scheduler_started_callback_count_); + EXPECT_EQ(0, core_disconnecting_callback_count_); + EXPECT_EQ(0, bad_callback_count_); } } // namespace policy diff --git a/chrome/browser/policy/cloud/cloud_policy_invalidator.cc b/chrome/browser/policy/cloud/cloud_policy_invalidator.cc index 5012fe7..e880225 100644 --- a/chrome/browser/policy/cloud/cloud_policy_invalidator.cc +++ b/chrome/browser/policy/cloud/cloud_policy_invalidator.cc @@ -14,7 +14,8 @@ #include "base/time/time.h" #include "base/values.h" #include "chrome/browser/invalidation/invalidation_service.h" -#include "chrome/browser/invalidation/invalidation_service_factory.h" +#include "chrome/browser/policy/cloud/cloud_policy_client.h" +#include "chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h" #include "chrome/browser/policy/cloud/enterprise_metrics.h" #include "chrome/common/chrome_switches.h" #include "policy/policy_constants.h" @@ -28,13 +29,11 @@ const int CloudPolicyInvalidator::kMaxFetchDelayMin = 1000; const int CloudPolicyInvalidator::kMaxFetchDelayMax = 300000; CloudPolicyInvalidator::CloudPolicyInvalidator( - CloudPolicyInvalidationHandler* invalidation_handler, - CloudPolicyStore* store, + CloudPolicyCore* core, const scoped_refptr<base::SequencedTaskRunner>& task_runner) - : invalidation_handler_(invalidation_handler), - store_(store), + : state_(UNINITIALIZED), + core_(core), task_runner_(task_runner), - profile_(NULL), invalidation_service_(NULL), invalidations_enabled_(false), invalidation_service_enabled_(false), @@ -44,40 +43,45 @@ CloudPolicyInvalidator::CloudPolicyInvalidator( unknown_version_invalidation_count_(0), ack_handle_(syncer::AckHandle::InvalidAckHandle()), weak_factory_(this), - max_fetch_delay_(kMaxFetchDelayDefault) { - DCHECK(invalidation_handler); - DCHECK(store); + max_fetch_delay_(kMaxFetchDelayDefault), + policy_refresh_count_(0) { + DCHECK(core); DCHECK(task_runner.get()); - DCHECK(!IsInitialized()); } -CloudPolicyInvalidator::~CloudPolicyInvalidator() {} - -void CloudPolicyInvalidator::InitializeWithProfile(Profile* profile) { - DCHECK(!IsInitialized()); - DCHECK(profile); - profile_ = profile; - Initialize(); +CloudPolicyInvalidator::~CloudPolicyInvalidator() { + DCHECK(state_ == SHUT_DOWN); } -void CloudPolicyInvalidator::InitializeWithService( +void CloudPolicyInvalidator::Initialize( invalidation::InvalidationService* invalidation_service) { - DCHECK(!IsInitialized()); + DCHECK(state_ == UNINITIALIZED); + DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(invalidation_service); invalidation_service_ = invalidation_service; - Initialize(); + state_ = STOPPED; + core_->AddObserver(this); + if (core_->refresh_scheduler()) + OnRefreshSchedulerStarted(core_); } void CloudPolicyInvalidator::Shutdown() { - if (IsInitialized()) { + DCHECK(state_ != SHUT_DOWN); + DCHECK(thread_checker_.CalledOnValidThread()); + if (state_ == STARTED) { if (registered_timestamp_) invalidation_service_->UnregisterInvalidationHandler(this); - store_->RemoveObserver(this); + core_->store()->RemoveObserver(this); + weak_factory_.InvalidateWeakPtrs(); } + if (state_ != UNINITIALIZED) + core_->RemoveObserver(this); + state_ = SHUT_DOWN; } void CloudPolicyInvalidator::OnInvalidatorStateChange( syncer::InvalidatorState state) { + DCHECK(state_ == STARTED); DCHECK(thread_checker_.CalledOnValidThread()); invalidation_service_enabled_ = state == syncer::INVALIDATIONS_ENABLED; UpdateInvalidationsEnabled(); @@ -85,6 +89,7 @@ void CloudPolicyInvalidator::OnInvalidatorStateChange( void CloudPolicyInvalidator::OnIncomingInvalidation( const syncer::ObjectIdInvalidationMap& invalidation_map) { + DCHECK(state_ == STARTED); DCHECK(thread_checker_.CalledOnValidThread()); const syncer::ObjectIdInvalidationMap::const_iterator invalidation = invalidation_map.find(object_id_); @@ -95,8 +100,28 @@ void CloudPolicyInvalidator::OnIncomingInvalidation( HandleInvalidation(invalidation->second); } +void CloudPolicyInvalidator::OnCoreConnected(CloudPolicyCore* core) {} + +void CloudPolicyInvalidator::OnRefreshSchedulerStarted(CloudPolicyCore* core) { + DCHECK(state_ == STOPPED); + DCHECK(thread_checker_.CalledOnValidThread()); + state_ = STARTED; + OnStoreLoaded(core_->store()); + core_->store()->AddObserver(this); +} + +void CloudPolicyInvalidator::OnCoreDisconnecting(CloudPolicyCore* core) { + DCHECK(state_ == STARTED || state_ == STOPPED); + DCHECK(thread_checker_.CalledOnValidThread()); + if (state_ == STARTED) { + Unregister(); + core_->store()->RemoveObserver(this); + state_ = STOPPED; + } +} + void CloudPolicyInvalidator::OnStoreLoaded(CloudPolicyStore* store) { - DCHECK(IsInitialized()); + DCHECK(state_ == STARTED); DCHECK(thread_checker_.CalledOnValidThread()); if (registered_timestamp_) { // Update the kMetricPolicyRefresh histogram. In some cases, this object can @@ -124,21 +149,6 @@ void CloudPolicyInvalidator::OnStoreLoaded(CloudPolicyStore* store) { void CloudPolicyInvalidator::OnStoreError(CloudPolicyStore* store) {} -base::WeakPtr<CloudPolicyInvalidator> CloudPolicyInvalidator::GetWeakPtr() { - DCHECK(!IsInitialized()); - return weak_factory_.GetWeakPtr(); -} - -void CloudPolicyInvalidator::Initialize() { - OnStoreLoaded(store_); - store_->AddObserver(this); -} - -bool CloudPolicyInvalidator::IsInitialized() { - // Could have been initialized with a profile or invalidation service. - return profile_ || invalidation_service_; -} - void CloudPolicyInvalidator::HandleInvalidation( const syncer::Invalidation& invalidation) { // The invalidation service may send an invalidation more than once if there @@ -171,20 +181,20 @@ void CloudPolicyInvalidator::HandleInvalidation( base::TimeDelta delay = base::TimeDelta::FromMilliseconds( base::RandInt(20, max_fetch_delay_)); - // If there is a payload, the invalidate callback can run at any time, so set - // the version and payload on the client immediately. Otherwise, the callback + // If there is a payload, the policy can be refreshed at any time, so set + // the version and payload on the client immediately. Otherwise, the refresh // must only run after at least kMissingPayloadDelay minutes. const std::string& payload = invalidation.payload; if (!invalidation.payload.empty()) - invalidation_handler_->SetInvalidationInfo(invalidation_version_, payload); + core_->client()->SetInvalidationInfo(invalidation_version_, payload); else delay += base::TimeDelta::FromMinutes(kMissingPayloadDelay); - // Schedule the invalidate callback to run. + // Schedule the policy to be refreshed. task_runner_->PostDelayedTask( FROM_HERE, base::Bind( - &CloudPolicyInvalidator::RunInvalidateCallback, + &CloudPolicyInvalidator::RefreshPolicy, weak_factory_.GetWeakPtr(), payload.empty() /* is_missing_payload */), delay); @@ -217,15 +227,6 @@ void CloudPolicyInvalidator::UpdateRegistration( void CloudPolicyInvalidator::Register( int64 timestamp, const invalidation::ObjectId& object_id) { - // Get the invalidation service from the profile if needed. - if (!invalidation_service_) { - DCHECK(profile_); - invalidation_service_ = - invalidation::InvalidationServiceFactory::GetForProfile(profile_); - if (!invalidation_service_) - return; - } - // Register this handler with the invalidation service if needed. if (!registered_timestamp_) { OnInvalidatorStateChange(invalidation_service_->GetInvalidatorState()); @@ -295,33 +296,32 @@ void CloudPolicyInvalidator::UpdateInvalidationsEnabled() { invalidation_service_enabled_ && registered_timestamp_; if (invalidations_enabled_ != invalidations_enabled) { invalidations_enabled_ = invalidations_enabled; - invalidation_handler_->OnInvalidatorStateChanged(invalidations_enabled); + core_->refresh_scheduler()->SetInvalidationServiceAvailability( + invalidations_enabled); } } -void CloudPolicyInvalidator::RunInvalidateCallback(bool is_missing_payload) { +void CloudPolicyInvalidator::RefreshPolicy(bool is_missing_payload) { DCHECK(thread_checker_.CalledOnValidThread()); // In the missing payload case, the invalidation version has not been set on // the client yet, so set it now that the required time has elapsed. - if (is_missing_payload) { - invalidation_handler_->SetInvalidationInfo( - invalidation_version_, - std::string()); - } - invalidation_handler_->InvalidatePolicy(); + if (is_missing_payload) + core_->client()->SetInvalidationInfo(invalidation_version_, std::string()); + core_->refresh_scheduler()->RefreshSoon(); + policy_refresh_count_++; } void CloudPolicyInvalidator::AcknowledgeInvalidation() { DCHECK(invalid_); invalid_ = false; - invalidation_handler_->SetInvalidationInfo(0, std::string()); + core_->client()->SetInvalidationInfo(0, std::string()); invalidation_service_->AcknowledgeInvalidation(object_id_, ack_handle_); - // Cancel any scheduled invalidate callbacks. + // Cancel any scheduled policy refreshes. weak_factory_.InvalidateWeakPtrs(); } int CloudPolicyInvalidator::GetPolicyRefreshMetric() { - if (store_->policy_changed()) { + if (core_->store()->policy_changed()) { if (invalid_) return METRIC_POLICY_REFRESH_INVALIDATED_CHANGED; if (invalidations_enabled_) diff --git a/chrome/browser/policy/cloud/cloud_policy_invalidator.h b/chrome/browser/policy/cloud/cloud_policy_invalidator.h index 2575d1b..6ce9aa9 100644 --- a/chrome/browser/policy/cloud/cloud_policy_invalidator.h +++ b/chrome/browser/policy/cloud/cloud_policy_invalidator.h @@ -12,11 +12,10 @@ #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" #include "base/threading/thread_checker.h" +#include "chrome/browser/policy/cloud/cloud_policy_core.h" #include "chrome/browser/policy/cloud/cloud_policy_store.h" #include "sync/notifier/invalidation_handler.h" -class Profile; - namespace base { class SequencedTaskRunner; } @@ -27,10 +26,9 @@ class InvalidationService; namespace policy { -class CloudPolicyInvalidationHandler; - // Listens for and provides policy invalidations. class CloudPolicyInvalidator : public syncer::InvalidationHandler, + public CloudPolicyCore::Observer, public CloudPolicyStore::Observer { public: // The number of minutes to delay a policy refresh after receiving an @@ -42,29 +40,20 @@ class CloudPolicyInvalidator : public syncer::InvalidationHandler, static const int kMaxFetchDelayMin; static const int kMaxFetchDelayMax; - // |invalidation_handler| handles invalidations provided by this object and - // must remain valid until Shutdown is called. - // |store| is cloud policy store. It must remain valid until Shutdown is - // called. + // |core| is the cloud policy core which connects the various policy objects. + // It must remain valid until Shutdown is called. // |task_runner| is used for scheduling delayed tasks. It must post tasks to // the main policy thread. CloudPolicyInvalidator( - CloudPolicyInvalidationHandler* invalidation_handler, - CloudPolicyStore* store, + CloudPolicyCore* core, const scoped_refptr<base::SequencedTaskRunner>& task_runner); virtual ~CloudPolicyInvalidator(); - // Initializes the invalidator with the given profile. The invalidator uses - // the profile to get a reference to the profile's invalidation service if - // needed. Both the profile and the invalidation service must remain valid - // until Shutdown is called. An Initialize method must only be called once. - void InitializeWithProfile(Profile* profile); - - // Initializes the invalidator with the invalidation service. It must remain - // valid until Shutdown is called. An Initialize method must only be called - // once. - void InitializeWithService( - invalidation::InvalidationService* invalidation_service); + // Initializes the invalidator. No invalidations will be generated before this + // method is called. This method must only be called once. + // |invalidation_service| is the invalidation service to use and must remain + // valid until Shutdown is called. + void Initialize(invalidation::InvalidationService* invalidation_service); // Shuts down and disables invalidations. It must be called before the object // is destroyed. @@ -81,23 +70,22 @@ class CloudPolicyInvalidator : public syncer::InvalidationHandler, virtual void OnIncomingInvalidation( const syncer::ObjectIdInvalidationMap& invalidation_map) OVERRIDE; + // CloudPolicyCore::Observer: + virtual void OnCoreConnected(CloudPolicyCore* core) OVERRIDE; + virtual void OnRefreshSchedulerStarted(CloudPolicyCore* core) OVERRIDE; + virtual void OnCoreDisconnecting(CloudPolicyCore* core) OVERRIDE; + // CloudPolicyStore::Observer: virtual void OnStoreLoaded(CloudPolicyStore* store) OVERRIDE; virtual void OnStoreError(CloudPolicyStore* store) OVERRIDE; - protected: - // Allows subclasses to create a weak pointer to the object. The pointer - // should only be used to call one of the Initialize methods, as after the - // object is initialized weak pointers may be invalidated at any time. - base::WeakPtr<CloudPolicyInvalidator> GetWeakPtr(); + // Expose the number of times the invalidator has refreshed the policy. For + // testing only. + int policy_refresh_count() { + return policy_refresh_count_; + } private: - // Initialize the invalidator. - void Initialize(); - - // Returns whether an Initialize method has been called. - bool IsInitialized(); - // Handle an invalidation to the policy. void HandleInvalidation(const syncer::Invalidation& invalidation); @@ -119,10 +107,10 @@ class CloudPolicyInvalidator : public syncer::InvalidationHandler, // value changed. void UpdateInvalidationsEnabled(); - // Run the invalidate callback on the invalidation handler. is_missing_payload - // is set to true if the callback is being invoked in response to an - // invalidation with a missing payload. - void RunInvalidateCallback(bool is_missing_payload); + // Refresh the policy. + // |is_missing_payload| is set to true if the callback is being invoked in + // response to an invalidation with a missing payload. + void RefreshPolicy(bool is_missing_payload); // Acknowledge the latest invalidation. void AcknowledgeInvalidation(); @@ -131,19 +119,21 @@ class CloudPolicyInvalidator : public syncer::InvalidationHandler, // when a policy is stored. int GetPolicyRefreshMetric(); - // The handler for invalidations provded by this object. - CloudPolicyInvalidationHandler* invalidation_handler_; + // The state of the object. + enum State { + UNINITIALIZED, + STOPPED, + STARTED, + SHUT_DOWN + }; + State state_; - // The cloud policy store. - CloudPolicyStore* store_; + // The cloud policy core. + CloudPolicyCore* core_; // Schedules delayed tasks. const scoped_refptr<base::SequencedTaskRunner> task_runner_; - // The profile which will be used to get a reference to an invalidation - // service. - Profile* profile_; - // The invalidation service. invalidation::InvalidationService* invalidation_service_; @@ -191,29 +181,10 @@ class CloudPolicyInvalidator : public syncer::InvalidationHandler, // thread. base::ThreadChecker thread_checker_; - DISALLOW_COPY_AND_ASSIGN(CloudPolicyInvalidator); -}; + // Policy refresh counter used for testing. + int policy_refresh_count_; -// Handles invalidations to cloud policy objects. -class CloudPolicyInvalidationHandler { - public: - virtual ~CloudPolicyInvalidationHandler() {} - - // This method is called when the current invalidation info should be set - // on the cloud policy client. - virtual void SetInvalidationInfo( - int64 version, - const std::string& payload) = 0; - - // This method is called when the policy should be refreshed due to an - // invalidation. A policy fetch should be scheduled in the near future. - virtual void InvalidatePolicy() = 0; - - // This method is called when the invalidator determines that the ability to - // receive policy invalidations becomes enabled or disabled. The invalidator - // starts in a disabled state, so the first call to this method is always when - // the invalidator becomes enabled. - virtual void OnInvalidatorStateChanged(bool invalidations_enabled) = 0; + DISALLOW_COPY_AND_ASSIGN(CloudPolicyInvalidator); }; } // namespace policy diff --git a/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc b/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc index eed8bfc..a621977 100644 --- a/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc +++ b/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc @@ -5,8 +5,10 @@ #include <string> #include "base/basictypes.h" +#include "base/bind.h" #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" +#include "base/message_loop/message_loop.h" #include "base/metrics/histogram.h" #include "base/metrics/histogram_samples.h" #include "base/metrics/sample_map.h" @@ -15,9 +17,10 @@ #include "base/time/time.h" #include "base/values.h" #include "chrome/browser/invalidation/fake_invalidation_service.h" +#include "chrome/browser/policy/cloud/cloud_policy_constants.h" #include "chrome/browser/policy/cloud/cloud_policy_core.h" #include "chrome/browser/policy/cloud/cloud_policy_invalidator.h" -#include "chrome/browser/policy/cloud/cloud_policy_service.h" +#include "chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h" #include "chrome/browser/policy/cloud/enterprise_metrics.h" #include "chrome/browser/policy/cloud/mock_cloud_policy_client.h" #include "chrome/browser/policy/cloud/mock_cloud_policy_store.h" @@ -30,8 +33,7 @@ namespace policy { -class CloudPolicyInvalidatorTest : public testing::Test, - public CloudPolicyInvalidationHandler { +class CloudPolicyInvalidatorTest : public testing::Test { protected: // Policy objects which can be used in tests. enum PolicyObject { @@ -47,11 +49,32 @@ class CloudPolicyInvalidatorTest : public testing::Test, virtual void TearDown() OVERRIDE; // Starts the invalidator which will be tested. - void StartInvalidator(bool initialize); + // |initialize| determines if the invalidator should be initialized. + // |start_refresh_scheduler| determines if the refresh scheduler should start. + void StartInvalidator(bool initialize, bool start_refresh_scheduler); void StartInvalidator() { - StartInvalidator(true /* initialize */); + StartInvalidator(true /* initialize */, true /* start_refresh_scheduler */); } + // Calls Initialize on the invalidator. + void InitializeInvalidator(); + + // Calls Shutdown on the invalidator. Test must call DestroyInvalidator + // afterwards to prevent Shutdown from being called twice. + void ShutdownInvalidator(); + + // Destroys the invalidator. + void DestroyInvalidator(); + + // Connects the cloud policy core. + void ConnectCore(); + + // Starts the refresh scheduler. + void StartRefreshScheduler(); + + // Disconnects the cloud policy core. + void DisconnectCore(); + // Simulates storing a new policy to the policy store. // |object| determines which policy object the store will report the // invalidator should register for. May be POLICY_OBJECT_NONE for no object. @@ -98,40 +121,36 @@ class CloudPolicyInvalidatorTest : public testing::Test, // Checks the expected value of the currently set invalidation info. bool CheckInvalidationInfo(int64 version, const std::string& payload); - // Checks that the invalidate callback was not called. - bool CheckInvalidateNotCalled(); + // Checks that the policy was not refreshed due to an invalidation. + bool CheckPolicyNotRefreshed(); - // Checks that the invalidate callback was called within an appropriate - // timeframe depending on whether the invalidation had unknown version. - bool CheckInvalidateCalled(bool unknown_version); - bool CheckInvalidateCalled() { - return CheckInvalidateCalled(true); - } - - // Checks that the state changed callback of the invalidation handler was not - // called. - bool CheckStateChangedNotCalled(); + // Checks that the policy was refreshed due to an invalidation within an + // appropriate timeframe depending on whether the invalidation had unknown + // version. + bool CheckPolicyRefreshed(); + bool CheckPolicyRefreshedWithUnknownVersion(); - // Checks that the state changed callback of the invalidation handler was - // called with the given state. - bool CheckStateChangedCalled(bool invalidations_enabled); + // Returns the invalidations enabled state set by the invalidator on the + // refresh scheduler. + bool InvalidationsEnabled(); // Determines if the invalidation with the given ack handle has been // acknowledged. bool IsInvalidationAcknowledged(const syncer::AckHandle& ack_handle); + // Determines if the invalidator has registered for an object with the + // invalidation service. + bool IsInvalidatorRegistered(); + // Get the current count for the given metric. base::HistogramBase::Count GetCount(MetricPolicyRefresh metric); base::HistogramBase::Count GetInvalidationCount(bool with_payload); - // CloudPolicyInvalidationHandler: - virtual void SetInvalidationInfo( - int64 version, - const std::string& payload) OVERRIDE; - virtual void InvalidatePolicy() OVERRIDE; - virtual void OnInvalidatorStateChanged(bool invalidations_enabled) OVERRIDE; - private: + // Checks that the policy was refreshed due to an invalidation with the given + // base delay. + bool CheckPolicyRefreshed(base::TimeDelta delay); + // Returns the object id of the given policy object. const invalidation::ObjectId& GetPolicyObjectId(PolicyObject object) const; @@ -142,15 +161,12 @@ class CloudPolicyInvalidatorTest : public testing::Test, // Objects the invalidator depends on. invalidation::FakeInvalidationService invalidation_service_; MockCloudPolicyStore store_; + CloudPolicyCore core_; scoped_refptr<base::TestSimpleTaskRunner> task_runner_; // The invalidator which will be tested. scoped_ptr<CloudPolicyInvalidator> invalidator_; - // The latest invalidation info set by the invalidator. - int64 invalidation_version_; - std::string invalidation_payload_; - // Object ids for the test policy objects. invalidation::ObjectId object_id_a_; invalidation::ObjectId object_id_b_; @@ -166,32 +182,27 @@ class CloudPolicyInvalidatorTest : public testing::Test, // The currently used policy value. const char* policy_value_cur_; - // Stores how many times the invalidate callback was called. - int invalidate_callback_count_; - - // Stores how many times the state change callback was called for each state. - int state_change_enabled_callback_count_; - int state_change_disabled_callback_count_; - // Stores starting histogram counts for kMetricPolicyRefresh. scoped_ptr<base::HistogramSamples> refresh_samples_; // Stores starting histogram counts for kMetricPolicyInvalidations. scoped_ptr<base::HistogramSamples> invalidations_samples_; + + // Initialize message loop. + base::MessageLoop loop_; }; CloudPolicyInvalidatorTest::CloudPolicyInvalidatorTest() - : task_runner_(new base::TestSimpleTaskRunner()), - invalidation_version_(0), + : core_(PolicyNamespaceKey(dm_protocol::kChromeUserPolicyType, + std::string()), + &store_), + task_runner_(new base::TestSimpleTaskRunner()), object_id_a_(135, "asdf"), object_id_b_(246, "zxcv"), timestamp_(123456), policy_value_a_("asdf"), policy_value_b_("zxcv"), - policy_value_cur_(policy_value_a_), - invalidate_callback_count_(0), - state_change_enabled_callback_count_(0), - state_change_disabled_callback_count_(0) {} + policy_value_cur_(policy_value_a_) {} void CloudPolicyInvalidatorTest::SetUp() { base::StatisticsRecorder::Initialize(); @@ -203,15 +214,43 @@ void CloudPolicyInvalidatorTest::TearDown() { EXPECT_FALSE(invalidation_service_.ReceivedInvalidAcknowledgement()); if (invalidator_) invalidator_->Shutdown(); + core_.Disconnect(); } -void CloudPolicyInvalidatorTest::StartInvalidator(bool initialize) { - invalidator_.reset(new CloudPolicyInvalidator( - this /* invalidation_handler */, - &store_, - task_runner_)); +void CloudPolicyInvalidatorTest::StartInvalidator( + bool initialize, + bool start_refresh_scheduler) { + invalidator_.reset(new CloudPolicyInvalidator(&core_, task_runner_)); + if (start_refresh_scheduler) { + ConnectCore(); + StartRefreshScheduler(); + } if (initialize) - invalidator_->InitializeWithService(&invalidation_service_); + InitializeInvalidator(); +} + +void CloudPolicyInvalidatorTest::InitializeInvalidator() { + invalidator_->Initialize(&invalidation_service_); +} + +void CloudPolicyInvalidatorTest::ShutdownInvalidator() { + invalidator_->Shutdown(); +} + +void CloudPolicyInvalidatorTest::DestroyInvalidator() { + invalidator_.reset(); +} + +void CloudPolicyInvalidatorTest::ConnectCore() { + core_.Connect(scoped_ptr<CloudPolicyClient>(new MockCloudPolicyClient())); +} + +void CloudPolicyInvalidatorTest::StartRefreshScheduler() { + core_.StartRefreshScheduler(); +} + +void CloudPolicyInvalidatorTest::DisconnectCore() { + core_.Disconnect(); } void CloudPolicyInvalidatorTest::StorePolicy( @@ -274,60 +313,29 @@ syncer::AckHandle CloudPolicyInvalidatorTest::FireInvalidation( bool CloudPolicyInvalidatorTest::CheckInvalidationInfo( int64 version, const std::string& payload) { - return version == invalidation_version_ && payload == invalidation_payload_; + MockCloudPolicyClient* client = + static_cast<MockCloudPolicyClient*>(core_.client()); + return version == client->invalidation_version_ && + payload == client->invalidation_payload_; } -bool CloudPolicyInvalidatorTest::CheckInvalidateNotCalled() { - bool result = true; - if (invalidate_callback_count_ != 0) - result = false; +bool CloudPolicyInvalidatorTest::CheckPolicyNotRefreshed() { + const int start_count = invalidator_->policy_refresh_count(); task_runner_->RunUntilIdle(); - if (invalidate_callback_count_ != 0) - result = false; - return result; + return invalidator_->policy_refresh_count() == start_count; } -bool CloudPolicyInvalidatorTest::CheckInvalidateCalled(bool unknown_version) { - base::TimeDelta min_delay; - base::TimeDelta max_delay = base::TimeDelta::FromMilliseconds( - CloudPolicyInvalidator::kMaxFetchDelayMin); - if (unknown_version) { - base::TimeDelta additional_delay = base::TimeDelta::FromMinutes( - CloudPolicyInvalidator::kMissingPayloadDelay); - min_delay += additional_delay; - max_delay += additional_delay; - } - - if (task_runner_->GetPendingTasks().empty()) - return false; - base::TimeDelta actual_delay = task_runner_->GetPendingTasks().back().delay; - EXPECT_GE(actual_delay, min_delay); - EXPECT_LE(actual_delay, max_delay); - - bool result = true; - if (invalidate_callback_count_ != 0) - result = false; - task_runner_->RunUntilIdle(); - if (invalidate_callback_count_ != 1) - result = false; - invalidate_callback_count_ = 0; - return result; +bool CloudPolicyInvalidatorTest::CheckPolicyRefreshed() { + return CheckPolicyRefreshed(base::TimeDelta()); } -bool CloudPolicyInvalidatorTest::CheckStateChangedNotCalled() { - return state_change_enabled_callback_count_ == 0 && - state_change_disabled_callback_count_ == 0; +bool CloudPolicyInvalidatorTest::CheckPolicyRefreshedWithUnknownVersion() { + return CheckPolicyRefreshed(base::TimeDelta::FromMinutes( + CloudPolicyInvalidator::kMissingPayloadDelay)); } -bool CloudPolicyInvalidatorTest::CheckStateChangedCalled( - bool invalidations_enabled) { - int expected_enabled_count_ = invalidations_enabled ? 1 : 0; - int expected_disabled_count_ = invalidations_enabled ? 0 : 1; - bool result = state_change_enabled_callback_count_ == expected_enabled_count_ - && state_change_disabled_callback_count_ == expected_disabled_count_; - state_change_enabled_callback_count_ = 0; - state_change_disabled_callback_count_ = 0; - return result; +bool CloudPolicyInvalidatorTest::InvalidationsEnabled() { + return core_.refresh_scheduler()->invalidations_available(); } bool CloudPolicyInvalidatorTest::IsInvalidationAcknowledged( @@ -335,6 +343,11 @@ bool CloudPolicyInvalidatorTest::IsInvalidationAcknowledged( return invalidation_service_.IsInvalidationAcknowledged(ack_handle); } +bool CloudPolicyInvalidatorTest::IsInvalidatorRegistered() { + return !invalidation_service_.invalidator_registrar() + .GetRegisteredIds(invalidator_.get()).empty(); +} + base::HistogramBase::Count CloudPolicyInvalidatorTest::GetCount( MetricPolicyRefresh metric) { return GetHistogramSamples(kMetricPolicyRefresh)->GetCount(metric) - @@ -348,25 +361,19 @@ base::HistogramBase::Count CloudPolicyInvalidatorTest::GetInvalidationCount( invalidations_samples_->GetCount(metric); } -void CloudPolicyInvalidatorTest::SetInvalidationInfo( - int64 version, - const std::string& payload) { - invalidation_version_ = version; - invalidation_payload_ = payload; -} +bool CloudPolicyInvalidatorTest::CheckPolicyRefreshed(base::TimeDelta delay) { + base::TimeDelta max_delay = delay + base::TimeDelta::FromMilliseconds( + CloudPolicyInvalidator::kMaxFetchDelayMin); -void CloudPolicyInvalidatorTest::InvalidatePolicy() { - ++invalidate_callback_count_; -} + if (task_runner_->GetPendingTasks().empty()) + return false; + base::TimeDelta actual_delay = task_runner_->GetPendingTasks().back().delay; + EXPECT_GE(actual_delay, delay); + EXPECT_LE(actual_delay, max_delay); -void CloudPolicyInvalidatorTest::OnInvalidatorStateChanged( - bool invalidations_enabled) { - if (invalidator_.get()) - EXPECT_EQ(invalidations_enabled, invalidator_->invalidations_enabled()); - if (invalidations_enabled) - ++state_change_enabled_callback_count_; - else - ++state_change_disabled_callback_count_; + const int start_count = invalidator_->policy_refresh_count(); + task_runner_->RunUntilIdle(); + return invalidator_->policy_refresh_count() == start_count + 1; } const invalidation::ObjectId& CloudPolicyInvalidatorTest::GetPolicyObjectId( @@ -386,98 +393,144 @@ scoped_ptr<base::HistogramSamples> } TEST_F(CloudPolicyInvalidatorTest, Uninitialized) { - // No invalidations should be processed if the invalidator is not intialized. - StartInvalidator(false /* initialize */); + // No invalidations should be processed if the invalidator is not initialized. + StartInvalidator(false /* initialize */, true /* start_refresh_scheduler */); StorePolicy(POLICY_OBJECT_A); + EXPECT_FALSE(IsInvalidatorRegistered()); FireInvalidation(POLICY_OBJECT_A); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); +} + +TEST_F(CloudPolicyInvalidatorTest, RefreshSchedulerNotStarted) { + // No invalidations should be processed if the refresh scheduler is not + // started. + StartInvalidator(true /* initialize */, false /* start_refresh_scheduler */); + StorePolicy(POLICY_OBJECT_A); + EXPECT_FALSE(IsInvalidatorRegistered()); + FireInvalidation(POLICY_OBJECT_A); + EXPECT_TRUE(CheckPolicyNotRefreshed()); +} + +TEST_F(CloudPolicyInvalidatorTest, DisconnectCoreThenInitialize) { + // No invalidations should be processed if the core is disconnected before + // initialization. + StartInvalidator(false /* initialize */, true /* start_refresh_scheduler */); + DisconnectCore(); + InitializeInvalidator(); + StorePolicy(POLICY_OBJECT_A); + EXPECT_FALSE(IsInvalidatorRegistered()); + FireInvalidation(POLICY_OBJECT_A); + EXPECT_TRUE(CheckPolicyNotRefreshed()); +} + +TEST_F(CloudPolicyInvalidatorTest, InitializeThenStartRefreshScheduler) { + // Make sure registration occurs and invalidations are processed when + // Initialize is called before starting the refresh scheduler. + // Note that the reverse case (start refresh scheduler then initialize) is + // the default behavior for the test fixture, so will be tested in most other + // tests. + StartInvalidator(true /* initialize */, false /* start_refresh_scheduler */); + ConnectCore(); + StartRefreshScheduler(); + StorePolicy(POLICY_OBJECT_A); + EXPECT_TRUE(IsInvalidatorRegistered()); + FireInvalidation(POLICY_OBJECT_A); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); } TEST_F(CloudPolicyInvalidatorTest, RegisterOnStoreLoaded) { // No registration when store is not loaded. StartInvalidator(); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_FALSE(IsInvalidatorRegistered()); + EXPECT_FALSE(InvalidationsEnabled()); FireInvalidation(POLICY_OBJECT_A); FireInvalidation(POLICY_OBJECT_B); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); // No registration when store is loaded with no invalidation object id. StorePolicy(POLICY_OBJECT_NONE); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_FALSE(IsInvalidatorRegistered()); + EXPECT_FALSE(InvalidationsEnabled()); FireInvalidation(POLICY_OBJECT_A); FireInvalidation(POLICY_OBJECT_B); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); // Check registration when store is loaded for object A. StorePolicy(POLICY_OBJECT_A); - EXPECT_TRUE(CheckStateChangedCalled(true)); + EXPECT_TRUE(IsInvalidatorRegistered()); + EXPECT_TRUE(InvalidationsEnabled()); FireInvalidation(POLICY_OBJECT_A); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); FireInvalidation(POLICY_OBJECT_B); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); } TEST_F(CloudPolicyInvalidatorTest, ChangeRegistration) { // Register for object A. StartInvalidator(); StorePolicy(POLICY_OBJECT_A); - EXPECT_TRUE(CheckStateChangedCalled(true)); + EXPECT_TRUE(IsInvalidatorRegistered()); + EXPECT_TRUE(InvalidationsEnabled()); FireInvalidation(POLICY_OBJECT_A); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); FireInvalidation(POLICY_OBJECT_B); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); syncer::AckHandle ack = FireInvalidation(POLICY_OBJECT_A); // Check re-registration for object B. Make sure the pending invalidation for // object A is acknowledged without making the callback. StorePolicy(POLICY_OBJECT_B); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_TRUE(IsInvalidatorRegistered()); + EXPECT_TRUE(InvalidationsEnabled()); EXPECT_TRUE(IsInvalidationAcknowledged(ack)); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); // Make sure future invalidations for object A are ignored and for object B // are processed. FireInvalidation(POLICY_OBJECT_A); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); FireInvalidation(POLICY_OBJECT_B); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); } TEST_F(CloudPolicyInvalidatorTest, UnregisterOnStoreLoaded) { // Register for object A. StartInvalidator(); StorePolicy(POLICY_OBJECT_A); - EXPECT_TRUE(CheckStateChangedCalled(true)); + EXPECT_TRUE(IsInvalidatorRegistered()); + EXPECT_TRUE(InvalidationsEnabled()); FireInvalidation(POLICY_OBJECT_A); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); // Check unregistration when store is loaded with no invalidation object id. syncer::AckHandle ack = FireInvalidation(POLICY_OBJECT_A); EXPECT_FALSE(IsInvalidationAcknowledged(ack)); StorePolicy(POLICY_OBJECT_NONE); + EXPECT_FALSE(IsInvalidatorRegistered()); EXPECT_TRUE(IsInvalidationAcknowledged(ack)); - EXPECT_TRUE(CheckStateChangedCalled(false)); + EXPECT_FALSE(InvalidationsEnabled()); FireInvalidation(POLICY_OBJECT_A); FireInvalidation(POLICY_OBJECT_B); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); // Check re-registration for object B. StorePolicy(POLICY_OBJECT_B); - EXPECT_TRUE(CheckStateChangedCalled(true)); + EXPECT_TRUE(IsInvalidatorRegistered()); + EXPECT_TRUE(InvalidationsEnabled()); FireInvalidation(POLICY_OBJECT_B); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); } TEST_F(CloudPolicyInvalidatorTest, HandleInvalidation) { // Register and fire invalidation StorePolicy(POLICY_OBJECT_A); StartInvalidator(); - EXPECT_TRUE(CheckStateChangedCalled(true)); + EXPECT_TRUE(InvalidationsEnabled()); syncer::AckHandle ack = FireInvalidation(POLICY_OBJECT_A, 12, "test_payload"); // Make sure client info is set as soon as the invalidation is received. EXPECT_TRUE(CheckInvalidationInfo(12, "test_payload")); - EXPECT_TRUE(CheckInvalidateCalled(false /* unknown_version */)); + EXPECT_TRUE(CheckPolicyRefreshed()); // Make sure invalidation is not acknowledged until the store is loaded. EXPECT_FALSE(IsInvalidationAcknowledged(ack)); @@ -496,7 +549,7 @@ TEST_F(CloudPolicyInvalidatorTest, HandleInvalidationWithUnknownVersion) { // Make sure client info is not set until after the invalidation callback is // made. EXPECT_TRUE(CheckInvalidationInfo(0, std::string())); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); EXPECT_TRUE(CheckInvalidationInfo(-1, std::string())); // Make sure invalidation is not acknowledged until the store is loaded. @@ -521,8 +574,8 @@ TEST_F(CloudPolicyInvalidatorTest, HandleMultipleInvalidations) { EXPECT_TRUE(IsInvalidationAcknowledged(ack1)); EXPECT_TRUE(IsInvalidationAcknowledged(ack2)); - // Make sure the invalidate callback is called once. - EXPECT_TRUE(CheckInvalidateCalled(false /* unknown_version */)); + // Make sure the policy is refreshed once. + EXPECT_TRUE(CheckPolicyRefreshed()); // Make sure that the last invalidation is only acknowledged after the store // is loaded with the latest version. @@ -542,15 +595,15 @@ TEST_F(CloudPolicyInvalidatorTest, StartInvalidator(); syncer::AckHandle ack1 = FireInvalidation(POLICY_OBJECT_A); EXPECT_TRUE(CheckInvalidationInfo(0, std::string())); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); EXPECT_TRUE(CheckInvalidationInfo(-1, std::string())); syncer::AckHandle ack2 = FireInvalidation(POLICY_OBJECT_A); EXPECT_TRUE(CheckInvalidationInfo(0, std::string())); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); EXPECT_TRUE(CheckInvalidationInfo(-2, std::string())); syncer::AckHandle ack3 = FireInvalidation(POLICY_OBJECT_A); EXPECT_TRUE(CheckInvalidationInfo(0, std::string())); - EXPECT_TRUE(CheckInvalidateCalled()); + EXPECT_TRUE(CheckPolicyRefreshedWithUnknownVersion()); EXPECT_TRUE(CheckInvalidationInfo(-3, std::string())); // Make sure the replaced invalidations are acknowledged. @@ -567,59 +620,107 @@ TEST_F(CloudPolicyInvalidatorTest, EXPECT_TRUE(IsInvalidationAcknowledged(ack3)); } -TEST_F(CloudPolicyInvalidatorTest, AcknowledgeBeforeInvalidateCallback) { +TEST_F(CloudPolicyInvalidatorTest, AcknowledgeBeforeRefresh) { // Generate an invalidation. StorePolicy(POLICY_OBJECT_A); StartInvalidator(); syncer::AckHandle ack = FireInvalidation(POLICY_OBJECT_A, 3, "test"); - // Ensure that the invalidate callback is not made and the invalidation is + // Ensure that the policy is not refreshed and the invalidation is // acknowledged if the store is loaded with the latest version before the - // callback is invoked. + // refresh can occur. StorePolicy(POLICY_OBJECT_A, 3); EXPECT_TRUE(IsInvalidationAcknowledged(ack)); - EXPECT_TRUE(CheckInvalidateNotCalled()); + EXPECT_TRUE(CheckPolicyNotRefreshed()); +} + +TEST_F(CloudPolicyInvalidatorTest, NoCallbackAfterShutdown) { + // Generate an invalidation. + StorePolicy(POLICY_OBJECT_A); + StartInvalidator(); + syncer::AckHandle ack = FireInvalidation(POLICY_OBJECT_A, 3, "test"); + + // Ensure that the policy refresh is not made after the invalidator is shut + // down. + ShutdownInvalidator(); + EXPECT_TRUE(CheckPolicyNotRefreshed()); + DestroyInvalidator(); } TEST_F(CloudPolicyInvalidatorTest, StateChanged) { - // Before registration, changes to the invalidation service state should not - // generate change state notifications. + // Test invalidation service state changes while not registered. StartInvalidator(); DisableInvalidationService(); EnableInvalidationService(); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_FALSE(InvalidationsEnabled()); - // After registration, changes to the invalidation service state should - // generate notifications. + // Test invalidation service state changes while registered. StorePolicy(POLICY_OBJECT_A); - EXPECT_TRUE(CheckStateChangedCalled(true)); + EXPECT_TRUE(InvalidationsEnabled()); DisableInvalidationService(); - EXPECT_TRUE(CheckStateChangedCalled(false)); + EXPECT_FALSE(InvalidationsEnabled()); DisableInvalidationService(); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_FALSE(InvalidationsEnabled()); EnableInvalidationService(); - EXPECT_TRUE(CheckStateChangedCalled(true)); + EXPECT_TRUE(InvalidationsEnabled()); EnableInvalidationService(); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_TRUE(InvalidationsEnabled()); - // When the invalidation service is enabled, changes to the registration - // state should generate notifications. + // Test registration changes with invalidation service enabled. StorePolicy(POLICY_OBJECT_NONE); - EXPECT_TRUE(CheckStateChangedCalled(false)); + EXPECT_FALSE(InvalidationsEnabled()); StorePolicy(POLICY_OBJECT_NONE); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_FALSE(InvalidationsEnabled()); StorePolicy(POLICY_OBJECT_A); - EXPECT_TRUE(CheckStateChangedCalled(true)); + EXPECT_TRUE(InvalidationsEnabled()); StorePolicy(POLICY_OBJECT_A); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_TRUE(InvalidationsEnabled()); - // When the invalidation service is disabled, changes to the registration - // state should not generate notifications. + // Test registration changes with invalidation service disabled. DisableInvalidationService(); - EXPECT_TRUE(CheckStateChangedCalled(false)); + EXPECT_FALSE(InvalidationsEnabled()); StorePolicy(POLICY_OBJECT_NONE); StorePolicy(POLICY_OBJECT_A); - EXPECT_TRUE(CheckStateChangedNotCalled()); + EXPECT_FALSE(InvalidationsEnabled()); +} + +TEST_F(CloudPolicyInvalidatorTest, Disconnect) { + // Generate an invalidation. + StorePolicy(POLICY_OBJECT_A); + StartInvalidator(); + syncer::AckHandle ack = FireInvalidation(POLICY_OBJECT_A, 1, "test"); + EXPECT_TRUE(InvalidationsEnabled()); + + // Ensure that the policy is not refreshed after disconnecting the core, but + // a call to indicate that invalidations are disabled is made. + DisconnectCore(); + EXPECT_TRUE(CheckPolicyNotRefreshed()); + + // Ensure that invalidation service events do not cause refreshes while the + // invalidator is stopped. + FireInvalidation(POLICY_OBJECT_A, 2, "test"); + EXPECT_TRUE(CheckPolicyNotRefreshed()); + DisableInvalidationService(); + EnableInvalidationService(); + + // Connect and disconnect without starting the refresh scheduler. + ConnectCore(); + FireInvalidation(POLICY_OBJECT_A, 3, "test"); + EXPECT_TRUE(CheckPolicyNotRefreshed()); + DisconnectCore(); + FireInvalidation(POLICY_OBJECT_A, 4, "test"); + EXPECT_TRUE(CheckPolicyNotRefreshed()); + + // Ensure that the invalidator returns to normal after reconnecting. + ConnectCore(); + StartRefreshScheduler(); + EXPECT_TRUE(CheckPolicyNotRefreshed()); + EXPECT_TRUE(InvalidationsEnabled()); + FireInvalidation(POLICY_OBJECT_A, 5, "test"); + EXPECT_TRUE(CheckInvalidationInfo(5, "test")); + EXPECT_TRUE(CheckPolicyRefreshed()); + DisableInvalidationService(); + EXPECT_FALSE(InvalidationsEnabled()); } TEST_F(CloudPolicyInvalidatorTest, RefreshMetricsUnregistered) { diff --git a/chrome/browser/policy/cloud/cloud_policy_manager.cc b/chrome/browser/policy/cloud/cloud_policy_manager.cc index 4c704ba..59fcc73 100644 --- a/chrome/browser/policy/cloud/cloud_policy_manager.cc +++ b/chrome/browser/policy/cloud/cloud_policy_manager.cc @@ -8,7 +8,6 @@ #include "base/bind_helpers.h" #include "base/logging.h" #include "base/prefs/pref_service.h" -#include "chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h" #include "chrome/browser/policy/cloud/cloud_policy_service.h" #include "chrome/browser/policy/policy_bundle.h" #include "chrome/browser/policy/policy_map.h" @@ -31,19 +30,6 @@ CloudPolicyManager::CloudPolicyManager(const PolicyNamespaceKey& policy_ns_key, CloudPolicyManager::~CloudPolicyManager() {} -void CloudPolicyManager::EnableInvalidations( - const base::Closure& initialize_invalidator) { - DCHECK(!initialize_invalidator.is_null()); - DCHECK(initialize_invalidator_.is_null()); - // If the refresh scheduler is already running, initialize the invalidator - // right away. Otherwise, store the closure so it can be invoked when the - // refresh scheduler starts. - if (core()->refresh_scheduler()) - initialize_invalidator.Run(); - else - initialize_invalidator_ = initialize_invalidator; -} - void CloudPolicyManager::Shutdown() { core_.Disconnect(); store()->RemoveObserver(this); @@ -80,24 +66,6 @@ void CloudPolicyManager::OnStoreError(CloudPolicyStore* cloud_policy_store) { CheckAndPublishPolicy(); } -void CloudPolicyManager::SetInvalidationInfo( - int64 version, - const std::string& payload) { - DCHECK(core()->client()); - core()->client()->SetInvalidationInfo(version, payload); -} - -void CloudPolicyManager::InvalidatePolicy() { - DCHECK(core()->refresh_scheduler()); - core()->refresh_scheduler()->RefreshSoon(); -} - -void CloudPolicyManager::OnInvalidatorStateChanged(bool invalidations_enabled) { - DCHECK(core()->refresh_scheduler()); - core()->refresh_scheduler()->SetInvalidationServiceAvailability( - invalidations_enabled); -} - void CloudPolicyManager::CheckAndPublishPolicy() { if (IsInitializationComplete(POLICY_DOMAIN_CHROME) && !waiting_for_policy_refresh_) { @@ -105,14 +73,6 @@ void CloudPolicyManager::CheckAndPublishPolicy() { } } -void CloudPolicyManager::StartRefreshScheduler() { - DCHECK(!core()->refresh_scheduler()); - core()->StartRefreshScheduler(); - // Initialize the invalidator if EnableInvalidations has been called. - if (!initialize_invalidator_.is_null()) - initialize_invalidator_.Run(); -} - scoped_ptr<PolicyBundle> CloudPolicyManager::CreatePolicyBundle() { scoped_ptr<PolicyBundle> bundle(new PolicyBundle); bundle->Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string())) diff --git a/chrome/browser/policy/cloud/cloud_policy_manager.h b/chrome/browser/policy/cloud/cloud_policy_manager.h index 6d63e87..34be652 100644 --- a/chrome/browser/policy/cloud/cloud_policy_manager.h +++ b/chrome/browser/policy/cloud/cloud_policy_manager.h @@ -8,12 +8,10 @@ #include <string> #include "base/basictypes.h" -#include "base/callback.h" #include "base/compiler_specific.h" #include "base/prefs/pref_member.h" #include "chrome/browser/policy/cloud/cloud_policy_constants.h" #include "chrome/browser/policy/cloud/cloud_policy_core.h" -#include "chrome/browser/policy/cloud/cloud_policy_invalidator.h" #include "chrome/browser/policy/cloud/cloud_policy_store.h" #include "chrome/browser/policy/configuration_policy_provider.h" @@ -29,8 +27,7 @@ class PolicyBundle; // functionality specific to user-level and device-level cloud policy, such as // blocking on initial user policy fetch or device enrollment. class CloudPolicyManager : public ConfigurationPolicyProvider, - public CloudPolicyStore::Observer, - public CloudPolicyInvalidationHandler { + public CloudPolicyStore::Observer { public: CloudPolicyManager(const PolicyNamespaceKey& policy_ns_key, CloudPolicyStore* cloud_policy_store); @@ -39,13 +36,6 @@ class CloudPolicyManager : public ConfigurationPolicyProvider, CloudPolicyCore* core() { return &core_; } const CloudPolicyCore* core() const { return &core_; } - // Enables invalidations to the cloud policy. This method must only be - // called once. - // |initialize_invalidator| should initialize the invalidator when invoked. - // If the manager is ready to receive invalidations, it will be invoked - // immediately; otherwise, it will be invoked upon becoming ready. - void EnableInvalidations(const base::Closure& initialize_invalidator); - // ConfigurationPolicyProvider: virtual void Shutdown() OVERRIDE; virtual bool IsInitializationComplete(PolicyDomain domain) const OVERRIDE; @@ -55,23 +45,11 @@ class CloudPolicyManager : public ConfigurationPolicyProvider, virtual void OnStoreLoaded(CloudPolicyStore* cloud_policy_store) OVERRIDE; virtual void OnStoreError(CloudPolicyStore* cloud_policy_store) OVERRIDE; - // CloudPolicyInvalidationHandler: - virtual void SetInvalidationInfo( - int64 version, - const std::string& payload) OVERRIDE; - virtual void InvalidatePolicy() OVERRIDE; - virtual void OnInvalidatorStateChanged(bool invalidations_enabled) OVERRIDE; - protected: // Check whether fully initialized and if so, publish policy by calling // ConfigurationPolicyStore::UpdatePolicy(). void CheckAndPublishPolicy(); - // Starts the policy refresh scheduler. This must be called instead of calling - // enabling the scheduler on core() directly so that invalidations are - // enabled correctly. - void StartRefreshScheduler(); - // Called by CheckAndPublishPolicy() to create a bundle with the current // policies. virtual scoped_ptr<PolicyBundle> CreatePolicyBundle(); @@ -94,10 +72,6 @@ class CloudPolicyManager : public ConfigurationPolicyProvider, // policy update notifications are deferred until after it completes. bool waiting_for_policy_refresh_; - // Used to initialize the policy invalidator once the refresh scheduler - // starts. - base::Closure initialize_invalidator_; - DISALLOW_COPY_AND_ASSIGN(CloudPolicyManager); }; diff --git a/chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc b/chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc index 04bfdaf..50e7c5f 100644 --- a/chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc +++ b/chrome/browser/policy/cloud/cloud_policy_manager_unittest.cc @@ -5,22 +5,17 @@ #include "chrome/browser/policy/cloud/cloud_policy_manager.h" #include "base/basictypes.h" -#include "base/bind.h" #include "base/callback.h" #include "base/compiler_specific.h" #include "base/memory/scoped_ptr.h" #include "base/message_loop/message_loop.h" -#include "base/test/test_simple_task_runner.h" -#include "chrome/browser/invalidation/fake_invalidation_service.h" #include "chrome/browser/policy/cloud/cloud_policy_constants.h" -#include "chrome/browser/policy/cloud/cloud_policy_invalidator.h" #include "chrome/browser/policy/cloud/mock_cloud_policy_client.h" #include "chrome/browser/policy/cloud/mock_cloud_policy_store.h" #include "chrome/browser/policy/cloud/policy_builder.h" #include "chrome/browser/policy/configuration_policy_provider_test.h" #include "chrome/browser/policy/external_data_fetcher.h" #include "chrome/browser/policy/mock_configuration_policy_provider.h" -#include "sync/notifier/invalidation_util.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -150,7 +145,6 @@ class TestCloudPolicyManager : public CloudPolicyManager { using CloudPolicyManager::store; using CloudPolicyManager::service; using CloudPolicyManager::CheckAndPublishPolicy; - using CloudPolicyManager::StartRefreshScheduler; private: DISALLOW_COPY_AND_ASSIGN(TestCloudPolicyManager); @@ -187,57 +181,6 @@ class CloudPolicyManagerTest : public testing::Test { manager_->Shutdown(); } - // Sets up for an invalidations test. - void CreateInvalidator() { - // Add the invalidation registration info to the policy data. - em::PolicyData* policy_data = new em::PolicyData(policy_.policy_data()); - policy_data->set_invalidation_source(12345); - policy_data->set_invalidation_name("12345"); - store_.policy_.reset(policy_data); - - // Connect the core. - MockCloudPolicyClient* client = new MockCloudPolicyClient(); - EXPECT_CALL(*client, SetupRegistration(_, _)); - manager_->core()->Connect(scoped_ptr<CloudPolicyClient>(client)); - - // Create invalidation objects. - task_runner_ = new base::TestSimpleTaskRunner(); - invalidation_service_.reset(new invalidation::FakeInvalidationService()); - invalidator_.reset(new CloudPolicyInvalidator( - manager_.get(), - manager_->core()->store(), - task_runner_)); - } - - void ShutdownInvalidator() { - invalidator_->Shutdown(); - } - - // Call EnableInvalidations on the manager. - void EnableInvalidations() { - manager_->EnableInvalidations( - base::Bind( - &CloudPolicyInvalidator::InitializeWithService, - base::Unretained(invalidator_.get()), - base::Unretained(invalidation_service_.get()))); - } - - // Determine if the invalidator has registered with the invalidation service. - bool IsInvalidatorRegistered() { - syncer::ObjectIdSet object_ids = - invalidation_service_->invalidator_registrar().GetAllRegisteredIds(); - return object_ids.size() == 1 && - object_ids.begin()->source() == 12345 && - object_ids.begin()->name() == "12345"; - } - - // Determine if the invalidator is unregistered with the invalidation service. - bool IsInvalidatorUnregistered() { - syncer::ObjectIdSet object_ids = - invalidation_service_->invalidator_registrar().GetAllRegisteredIds(); - return object_ids.empty(); - } - // Required by the refresh scheduler that's created by the manager. base::MessageLoop loop_; @@ -252,11 +195,6 @@ class CloudPolicyManagerTest : public testing::Test { MockCloudPolicyStore store_; scoped_ptr<TestCloudPolicyManager> manager_; - // Invalidation objects. - scoped_refptr<base::TestSimpleTaskRunner> task_runner_; - scoped_ptr<invalidation::FakeInvalidationService> invalidation_service_; - scoped_ptr<CloudPolicyInvalidator> invalidator_; - private: DISALLOW_COPY_AND_ASSIGN(CloudPolicyManagerTest); }; @@ -401,25 +339,5 @@ TEST_F(CloudPolicyManagerTest, SignalOnError) { EXPECT_TRUE(manager_->IsInitializationComplete(POLICY_DOMAIN_CHROME)); } -TEST_F(CloudPolicyManagerTest, EnableInvalidationsBeforeRefreshScheduler) { - CreateInvalidator(); - EXPECT_TRUE(IsInvalidatorUnregistered()); - EnableInvalidations(); - EXPECT_TRUE(IsInvalidatorUnregistered()); - manager_->StartRefreshScheduler(); - EXPECT_TRUE(IsInvalidatorRegistered()); - ShutdownInvalidator(); -} - -TEST_F(CloudPolicyManagerTest, EnableInvalidationsAfterRefreshScheduler) { - CreateInvalidator(); - EXPECT_TRUE(IsInvalidatorUnregistered()); - manager_->StartRefreshScheduler(); - EXPECT_TRUE(IsInvalidatorUnregistered()); - EnableInvalidations(); - EXPECT_TRUE(IsInvalidatorRegistered()); - ShutdownInvalidator(); -} - } // namespace } // namespace policy diff --git a/chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h b/chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h index 4242512..675f964 100644 --- a/chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h +++ b/chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h @@ -62,6 +62,12 @@ class CloudPolicyRefreshScheduler // lower. void SetInvalidationServiceAvailability(bool is_available); + // Whether the invalidations service is available and receiving notifications + // of policy updates. + bool invalidations_available() { + return invalidations_available_; + } + // CloudPolicyClient::Observer: virtual void OnPolicyFetched(CloudPolicyClient* client) OVERRIDE; virtual void OnRegistrationStateChanged(CloudPolicyClient* client) OVERRIDE; diff --git a/chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc b/chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc index 26e001a..896f151 100644 --- a/chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc +++ b/chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc @@ -7,7 +7,7 @@ #include "base/bind.h" #include "base/message_loop/message_loop_proxy.h" #include "chrome/browser/chrome_notification_types.h" -#include "chrome/browser/policy/cloud/cloud_policy_core.h" +#include "chrome/browser/invalidation/invalidation_service_factory.h" #include "chrome/browser/policy/cloud/cloud_policy_manager.h" #include "content/public/browser/notification_source.h" @@ -17,14 +17,18 @@ UserCloudPolicyInvalidator::UserCloudPolicyInvalidator( Profile* profile, CloudPolicyManager* policy_manager) : CloudPolicyInvalidator( - policy_manager, - policy_manager->core()->store(), + policy_manager->core(), base::MessageLoopProxy::current()), - profile_(profile), - policy_manager_(policy_manager) { + profile_(profile) { DCHECK(profile); - // Register for notification that profile creation is complete. + // Register for notification that profile creation is complete. The + // invalidator must not be initialized before then because the invalidation + // service cannot be started because it depends on components initialized + // after this object is instantiated. + // TODO(stepco): Delayed initialization can be removed once the request + // context can be accessed during profile-keyed service creation. Tracked by + // bug 286209. registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED, content::Source<Profile>(profile)); @@ -38,13 +42,13 @@ void UserCloudPolicyInvalidator::Observe( int type, const content::NotificationSource& source, const content::NotificationDetails& details) { - // Enable invalidations now that profile creation is complete. + // Initialize now that profile creation is complete and the invalidation + // service can safely be initialized. DCHECK(type == chrome::NOTIFICATION_PROFILE_ADDED); - policy_manager_->EnableInvalidations( - base::Bind( - &CloudPolicyInvalidator::InitializeWithProfile, - GetWeakPtr(), - base::Unretained(profile_))); + invalidation::InvalidationService* invalidation_service = + invalidation::InvalidationServiceFactory::GetForProfile(profile_); + if (invalidation_service) + Initialize(invalidation_service); } } // namespace policy diff --git a/chrome/browser/policy/cloud/user_cloud_policy_invalidator.h b/chrome/browser/policy/cloud/user_cloud_policy_invalidator.h index 96988c1..52f01be 100644 --- a/chrome/browser/policy/cloud/user_cloud_policy_invalidator.h +++ b/chrome/browser/policy/cloud/user_cloud_policy_invalidator.h @@ -43,9 +43,6 @@ class UserCloudPolicyInvalidator : public CloudPolicyInvalidator, // The profile associated with the invalidator. Profile* profile_; - // The policy manager for the user policy. - CloudPolicyManager* policy_manager_; - // Used to register for notification that profile creation is complete. content::NotificationRegistrar registrar_; diff --git a/chrome/browser/policy/cloud/user_cloud_policy_manager.cc b/chrome/browser/policy/cloud/user_cloud_policy_manager.cc index 1772f03..09db701 100644 --- a/chrome/browser/policy/cloud/user_cloud_policy_manager.cc +++ b/chrome/browser/policy/cloud/user_cloud_policy_manager.cc @@ -35,7 +35,7 @@ UserCloudPolicyManager::~UserCloudPolicyManager() { void UserCloudPolicyManager::Connect( PrefService* local_state, scoped_ptr<CloudPolicyClient> client) { core()->Connect(client.Pass()); - StartRefreshScheduler(); + core()->StartRefreshScheduler(); core()->TrackRefreshDelayPref(local_state, prefs::kUserPolicyRefreshRate); } |