Fix policy invalidator lifecycle bugs for Android.

chrome/browser/policy/cloud/cloud_policy_manager.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_POLICY_CLOUD_CLOUD_POLICY_MANAGER_H_
 #define CHROME_BROWSER_POLICY_CLOUD_CLOUD_POLICY_MANAGER_H_
 
 #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"
 #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"
 
 namespace policy {
 
 class PolicyBundle;
 
 // CloudPolicyManager is the main switching central between cloud policy and the
 // upper layers of the policy stack. It wires up a CloudPolicyCore to the
 // ConfigurationPolicyProvider interface.
 //
 // This class contains the base functionality, there are subclasses that add
 // 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:
   CloudPolicyManager(const PolicyNamespaceKey& policy_ns_key,
                      CloudPolicyStore* cloud_policy_store);
   virtual ~CloudPolicyManager();
 
   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;
   virtual bool IsInitializationComplete(PolicyDomain domain) const OVERRIDE;
   virtual void RefreshPolicies() OVERRIDE;
 
   // CloudPolicyStore::Observer:
   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();
+  // 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.
   virtual scoped_ptr<PolicyBundle> CreatePolicyBundle();
 
   // Convenience accessors to core() components.
   CloudPolicyClient* client() { return core_.client(); }
   const CloudPolicyClient* client() const { return core_.client(); }
   CloudPolicyStore* store() { return core_.store(); }
   const CloudPolicyStore* store() const { return core_.store(); }
   CloudPolicyService* service() { return core_.service(); }
   const CloudPolicyService* service() const { return core_.service(); }
 
  private:
   // Completion handler for policy refresh operations.
   void OnRefreshComplete(bool success);
 
   CloudPolicyCore core_;
 
   // Whether there's a policy refresh operation pending, in which case all
   // 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);
 };
 
 }  // namespace policy
 
 #endif  // CHROME_BROWSER_POLICY_CLOUD_CLOUD_POLICY_MANAGER_H_