Fix policy invalidator lifecycle bugs for Android.

chrome/browser/policy/cloud/cloud_policy_invalidator.cc

Index: chrome/browser/policy/cloud/cloud_policy_invalidator.cc
diff --git a/chrome/browser/policy/cloud/cloud_policy_invalidator.cc b/chrome/browser/policy/cloud/cloud_policy_invalidator.cc
index 5012fe7a3998a386029a0b6b5f9782b03f6b487d..f96e859f9546845728b41baf1cc0c9b21bb154b8 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,47 @@ 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),
+      invalidations_enabled_count_(0),
+      invalidations_disabled_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(
-    invalidation::InvalidationService* invalidation_service) {
-  DCHECK(!IsInitialized());
-  DCHECK(invalidation_service);
-  invalidation_service_ = invalidation_service;
-  Initialize();
+void CloudPolicyInvalidator::Initialize(
+    const GetInvalidationService& get_invalidation_service) {
+  DCHECK(state_ == UNINITIALIZED);
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!get_invalidation_service.is_null());
+  get_invalidation_service_ = get_invalidation_service;
+  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 +91,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 +102,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::OnCoreDisconnected(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 +151,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 +183,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);
+    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,11 +229,9 @@ void CloudPolicyInvalidator::UpdateRegistration(
 void CloudPolicyInvalidator::Register(
     int64 timestamp,
     const invalidation::ObjectId& object_id) {
-  // Get the invalidation service from the profile if needed.
+  // Get the invalidation service if needed.
   if (!invalidation_service_) {
-    DCHECK(profile_);
-    invalidation_service_ =
-        invalidation::InvalidationServiceFactory::GetForProfile(profile_);
+    invalidation_service_ = get_invalidation_service_.Run();
     if (!invalidation_service_)
       return;
   }
@@ -295,33 +305,47 @@ void CloudPolicyInvalidator::UpdateInvalidationsEnabled() {
       invalidation_service_enabled_ && registered_timestamp_;
   if (invalidations_enabled_ != invalidations_enabled) {
     invalidations_enabled_ = invalidations_enabled;
-    invalidation_handler_->OnInvalidatorStateChanged(invalidations_enabled);
+    DCHECK(core_->refresh_scheduler());

[Mattias Nissler (ping if slow), 2013/09/02 11:54:52]

unneeded, we'll crash in the next line anyways

[Steve Condie, 2013/09/03 04:41:56]

Done.

+    core_->refresh_scheduler()->SetInvalidationServiceAvailability(
+        invalidations_enabled);
+
+    // Update test counters
+    if (invalidations_enabled)
+      invalidations_enabled_count_++;
+    else
+      invalidations_disabled_count_++;

[Mattias Nissler (ping if slow), 2013/09/02 11:54:52]

Can't we just check invalidations_enabled on the refresh scheduler as a signal
for the test to determine whether the correct action was taken?

[Steve Condie, 2013/09/03 04:41:56]

Changed test to check state on refresh scheduler. The semantics of the test are
not quite the same as before since we don't verify that the update state
function is only called when the state changes. However, since
CloudPolicyRefreshScheduler doesn't require that behavior I think it's fine to
not test it.

   }
 }
 
-void CloudPolicyInvalidator::RunInvalidateCallback(bool is_missing_payload) {
+void CloudPolicyInvalidator::SetInvalidationInfo(int64 invalidation_version,
+                                                 const std::string& payload) {
+  DCHECK(core_->client());

[Mattias Nissler (ping if slow), 2013/09/02 11:54:52]

unneeded, we'll crash in the next line anyways

[Steve Condie, 2013/09/03 04:41:56]

Done.

+  core_->client()->SetInvalidationInfo(invalidation_version, 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)
+    SetInvalidationInfo(invalidation_version_, std::string());
+  DCHECK(core_->refresh_scheduler());

[Mattias Nissler (ping if slow), 2013/09/02 11:54:52]

unneeded, we'll crash in the next line anyways

[Steve Condie, 2013/09/03 04:41:56]

Done.

+  core_->refresh_scheduler()->RefreshSoon();
+  policy_refresh_count_++;

[Mattias Nissler (ping if slow), 2013/09/02 11:54:52]

Can we just examine the state of the refresh scheduler to determine whether it
refreshed in the test?

[Steve Condie, 2013/09/03 04:41:56]

I don't think there's a very straightforward way to do this since the refresh
scheduler posts a task to a task runner which we don't have access to. In the
refresh scheduler unit tests, this is overcome by having knowledge of the
internals of the class and by passing a testable task runner in the constructor;
however, in our case since the refresh scheduler is being created indirectly
(through the CloudPolicyCore) there is currently no way to pass a task runner
under our control to the refresh scheduler.

Maybe we can open a bug to make this functionality more testable on the refresh
scheduler?

[Mattias Nissler (ping if slow), 2013/09/03 14:46:36]

Can we just add an is_refresh_pending() function to the refresh scheduler?

[Steve Condie, 2013/09/04 18:00:38]

It's not quite that simple because a refresh is essentially always pending, so
we have to check what the current delay before a refresh is, and we have to have
a way of running or resetting the current delay. The TaskRunner interface
doesn't expose this type of functionality, which is why the refresh scheduler
unit tests use a TestSimpleTaskRunner which exposes a lot more testing
functionality. However, the way CloudPolicyCore is written, we can't pass a
TestSimpleTaskRunner to the refresh scheduler, and even if we could the test
might be too fragile, relying on internal implementation details of the refresh
scheduler.

I suggest fixing this is involved enough to merit a separate bug and CL.
Meanwhile, we can get this CL in and improve that aspect of the unit test later.

[Mattias Nissler (ping if slow), 2013/09/05 14:54:04]

Thought some more about this. Given that you're now testing against the
production CloudPolicyCore, the thing you actually want to test is that a policy
fetch gets issued. To do so, you could just spin the main message loop to see
whether the whole thing does actually result in a FetchPolicy() call on the
client.

I'm fine with doing this in a follow-up CL.

[Steve Condie, 2013/09/06 06:23:05]

Tracked in bug 286213.

 }
 
 void CloudPolicyInvalidator::AcknowledgeInvalidation() {
   DCHECK(invalid_);
   invalid_ = false;
-  invalidation_handler_->SetInvalidationInfo(0, std::string());
+  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_)