Fix policy invalidator lifecycle bugs for Android.

Fix policy invalidator lifecycle bugs for Android.

On Android, the user may disconnect from their Google account and reconnect multiple times within the same session. This case wasn't considered properly when developing the CloudPolicyInvalidator.

When the account is disconnected, the CloudPolicyCore is disconnected before the CloudPolicyStore is reset. When the store is reset, the invalidator unregisters for the policy object and makes a callback to indicate that invalidations are disabled. However, since the CloudPolicyCore is disconnected at this point, a crash is occurring because the code blindly attempts to use objects connected to the core.

Also, when the account becomes reconnected, the refresh scheduler is restarted. This caused the invalidator to be initialized again, which is unexpected.

The solution to these issues is to refactor the CloudPolicyInvalidator to support starting and stopping. The invalidator will listen to events published by the CloudPolicyCore to determine when to start and stop. The invalidator starts when the refresh scheduler starts and stops when it stops.

Also, when CloudPolicyInvalidator::Shutdown is called, outstanding weak pointers held in callbacks should be invalidated. This may not cause any issues since it is likely no messages are pumped on the thread in between the calls to Shutdown and the destructor, but should be fixed.

BUG=280345, 272574, 280536
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221837

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

Some initial comments, let me know whether they make sense.

https://chromiumcodereview-hr.appspot.com/23592017/diff/6001/chrome/browser/p...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://chromiumcodereview-hr.appspot.com/23592017/diff/6001/chrome/browser/p...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:52: // ready or if it is
not needed.
Can you tell me more about the not-ready case? It seems that should be covered
by declaring dependencies properly?

https://chromiumcodereview-hr.appspot.com/23592017/diff/6001/chrome/browser/p...
File chrome/browser/policy/cloud/cloud_policy_manager.h (right):

https://chromiumcodereview-hr.appspot.com/23592017/diff/6001/chrome/browser/p...
chrome/browser/policy/cloud/cloud_policy_manager.h:44: void SetInvalidator(const
base::WeakPtr<CloudPolicyInvalidator>& invalidator);
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.

https://chromiumcodereview-hr.appspot.com/23592017/diff/6001/chrome/browser/p...
File chrome/browser/policy/cloud/user_cloud_policy_manager.cc (right):

https://chromiumcodereview-hr.appspot.com/23592017/diff/6001/chrome/browser/p...
chrome/browser/policy/cloud/user_cloud_policy_manager.cc:40: StartInvalidator();
So the signal the invalidator apparently needs to listen to would be "core
connected/disconnected" AFAICS, correct?

[Steve Condie, 2013-08-30 00:16:34]

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:52: // ready or if it is
not needed.
On 2013/08/29 16:02:30, Mattias Nissler wrote:
> Can you tell me more about the not-ready case? It seems that should be covered
> by declaring dependencies properly?

The requirements to turn on the invalidator are
1) The invalidation service can be created (which needs to be after profile
creation is complete; cannot happen at profile-keyed-service creation time).
2) The cloud policy refresh scheduler is running (alternatively, we could have
some way of storing an invalidation until the refresh scheduler is running, but
I think that would be more complex).

I think in the latest patch set waiting for these dependencies is implemented in
a fairly clean way. 1) is fulfilled by waiting until the profile creation
notification to call the Initialize method and 2) is fulfilled by registering a
CloudPolicyCore observer and starting/stopping the invalidator when the refresh
scheduler is started/stopped.

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/cloud_policy_manager.h:44: void SetInvalidator(const
base::WeakPtr<CloudPolicyInvalidator>& invalidator);
On 2013/08/29 16:02:30, Mattias Nissler wrote:
> 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.

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.

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud/user_cloud_policy_manager.cc (right):

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/user_cloud_policy_manager.cc:40: StartInvalidator();
On 2013/08/29 16:02:30, Mattias Nissler wrote:
> So the signal the invalidator apparently needs to listen to would be "core
> connected/disconnected" AFAICS, correct?

Yes, these signals are now published by the CloudPolicyCore.

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

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:52: // ready or if it is
not needed.
On 2013/08/30 00:16:35, stepco wrote:
> On 2013/08/29 16:02:30, Mattias Nissler wrote:
> > Can you tell me more about the not-ready case? It seems that should be
covered
> > by declaring dependencies properly?
> 
> The requirements to turn on the invalidator are
> 1) The invalidation service can be created (which needs to be after profile
> creation is complete; cannot happen at profile-keyed-service creation time).

