Fix policy invalidator lifecycle bugs for Android.

chrome/browser/policy/cloud/cloud_policy_invalidator.h

Index: chrome/browser/policy/cloud/cloud_policy_invalidator.h
diff --git a/chrome/browser/policy/cloud/cloud_policy_invalidator.h b/chrome/browser/policy/cloud/cloud_policy_invalidator.h
index 2575d1be371d67a6b97a3248e6e7dcc59eec0960..787c2e8ea63f7484c443c1448363f1e5117b6b69 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,25 @@ 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.
+  // |get_invalidation_service| is a callback which gets a pointer to the
+  // invalidation service. It must be callable until Shutdown is called and the
+  // pointer returned must also remain valid during that time. A callback is
+  // passed instead of of the raw pointer so that the InvalidationService can be
+  // lazily initialized.
+  typedef base::Callback<invalidation::InvalidationService*()>
+      GetInvalidationService;
+  void Initialize(const GetInvalidationService& get_invalidation_service);

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

I still don't see why we can't pass the InvalidationService pointer to the
constructor.

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

Creating and registering with the InvalidationService will cause a crash if it
is not delayed until after ProfileImpl::DoFinalInit runs. The constructor runs
during ProfileImpl::OnPrefsLoaded, before DoFinalInit is called.

The crash I get is because SSLConfigServiceManager::CreateDefaultManager has not
been called, which is part of DoFinalInit.

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

Do you have a stack trace? This likely indicates a deficiency on the
InvalidationService implementation (which is the one that's crashing btw.) that
we probably want to solve on the invalidation service side. It doesn't make
sense for all clients of InvalidationService to jump through the same hoops to
work around problems with InvalidationService.

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

Added a TODO to investigate a solution on the InvalidationService side.

Note that the problem is with simply instantiating the InvalidationService, not
even calling any methods on it. I agree that this would need to be handled on
the InvalidationService end; it doesn't make sense for us to explicitly declare
dependencies on indirect dependencies, which is why I didn't put any additional
details in my comment other than that we had to wait for the profile to be
completely initialized before using the InvalidationService. Here is the
callstack:

[4548:4548:0904/103641:FATAL:profile_impl.cc(906)] Check failed:
ssl_config_service_manager_. SSLConfigServiceManager is not initialized yet
 [0x7ff50aa4e5f8] base::debug::StackTrace::StackTrace()
 [0x7ff50aa9121b] logging::LogMessage::~LogMessage()
 [0x7ff50e0fe388] ProfileImpl::GetSSLConfigService()
 [0x7ff50dd8e5bd] ProfileIOData::InitializeOnUIThread()
 [0x7ff50e102966] ProfileImplIOData::Handle::LazyInitialize()
 [0x7ff50e1018e5] ProfileImplIOData::Handle::GetResourceContext()
 [0x7ff50e0fe21c] ProfileImpl::GetResourceContext()
 [0x7ff501bf5ec2] content::StoragePartitionImplMap::Get()
 [0x7ff5017be58d] content::(anonymous
namespace)::GetStoragePartitionFromConfig()
 [0x7ff5017beaa1] content::BrowserContext::GetStoragePartition()
 [0x7ff5017bebd7] content::BrowserContext::GetDefaultStoragePartition()
 [0x7ff50e0fe064] ProfileImpl::GetRequestContext()
 [0x7ff50de0f666] TokenService::Initialize()
 [0x7ff50de0abc3] SigninManagerBase::InitTokenService()
 [0x7ff50de05c2c] SigninManager::InitTokenService()
 [0x7ff50de0ab66] SigninManagerBase::Initialize()
 [0x7ff50de07106] SigninManager::Initialize()
 [0x7ff50de0df66] SigninManagerFactory::BuildServiceInstanceFor()
 [0x7ff50d5cfb5e]
BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
 [0x7ff50de0dd1d] SigninManagerFactory::GetForProfile()
 [0x7ff50df7b542]
invalidation::InvalidationServiceFactory::BuildServiceInstanceFor()
 [0x7ff50d5cfb5e]
BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
 [0x7ff50df7b361] invalidation::InvalidationServiceFactory::GetForProfile()
 [0x7ff50e07e25e]
policy::UserCloudPolicyInvalidator::UserCloudPolicyInvalidator()
 [0x7ff50e07de9e]
policy::UserCloudPolicyInvalidatorFactory::BuildServiceInstanceFor()
 [0x7ff50d5cfb5e]
BrowserContextKeyedServiceFactory::GetServiceForBrowserContext()
 [0x7ff50d5cfea2] BrowserContextKeyedServiceFactory::CreateServiceNow()
 [0x7ff50d5cccb6]
BrowserContextDependencyManager::DoCreateBrowserContextServices()
 [0x7ff50d5cc9de]
BrowserContextDependencyManager::CreateBrowserContextServices()
 [0x7ff50e0fd8fa] ProfileImpl::OnPrefsLoaded()
 [0x7ff50e0fbd15] ProfileImpl::ProfileImpl()
 [0x7ff50e0fb172] Profile::CreateProfile()
 [0x7ff50dd9d0c0] ProfileManager::CreateProfileHelper()
 [0x7ff50dd9b7ab] ProfileManager::GetProfile()
 [0x7ff50e27acb4] (anonymous namespace)::CreateProfile()

 
   // Shuts down and disables invalidations. It must be called before the object
   // is destroyed.
@@ -81,23 +75,31 @@ 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 OnCoreDisconnected(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();
-
- private:
-  // Initialize the invalidator.
-  void Initialize();
+  // Expose the number of times the invalidator has refreshed the policy. For
+  // testing only.
+  int policy_refresh_count() {
+    return policy_refresh_count_;
+  }
 
-  // Returns whether an Initialize method has been called.
-  bool IsInitialized();
+  // Expose the number of times the invalidator has notified the refresh
+  // scheduler of a state change. For testing only.
+  int invalidations_enabled_count() {
+    return invalidations_enabled_count_;
+  }
+  int invalidations_disabled_count() {
+    return invalidations_disabled_count_;
+  }
 
+ private:
   // Handle an invalidation to the policy.
   void HandleInvalidation(const syncer::Invalidation& invalidation);
 
@@ -119,10 +121,14 @@ 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);
+  // Sets the current invalidation info on the cloud policy client.
+  void SetInvalidationInfo(int64 invalidation_version,
+                           const std::string& 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,18 +137,23 @@ 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 callback invoked to lazily retrieve the invalidation service.
+  GetInvalidationService get_invalidation_service_;
 
   // The invalidation service.
   invalidation::InvalidationService* invalidation_service_;
@@ -191,29 +202,12 @@ class CloudPolicyInvalidator : public syncer::InvalidationHandler,
   // thread.
   base::ThreadChecker thread_checker_;
 
-  DISALLOW_COPY_AND_ASSIGN(CloudPolicyInvalidator);
-};
+  // Counters used for testing.
+  int policy_refresh_count_;
+  int invalidations_enabled_count_;
+  int invalidations_disabled_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