Fix policy invalidator lifecycle bugs for Android.

chrome/browser/policy/cloud/cloud_policy_invalidator.cc

 // 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 "chrome/browser/policy/cloud/cloud_policy_invalidator.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #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"
 #include "sync/notifier/object_id_invalidation_map.h"
 
 namespace policy {
 
 const int CloudPolicyInvalidator::kMissingPayloadDelay = 5;
 const int CloudPolicyInvalidator::kMaxFetchDelayDefault = 5000;
 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),
       registered_timestamp_(0),
       invalid_(false),
       invalidation_version_(0),
       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();
 }
 
 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_);
   if (invalidation == invalidation_map.end()) {
     NOTREACHED();
     return;
   }
   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
     // be constructed during an OnStoreLoaded callback, which causes
     // OnStoreLoaded to be called twice at initialization time, so make sure
     // that the timestamp does not match the timestamp at which registration
     // occurred. We only measure changes which occur after registration.
     if (!store->policy() || !store->policy()->has_timestamp() ||
         store->policy()->timestamp() != registered_timestamp_) {
       UMA_HISTOGRAM_ENUMERATION(
           kMetricPolicyRefresh,
           GetPolicyRefreshMetric(),
           METRIC_POLICY_REFRESH_SIZE);
     }
 
     // If the policy was invalid and the version stored matches the latest
     // invalidation version, acknowledge the latest invalidation.
     if (invalid_ && store->invalidation_version() == invalidation_version_)
       AcknowledgeInvalidation();
   }
 
   UpdateRegistration(store->policy());
   UpdateMaxFetchDelay(store->policy_map());
 }
 
 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
   // is a delay in acknowledging it. Duplicate invalidations are ignored.
   if (invalid_ && ack_handle_.Equals(invalidation.ack_handle))
     return;
 
   // If there is still a pending invalidation, acknowledge it, since we only
   // care about the latest invalidation.
   if (invalid_)
@@ skipping 12 matching lines @@
     invalidation_version_ = -(++unknown_version_invalidation_count_);
 
   // In order to prevent the cloud policy server from becoming overwhelmed when
   // a policy with many users is modified, delay for a random period of time
   // before fetching the policy. Delay for at least 20ms so that if multiple
   // invalidations are received in quick succession, only one fetch will be
   // performed.
   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);
 
   // Update the kMetricPolicyInvalidations histogram.
   UMA_HISTOGRAM_BOOLEAN(kMetricPolicyInvalidations, !payload.empty());
 }
 
 void CloudPolicyInvalidator::UpdateRegistration(
     const enterprise_management::PolicyData* policy) {
@@ skipping 12 matching lines @@
 
   // If the policy object id in the policy data is different from the currently
   // registered object id, update the object registration.
   if (!registered_timestamp_ || !(object_id == object_id_))
     Register(policy->timestamp(), object_id);
 }
 
 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;
   }
 
   // Register this handler with the invalidation service if needed.
   if (!registered_timestamp_) {
     OnInvalidatorStateChange(invalidation_service_->GetInvalidatorState());
     invalidation_service_->RegisterInvalidationHandler(this);
   }
 
@@ skipping 53 matching lines @@
     max_fetch_delay_ = kMaxFetchDelayMax;
   else
     max_fetch_delay_ = delay;
 }
 
 void CloudPolicyInvalidator::UpdateInvalidationsEnabled() {
   bool invalidations_enabled =
       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_)
       return METRIC_POLICY_REFRESH_CHANGED;
     return METRIC_POLICY_REFRESH_CHANGED_NO_INVALIDATIONS;
   }
   if (invalid_)
     return METRIC_POLICY_REFRESH_INVALIDATED_UNCHANGED;
   return METRIC_POLICY_REFRESH_UNCHANGED;
 }
 
 }  // namespace policy