Why?

> 2) The cloud policy refresh scheduler is running (alternatively, we could have
> some way of storing an invalidation until the refresh scheduler is running,
but
> I think that would be more complex).
> 
> I think in the latest patch set waiting for these dependencies is implemented
in
> a fairly clean way. 1) is fulfilled by waiting until the profile creation
> notification to call the Initialize method and 2) is fulfilled by registering
a
> CloudPolicyCore observer and starting/stopping the invalidator when the
refresh
> scheduler is started/stopped.

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/23592017/diff/6001/chrome/browser/policy/clou...
chrome/browser/policy/cloud/cloud_policy_manager.h:44: void SetInvalidator(const
base::WeakPtr<CloudPolicyInvalidator>& invalidator);
On 2013/08/30 00:16:35, stepco wrote:
> On 2013/08/29 16:02:30, Mattias Nissler wrote:
> > 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.
> 
> 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.


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

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_core.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_core.cc:37: FOR_EACH_OBSERVER(Observer,
observers_, OnCoreDisconnected(this));
nit: Going by the language, this should run after all the resets below. Or
should this rather be OnCoreDisconnecting?

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:308:
DCHECK(core_->refresh_scheduler());
unneeded, we'll crash in the next line anyways

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:316:
invalidations_disabled_count_++;
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?

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:322:
DCHECK(core_->client());
unneeded, we'll crash in the next line anyways

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:333:
DCHECK(core_->refresh_scheduler());
unneeded, we'll crash in the next line anyways

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:335:
policy_refresh_count_++;
Can we just examine the state of the refresh scheduler to determine whether it
refreshed in the test?

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:61: void Initialize(const
GetInvalidationService& get_invalidation_service);
I still don't see why we can't pass the InvalidationService pointer to the
constructor.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:357: int
start_count = invalidator_->policy_refresh_count();
nit: const

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:28: // after this
object is instantiated.
What are those? Shouldn't they be declared as dependencies of
InvalidationService then?

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

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_core.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_core.cc:37: FOR_EACH_OBSERVER(Observer,
observers_, OnCoreDisconnected(this));
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> nit: Going by the language, this should run after all the resets below. Or
> should this rather be OnCoreDisconnecting?

Renamed to OnCoreDisconnecting.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:308:
DCHECK(core_->refresh_scheduler());
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> unneeded, we'll crash in the next line anyways

Done.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:316:
invalidations_disabled_count_++;
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> 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?

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.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:322:
DCHECK(core_->client());
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> unneeded, we'll crash in the next line anyways

Done.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:333:
DCHECK(core_->refresh_scheduler());
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> unneeded, we'll crash in the next line anyways

Done.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:335:
policy_refresh_count_++;
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> Can we just examine the state of the refresh scheduler to determine whether it
> refreshed in the test?

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?

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:61: void Initialize(const
GetInvalidationService& get_invalidation_service);
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> I still don't see why we can't pass the InvalidationService pointer to the
> constructor.

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.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:357: int
start_count = invalidator_->policy_refresh_count();
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> nit: const

Done.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:28: // after this
object is instantiated.
On 2013/09/02 11:54:52, Mattias Nissler wrote:
> What are those? Shouldn't they be declared as dependencies of
> InvalidationService then?

SSLConfigServiceManager::CreateDefaultManager must run. This is called during
ProfileImpl::DoFinalInit. Since all profile-keyed services are instantiated
before that, I don't see any way to declare that as a dependency to delay
instantiation. The best way I can see to be notified when that is ready is to
wait for the PROFILE_ADDED event (this was the solution suggested by Joao during
the initial code review; he said the same strategy is used in a couple other
places in the policy code).

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

Almost there. I'd like to resolve the question w.r.t. how InvalidationService
initialization should be handled properly. Once we know, we should put a comment
to either (1) justify why we do delayed initialization or (2) add a TODO
explaining that InvalidationService needs to be fixed at which point the delayed
initialization logic can be removed.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:335:
policy_refresh_count_++;
On 2013/09/03 04:41:56, stepco wrote:
> On 2013/09/02 11:54:52, Mattias Nissler wrote:
> > Can we just examine the state of the refresh scheduler to determine whether
it
> > refreshed in the test?
> 
> 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?

