Fix policy invalidator lifecycle bugs for Android.

chrome/browser/policy/cloud/cloud_policy_manager.h

Index: chrome/browser/policy/cloud/cloud_policy_manager.h
diff --git a/chrome/browser/policy/cloud/cloud_policy_manager.h b/chrome/browser/policy/cloud/cloud_policy_manager.h
index 6d63e874782eca6e97e9b5e8476e3ea54c398052..81cc1d0f4ada68dc3585564fe90430f7dd22da9e 100644
--- a/chrome/browser/policy/cloud/cloud_policy_manager.h
+++ b/chrome/browser/policy/cloud/cloud_policy_manager.h
@@ -8,8 +8,8 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.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"
@@ -39,12 +39,9 @@ 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);
+  // Sets the cloud policy invalidator used to enable invalidations to the
+  // cloud policy. This method must only be called once.
+  void SetInvalidator(const base::WeakPtr<CloudPolicyInvalidator>& invalidator);

[Mattias Nissler (ping if slow), 2013/08/29 16:02:30]

I'd favor a design where CloudPolicyManager doesn't know about invalidators and
instead just makes appropriate state-change notifications available that the
invalidator can register for and take action on. Reason for this is that
CloudPolicyManager is concerned with the bare essentials of policy handling that
need to happen when we initialize the Profile. In fact, the policy stuff is
among the Profile dependencies that are brought up as the very first pieces.
Putting an edge into the graph that points from one of the Profile's base
services to something that has heavy-weight dependencies such as
InvalidationService thus seems like an unfortunate design to me.

Granted, the various Observer interfaces we already have are not really helping
in this, maybe we need a new observer interfaces that signals events more
helpful to external observers? Note that the sign-in logic may want to use a
similar thing, it's currently also layered on top of additional heavyweight
dependencies (such as SigninManager and ProfileOAuthTokenService)  and calls
down to poke the basic cloud policy pieces when appropriate.

[Steve Condie, 2013/08/30 00:16:35]

Done. In the latest patch nothing calls into the invalidator. The invalidator
starts automatically as a profile service and observes the policy core and
policy store objects to get all of the info it needs. This required a new
observer interface on CloudPolicyCore.

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

Thanks, the new approach seems a lot cleaner (no weak ptrs!), /me likes.

 
   // ConfigurationPolicyProvider:
   virtual void Shutdown() OVERRIDE;
@@ -67,10 +64,16 @@ class CloudPolicyManager : public ConfigurationPolicyProvider,
   // 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();
+  // Starts the policy invalidator. The invalidator must not be started and the
+  // cloud policy refresh scheduler must be running. If the cloud policy core
+  // is disconnected before shutdown, then StopCloudPolicyInvalidator must be
+  // called before disconnecting.
+  void StartInvalidator();
+
+  // Stops the policy invalidator. If the invalidator was started, then this
+  // method must be called before disconnecting the cloud policy core (except
+  // during a normal shutdown).
+  void StopInvalidator();
 
   // Called by CheckAndPublishPolicy() to create a bundle with the current
   // policies.
@@ -94,9 +97,12 @@ 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_;
+  // The cloud policy invalidator.
+  base::WeakPtr<CloudPolicyInvalidator> invalidator_;
+
+  // Whether the invalidator should be started. This is needed since a subclass
+  // may call StartInvalidator before the invalidator has been set.
+  bool invalidator_started_;
 
   DISALLOW_COPY_AND_ASSIGN(CloudPolicyManager);
 };