Fix policy invalidator lifecycle bugs for Android.

chrome/browser/policy/cloud/cloud_policy_invalidator.h

 // 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.
 
 #ifndef CHROME_BROWSER_POLICY_CLOUD_CLOUD_POLICY_INVALIDATOR_H_
 #define CHROME_BROWSER_POLICY_CLOUD_CLOUD_POLICY_INVALIDATOR_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #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;
 }
 
 namespace invalidation {
 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
   // invalidation with no payload.
   static const int kMissingPayloadDelay;
 
   // The default, min and max values for max_fetch_delay_.
   static const int kMaxFetchDelayDefault;
   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.
   void Shutdown();
 
   // Whether the invalidator currently has the ability to receive invalidations.
   bool invalidations_enabled() {
     return invalidations_enabled_;
   }
 
   // syncer::InvalidationHandler:
   virtual void OnInvalidatorStateChange(
       syncer::InvalidatorState state) OVERRIDE;
   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();
+  // Expose the number of times the invalidator has refreshed the policy. For
+  // testing only.
+  int policy_refresh_count() {
+    return policy_refresh_count_;
+  }
+
+  // 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:
-  // Initialize the invalidator.
-  void Initialize();
-
-  // Returns whether an Initialize method has been called.
-  bool IsInitialized();
-
   // Handle an invalidation to the policy.
   void HandleInvalidation(const syncer::Invalidation& invalidation);
 
   // Update object registration with the invalidation service based on the
   // given policy data.
   void UpdateRegistration(const enterprise_management::PolicyData* policy);
 
   // Registers the given object with the invalidation service.
   void Register(int64 timestamp, const invalidation::ObjectId& object_id);
 
   // Unregisters the current object with the invalidation service.
   void Unregister();
 
   // Update |max_fetch_delay_| based on the given policy map.
   void UpdateMaxFetchDelay(const PolicyMap& policy_map);
   void set_max_fetch_delay(int delay);
 
   // Updates invalidations_enabled_ and calls the invalidation handler if the
   // 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();
 
   // Get the kMetricPolicyRefresh histogram metric which should be incremented
   // 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_;
 
   // Whether the invalidator currently has the ability to receive invalidations.
   // This is true if the invalidation service is enabled and the invalidator
   // has registered for a policy object.
   bool invalidations_enabled_;
 
   // Whether the invalidation service is currently enabled.
@@ skipping 28 matching lines @@
   base::WeakPtrFactory<CloudPolicyInvalidator> weak_factory_;
 
   // The maximum random delay, in ms, between receiving an invalidation and
   // fetching the new policy.
   int max_fetch_delay_;
 
   // A thread checker to make sure that callbacks are invoked on the correct
   // thread.
   base::ThreadChecker thread_checker_;
 
+  // Counters used for testing.
+  int policy_refresh_count_;
+  int invalidations_enabled_count_;
+  int invalidations_disabled_count_;
+
   DISALLOW_COPY_AND_ASSIGN(CloudPolicyInvalidator);
 };
 
-// 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;
-};
-
 }  // namespace policy
 
 #endif  // CHROME_BROWSER_POLICY_CLOUD_CLOUD_POLICY_INVALIDATOR_H_