Can we just add an is_refresh_pending() function to the refresh scheduler?

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:61: void Initialize(const
GetInvalidationService& get_invalidation_service);
On 2013/09/03 04:41:56, stepco wrote:
> On 2013/09/02 11:54:52, Mattias Nissler wrote:
> > I still don't see why we can't pass the InvalidationService pointer to the
> > constructor.
> 
> 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.

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]

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:335:
policy_refresh_count_++;
On 2013/09/03 14:46:36, Mattias Nissler wrote:
> On 2013/09/03 04:41:56, stepco wrote:
> > On 2013/09/02 11:54:52, Mattias Nissler wrote:
> > > Can we just examine the state of the refresh scheduler to determine
whether
> it
> > > refreshed in the test?
> > 
> > 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?
> 
> Can we just add an is_refresh_pending() function to the refresh scheduler?

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.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.h (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.h:61: void Initialize(const
GetInvalidationService& get_invalidation_service);
On 2013/09/03 14:46:36, Mattias Nissler wrote:
> On 2013/09/03 04:41:56, stepco wrote:
> > On 2013/09/02 11:54:52, Mattias Nissler wrote:
> > > I still don't see why we can't pass the InvalidationService pointer to the
> > > constructor.
> > 
> > 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.
> 
> 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.

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()

[Mattias Nissler (ping if slow), 2013-09-05 14:54:04]

Thanks for bearing with me, almost there.

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:335:
policy_refresh_count_++;
On 2013/09/04 18:00:38, stepco wrote:
> On 2013/09/03 14:46:36, Mattias Nissler wrote:
> > On 2013/09/03 04:41:56, stepco wrote:
> > > On 2013/09/02 11:54:52, Mattias Nissler wrote:
> > > > Can we just examine the state of the refresh scheduler to determine
> whether
> > it
> > > > refreshed in the test?
> > > 
> > > 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?
> > 
> > Can we just add an is_refresh_pending() function to the refresh scheduler?
> 
> 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.
> 
> 

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.

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:131: bool
CheckPolicyRefreshed() {
nit: style guide advises against non-obvious overloads, rename to
CheckPolicyRefreshedWithUnknownVersion?

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:29: //
TODO(stepco): Investigate if we can update the invalidation service such
OK, thanks for the stack trace. It turns out that there's already a bug on file
that complains about GetRequestContext() not working during profile
initialization: https://code.google.com/p/chromium/issues/detail?id=171406

Can you please file a new bug that you reference here which tracks cleaning this
up and block the new bug on 171406?

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:48:
&invalidation::InvalidationServiceFactory::GetForProfile,
I guess you could just call GetForProfile here and pass the the
InvalidationService pointer?

[Steve Condie, 2013-09-06 06:23:05]

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/18001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator.cc:335:
policy_refresh_count_++;
On 2013/09/05 14:54:04, Mattias Nissler wrote:
> On 2013/09/04 18:00:38, stepco wrote:
> > On 2013/09/03 14:46:36, Mattias Nissler wrote:
> > > On 2013/09/03 04:41:56, stepco wrote:
> > > > On 2013/09/02 11:54:52, Mattias Nissler wrote:
> > > > > Can we just examine the state of the refresh scheduler to determine
> > whether
> > > it
> > > > > refreshed in the test?
> > > > 
> > > > 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?
> > > 
> > > Can we just add an is_refresh_pending() function to the refresh scheduler?
> > 
> > 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.
> > 
> > 
> 
> 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.

Tracked in bug 286213.

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc (right):

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc:131: bool
CheckPolicyRefreshed() {
On 2013/09/05 14:54:04, Mattias Nissler wrote:
> nit: style guide advises against non-obvious overloads, rename to
> CheckPolicyRefreshedWithUnknownVersion?

Done.

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:29: //
TODO(stepco): Investigate if we can update the invalidation service such
On 2013/09/05 14:54:04, Mattias Nissler wrote:
> OK, thanks for the stack trace. It turns out that there's already a bug on
file
> that complains about GetRequestContext() not working during profile
> initialization: https://code.google.com/p/chromium/issues/detail?id=171406
> 
> Can you please file a new bug that you reference here which tracks cleaning
this
> up and block the new bug on 171406?

Thanks for looking into that and finding the existing bug. I created bug 286209
to track the work.

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:48:
&invalidation::InvalidationServiceFactory::GetForProfile,
On 2013/09/05 14:54:04, Mattias Nissler wrote:
> I guess you could just call GetForProfile here and pass the the
> InvalidationService pointer?

Since the invalidation service is initialized when GetForProfile is called, and
we're not sure at this point if it will be needed (or if cloud policy will even
be loaded), I thought it would be better to only call GetForProfile once we know
we need the service.

[Mattias Nissler (ping if slow), 2013-09-06 08:51:53]

LGTM once you address the two remaining comments.

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:48:
&invalidation::InvalidationServiceFactory::GetForProfile,
On 2013/09/06 06:23:05, stepco wrote:
> On 2013/09/05 14:54:04, Mattias Nissler wrote:
> > I guess you could just call GetForProfile here and pass the the
> > InvalidationService pointer?
> 
> Since the invalidation service is initialized when GetForProfile is called,
and
> we're not sure at this point if it will be needed (or if cloud policy will
even
> be loaded), I thought it would be better to only call GetForProfile once we
know
> we need the service.

Other consumers (such as the push messaging extension API) grab the
InvalidationService pointer immediately at startup as well, so grabbing the
pointer here directly doesn't make a difference (at least on desktop
platfrorms). Do you have any indication that instantiating InvalidationService
when we don't actually use it causes significant overhead that should be
avoided?

Even if so, I'd again prefer to not work around peculiarities of the
dependencies in consuming code (i.e. InvalidationService should make sure it
keeps its footprint within reasonable limits as long as it is unused), because
that adds noise to all consumers.

If there are no known issues with grabbing the pointer immediately, I prefer
doing it for code clarity.

https://codereview.chromium.org/23592017/diff/57001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/57001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:29: //
TODO(stepco): Investigate if we can update the invalidation service such
Please reference the bug and update the comment to say that delayed
initialization can go away as soon as the request context can be accessed during
profile startup.

[Steve Condie, 2013-09-06 18:01:40]

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/52001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:48:
&invalidation::InvalidationServiceFactory::GetForProfile,
On 2013/09/06 08:51:54, Mattias Nissler wrote:
> On 2013/09/06 06:23:05, stepco wrote:
> > On 2013/09/05 14:54:04, Mattias Nissler wrote:
> > > I guess you could just call GetForProfile here and pass the the
> > > InvalidationService pointer?
> > 
> > Since the invalidation service is initialized when GetForProfile is called,
> and
> > we're not sure at this point if it will be needed (or if cloud policy will
> even
> > be loaded), I thought it would be better to only call GetForProfile once we
> know
> > we need the service.
> 
> Other consumers (such as the push messaging extension API) grab the
> InvalidationService pointer immediately at startup as well, so grabbing the
> pointer here directly doesn't make a difference (at least on desktop
> platfrorms). Do you have any indication that instantiating InvalidationService
> when we don't actually use it causes significant overhead that should be
> avoided?
> 
> Even if so, I'd again prefer to not work around peculiarities of the
> dependencies in consuming code (i.e. InvalidationService should make sure it
> keeps its footprint within reasonable limits as long as it is unused), because
> that adds noise to all consumers.
> 
> If there are no known issues with grabbing the pointer immediately, I prefer
> doing it for code clarity.

Done.

https://codereview.chromium.org/23592017/diff/57001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc (right):

https://codereview.chromium.org/23592017/diff/57001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc:29: //
TODO(stepco): Investigate if we can update the invalidation service such
On 2013/09/06 08:51:54, Mattias Nissler wrote:
> Please reference the bug and update the comment to say that delayed
> initialization can go away as soon as the request context can be accessed
during
> profile startup.

Done.

[rlarocque, 2013-09-06 18:10:08]

DEPS change LGTM.

[commit-bot: I haz the power, 2013-09-06 18:26:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/23592017/61001

[commit-bot: I haz the power, 2013-09-07 00:42:25]

Change committed as 221837