Draft: InvalidationService

Draft: InvalidationService

BUG=

[rlarocque, 2013-04-05 20:40:06]

Wondering why you're being CC'ed on this review?  Jump to the bottom of this
message to see if I have specific questions for you.

This is the big InvalidationService patch I've been talking about for a while.
Here are some of the highlights:
- Implements invalidations as a mostly stand-alone ProfileKeyedService.
- The invalidator now lives on the UI thread, not the sync thread.
- The SyncBackendHost is now responsible for receiving invalidations and
  forwarding them on to the SyncManager, rather than the other way around.
- Most invalidations related code has been pulled out of ProfileSyncService.
- The main public-facing API for invalidations is now InvalidationFrontend,
  not syncer::Invalidator.
- The P2P notification for integration tests has been moved to
  ProfileSyncServiceHarness and other integration-test-only classes.
- Invalidations for Android (which always lived on the UI thread) are now
  represented as a different implementation of InvalidationFrontend and
  ProfileKeyedService.  The InvalidationServiceFactory chooses which one to
  instantiate.

And some of the remaining issues:
- The authentication logic is probably wrong, and still depends on sync.
- Headers and forward declarations are a mess.
- This change makes it harder to implement ack-tracking in sync.  We might need
  a follow-up change to handle this better.
- I should probably add some unit tests to make up for the ones I had to
  delete.
- Files and namespaces need to be rearranged.  For example, the sync/notifier
  directory probably shouldn't exist anymore.  I haven't tried to address this
  problem yet in order to keep the diffs readable.

Speaking of diffs, here's the diffstat:
 69 files changed, 1120 insertions(+), 2171 deletions(-)

The reason why I'm publishing this is that I've got questions for some of you:

Tommy: Does the current strategy for Android support look reasonable?
(see android_invalidation_service.cc, invalidation_service_factory.cc)

Tim, Pavel: The current version of this patch reuses sync's auth token.  Should
the invalidator have its own token?  How do we go about implementing it?

Also, I'm not really sure how to implement a "signed in" service.  Most of the
auth related code is copy/pasted and probably wrong.  I'd appreciate any tips
you may have or pointers to relevant docs.  

Daniel, Fred: You've been added as an FYI.  Feel free to skim the code and leave
comments, or just ignore it.

[rlarocque, 2013-04-05 20:41:32]

On 2013/04/05 20:40:06, rlarocque wrote:
> Wondering why you're being CC'ed on this review?  Jump to the bottom of this
> message to see if I have specific questions for you.
> 
> This is the big InvalidationService patch I've been talking about for a while.
> Here are some of the highlights:
> - Implements invalidations as a mostly stand-alone ProfileKeyedService.
> - The invalidator now lives on the UI thread, not the sync thread.
> - The SyncBackendHost is now responsible for receiving invalidations and
>   forwarding them on to the SyncManager, rather than the other way around.
> - Most invalidations related code has been pulled out of ProfileSyncService.
> - The main public-facing API for invalidations is now InvalidationFrontend,
>   not syncer::Invalidator.
> - The P2P notification for integration tests has been moved to
>   ProfileSyncServiceHarness and other integration-test-only classes.
> - Invalidations for Android (which always lived on the UI thread) are now
>   represented as a different implementation of InvalidationFrontend and
>   ProfileKeyedService.  The InvalidationServiceFactory chooses which one to
>   instantiate.
> 
> And some of the remaining issues:
> - The authentication logic is probably wrong, and still depends on sync.
> - Headers and forward declarations are a mess.
> - This change makes it harder to implement ack-tracking in sync.  We might
need
>   a follow-up change to handle this better.
> - I should probably add some unit tests to make up for the ones I had to
>   delete.
> - Files and namespaces need to be rearranged.  For example, the sync/notifier
>   directory probably shouldn't exist anymore.  I haven't tried to address this
>   problem yet in order to keep the diffs readable.
> 
> Speaking of diffs, here's the diffstat:
>  69 files changed, 1120 insertions(+), 2171 deletions(-)
> 
> The reason why I'm publishing this is that I've got questions for some of you:
> 
> Tommy: Does the current strategy for Android support look reasonable?
> (see android_invalidation_service.cc, invalidation_service_factory.cc)
> 
> Tim, Pavel: The current version of this patch reuses sync's auth token. 
Should
> the invalidator have its own token?  How do we go about implementing it?
> 
> Also, I'm not really sure how to implement a "signed in" service.  Most of the
> auth related code is copy/pasted and probably wrong.  I'd appreciate any tips
> you may have or pointers to relevant docs.  
> 
> Daniel, Fred: You've been added as an FYI.  Feel free to skim the code and
leave
> comments, or just ignore it.

I forgot to mention that the auth-related code can be found in
invalidation_service.cc

[tim (not reviewing), 2013-04-15 16:48:24]

This is a big (but cool) patch.  Hopefully it can be split up a bit?

I didn't give this an in-depth review as I wasn't asked to yet, but I did try to
address your questions about auth. +Roger and Drew (see my shout out below).

Tommy can comment on the Android question.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/android_in...
File chrome/browser/android_invalidation_service.h (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/android_in...
chrome/browser/android_invalidation_service.h:5: #ifndef
CHROME_BROWSER_ANDROID_INVALIDATION_SERVICE_H_
Just a heads up, we should put chrome-focused invalidations files in a
subdirectory of browser/, like sync/.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:275:
InvalidationService* is = InvalidationServiceFactory::GetForProfile(profile_);
nit - |service| would be a better name here, we shouldn't use acronyms.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
File chrome/browser/invalidation_service.cc (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:182: case
chrome::NOTIFICATION_TOKEN_AVAILABLE: {
I forget, do we always send TOKEN_AVAILABLE from TokenService when it loads
tokens from storage at startup (vs TOKEN_LOADING_FINISHED)?  Is it the same on
all platforms?

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:189: Start();
You should check with atwilson@ and rogerta@ about hooking InvalidationService
up with SigninTracker.  See SigninTracker::AreServicesSignedIn, for example.  As
written I think this patch slightly changes the behavior when you sign in to
Chrome, since before we were checking ProfileSyncService auth errors (hence
invalidator auth errors) before broadcasting
SigninTracker::Observer::SigninSuccess.  The OneClickSigninHelper uses that
interface as the primary hook to know when its job is done.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:196: case
chrome::NOTIFICATION_GOOGLE_SIGNED_OUT:
This may get interesting when we have service accounts for device-specific (i.e
not tied to a user) use cases for invalidations. The same can be said for
ProfileSyncService, though it may happen sooner for Invalidations... anyway,
this is just a heads up.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:268: // FIXME: Is it right to use the
sync token here?
Yes, this seems correct and equivalent to before this CL. I think it's true that
we'll likely want a separate token down the road for "user-type syncing
capabilities" versus "chrome system syncing capabilities", and the latter may or
may not be strictly limited to Invalidations (e.g, non-user-controlled datatypes
for playpen / enterprise policy).  If there isn't already a bug for this, feel
free to open one and reference it here in a TODO.  But for now what you are
doing here is the right thing.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
File chrome/browser/invalidation_service.h (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.h:5: #ifndef
CHROME_BROWSER_INVALIDATION_SERVICE_H_
This may have been your plan given the 'Draft' title on the patch, but the new
InvalidationService[Factory, Tests] would be better reviewed as their own small
CL.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
File chrome/browser/invalidation_service_factory.h (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service_factory.h:23: static ProfileKeyedService*
BuildTestServiceInstanceFor(Profile* profile);
Why are the Build functions made public?

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:538:
invalidator_->UnregisterInvalidationHandler(this);
I'm trying to convince myself this is a safe change to shutdown ordering. We
were pretty careful in when we stopped the invalidator vs TerminateAllIO vs
scheduler vs InvalidateWeakPtrs vs SaveChanges and all that stuff in
SyncManager.

But I think this is the right thing to do now, since invalidations don't flow
from SyncManager.  Just calling it out to make sure you / Fred consider shutdown
ordering carefully.

https://codereview.chromium.org/13197004/diff/17001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/13197004/diff/17001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:685: // TODO(akalin): Fix this behavior.
This TODO probably doesn't belong here anymore.

[tim (not reviewing), 2013-04-15 16:49:45]

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/android_in...
File chrome/browser/android_invalidation_service.h (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/android_in...
chrome/browser/android_invalidation_service.h:5: #ifndef
CHROME_BROWSER_ANDROID_INVALIDATION_SERVICE_H_
On 2013/04/15 16:48:24, timsteele wrote:
> Just a heads up, we should put chrome-focused invalidations files in a
> subdirectory of browser/, like sync/.

Er, to clarify, I didn't mean put it in sync/ I meant create a folder "like the
sync folder", perhaps called chrome/browser/invalidations.  There is currently a
chrome/browser/sync/invalidations, which I put there as a temporary home for
some files, but the idea was that the two dirs would be peers.

[rlarocque, 2013-04-17 01:02:32]

Drew/Roger: Ping.  I don't need you to review the entire CL, I just need
pointers to the appropriate docs or examples that would guide me towards
properly "borrowing" sync's credentials.  I haven't found much consistency among
existing signed in services, so I'm wondering which (if any) would be a good
example to follow.

Since one of Tim's suggestions was to break this up into smaller CLs, I guess
I'll upload a smaller CL and start addressing his comments over there.  My
current plan is to split this up into one or two CLs that commit the new classes
as dead code, and another CL that switches over all current invalidation clients
to use InvalidationService and remove the old invalidator code.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/android_in...
File chrome/browser/android_invalidation_service.h (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/android_in...
chrome/browser/android_invalidation_service.h:5: #ifndef
CHROME_BROWSER_ANDROID_INVALIDATION_SERVICE_H_
On 2013/04/15 16:49:45, timsteele wrote:
> On 2013/04/15 16:48:24, timsteele wrote:
> > Just a heads up, we should put chrome-focused invalidations files in a
> > subdirectory of browser/, like sync/.
> 
> Er, to clarify, I didn't mean put it in sync/ I meant create a folder "like
the
> sync folder", perhaps called chrome/browser/invalidations.  There is currently
a
> chrome/browser/sync/invalidations, which I put there as a temporary home for
> some files, but the idea was that the two dirs would be peers.

SGTM.  I wanted to avoid moving files around in this version of the patch to
make it easier to see the diffs for existing files, but I'm in favour of adding
a chrome/browser/invalidations directory.

[rlarocque, 2013-04-22 21:47:15]

I've uploaded a rebased patch and responded to some comments.  I expect this is
the last time I will touch this review.  I'm going to abandon it and upload a
series of smaller patches.  

I talked with Pavel today about token management.  It turns out that I guessed
correctly; this draft patch does a pretty good job of managing tokens.  I don't
think I'll need to make any substantial changes to the token management logic.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/extensions...
File chrome/browser/extensions/api/push_messaging/push_messaging_api.cc (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/api/push_messaging/push_messaging_api.cc:275:
InvalidationService* is = InvalidationServiceFactory::GetForProfile(profile_);
On 2013/04/15 16:48:24, timsteele wrote:
> nit - |service| would be a better name here, we shouldn't use acronyms.

Done.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
File chrome/browser/invalidation_service.cc (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:182: case
chrome::NOTIFICATION_TOKEN_AVAILABLE: {
On 2013/04/15 16:48:24, timsteele wrote:
> I forget, do we always send TOKEN_AVAILABLE from TokenService when it loads
> tokens from storage at startup (vs TOKEN_LOADING_FINISHED)?  Is it the same on
> all platforms?

I think that's always guaranteed.  I was a bit suspicious, since the
ProfileSyncService seems to think it needs to register for both notifications.  

Looking at the implementation:
 - TOKEN_LOADING_FINISHED is emitted by TokenService::LoadTokensIntoMemory()
 - LoadTokensIntoMemory also has a loop that calls
TokenService::LoadSingleTokenIntoMemory()
 - LoadSingleTokenIntoMemory calls FireTokenAvailableNotifications(), which
emits a TOKEN_AVAILABLE notification.

I think ProfileSyncService got it wrong; we only need to register for
TOKEN_AVAILABLE.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:189: Start();
On 2013/04/15 16:48:24, timsteele wrote:
> You should check with atwilson@ and rogerta@ about hooking InvalidationService
> up with SigninTracker.  See SigninTracker::AreServicesSignedIn, for example. 
As
> written I think this patch slightly changes the behavior when you sign in to
> Chrome, since before we were checking ProfileSyncService auth errors (hence
> invalidator auth errors) before broadcasting
> SigninTracker::Observer::SigninSuccess.  The OneClickSigninHelper uses that
> interface as the primary hook to know when its job is done.

I'm not sure that we want to go in that direction.  I see that
AreServicesSignedIn() checks for sync-specific things, like
!service->HasUnrecoverableError().  Should the InvalidationService fail to start
if some data type's configuration failed and generated an unrecoverable error? 
I don't think so, but maybe I'm missing something.  

We can generalize the question as "Should the InvalidationService stop working
when sync isn't working?"  Currently, invalidations stop working when sync does,
so the backwards-compatibility argument favours disabling invalidations when
sync is broken.  On the other hand, part of the point of this patch is to keep
sync-specific issues from interfering with invalidations.  

I'm currently leaning towards keeping them independent, mostly because I have
more confidence that I could implement this approach correctly.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:268: // FIXME: Is it right to use the
sync token here?
On 2013/04/15 16:48:24, timsteele wrote:
> Yes, this seems correct and equivalent to before this CL. I think it's true
that
> we'll likely want a separate token down the road for "user-type syncing
> capabilities" versus "chrome system syncing capabilities", and the latter may
or
> may not be strictly limited to Invalidations (e.g, non-user-controlled
datatypes
> for playpen / enterprise policy).  If there isn't already a bug for this, feel
> free to open one and reference it here in a TODO.  But for now what you are
> doing here is the right thing.

OK.  Just to be clear, my intent with this patch is not to create a system-level
syncing service.  InvalidationService is still a ProfileKeyedService and is
still tied to a specific user.  

I was considering using a separate token here so that we have less dependencies
on sync.  For example, I see that Chrome-to-Mobile and cloud policy have their
own sync tokens.  It made me think that maybe InvalidationService should have
its own token, too.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
File chrome/browser/invalidation_service_factory.h (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service_factory.h:23: static ProfileKeyedService*
BuildTestServiceInstanceFor(Profile* profile);
On 2013/04/15 16:48:24, timsteele wrote:
> Why are the Build functions made public?

That's so we can use it with
ProfileKeyedServiceFactory::SetTestingFactoryAndUse() to inject a P2P
invalidator in integration tests.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:538:
invalidator_->UnregisterInvalidationHandler(this);
On 2013/04/15 16:48:24, timsteele wrote:
> I'm trying to convince myself this is a safe change to shutdown ordering. We
> were pretty careful in when we stopped the invalidator vs TerminateAllIO vs
> scheduler vs InvalidateWeakPtrs vs SaveChanges and all that stuff in
> SyncManager.
> 
> But I think this is the right thing to do now, since invalidations don't flow
> from SyncManager.  Just calling it out to make sure you / Fred consider
shutdown
> ordering carefully.

It's actually pretty easy to verify that this is safe.  We only dereference the
invalidator_ pointer:
- When fetching the client ID, during init.
- When ACKing a received invalidation.
- When updating the list of enabled types following a configure. 
(FinishConfigureDataTypesOnFrontendLoop)

None of which should happen after Shutdown has been called.

https://codereview.chromium.org/13197004/diff/17001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/13197004/diff/17001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:685: // TODO(akalin): Fix this behavior.
On 2013/04/15 16:48:24, timsteele wrote:
> This TODO probably doesn't belong here anymore.
>   

I think it does.  I left it there because I figured it applied equally to
connection_manager_.

[tim (not reviewing), 2013-04-22 22:13:56]

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
File chrome/browser/invalidation_service.cc (right):

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:182: case
chrome::NOTIFICATION_TOKEN_AVAILABLE: {
On 2013/04/22 21:47:15, rlarocque wrote:
> On 2013/04/15 16:48:24, timsteele wrote:
> > I forget, do we always send TOKEN_AVAILABLE from TokenService when it loads
> > tokens from storage at startup (vs TOKEN_LOADING_FINISHED)?  Is it the same
on
> > all platforms?
> 
> I think that's always guaranteed.  I was a bit suspicious, since the
> ProfileSyncService seems to think it needs to register for both notifications.
 
> 
> Looking at the implementation:
>  - TOKEN_LOADING_FINISHED is emitted by TokenService::LoadTokensIntoMemory()
>  - LoadTokensIntoMemory also has a loop that calls
> TokenService::LoadSingleTokenIntoMemory()
>  - LoadSingleTokenIntoMemory calls FireTokenAvailableNotifications(), which
> emits a TOKEN_AVAILABLE notification.
> 
> I think ProfileSyncService got it wrong; we only need to register for
> TOKEN_AVAILABLE.

More likely is at one time TOKEN_AVAILABLE was actually required, but this code
was left behind in the dust of a refactoring elsewhere.  In any case, Drew would
be the best person to review this.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:189: Start();
On 2013/04/22 21:47:15, rlarocque wrote:
> On 2013/04/15 16:48:24, timsteele wrote:
> > You should check with atwilson@ and rogerta@ about hooking
InvalidationService
> > up with SigninTracker.  See SigninTracker::AreServicesSignedIn, for example.

> As
> > written I think this patch slightly changes the behavior when you sign in to
> > Chrome, since before we were checking ProfileSyncService auth errors (hence
> > invalidator auth errors) before broadcasting
> > SigninTracker::Observer::SigninSuccess.  The OneClickSigninHelper uses that
> > interface as the primary hook to know when its job is done.
> 
> I'm not sure that we want to go in that direction.  I see that
> AreServicesSignedIn() checks for sync-specific things, like
> !service->HasUnrecoverableError().  Should the InvalidationService fail to
start
> if some data type's configuration failed and generated an unrecoverable error?

> I don't think so, but maybe I'm missing something.  
> 
> We can generalize the question as "Should the InvalidationService stop working
> when sync isn't working?"  Currently, invalidations stop working when sync
does,
> so the backwards-compatibility argument favours disabling invalidations when
> sync is broken.  On the other hand, part of the point of this patch is to keep
> sync-specific issues from interfering with invalidations.  
> 
> I'm currently leaning towards keeping them independent, mostly because I have
> more confidence that I could implement this approach correctly.  

As I said, just make sure you check with Drew (at least), since he created the
AreServicesSignedIn method and at one time had some thoughts about how we might
extend this in the future for other non-sync services.

https://codereview.chromium.org/13197004/diff/17001/chrome/browser/invalidati...
chrome/browser/invalidation_service.cc:268: // FIXME: Is it right to use the
sync token here?
On 2013/04/22 21:47:15, rlarocque wrote:
> On 2013/04/15 16:48:24, timsteele wrote:
> > Yes, this seems correct and equivalent to before this CL. I think it's true
> that
> > we'll likely want a separate token down the road for "user-type syncing
> > capabilities" versus "chrome system syncing capabilities", and the latter
may
> or
> > may not be strictly limited to Invalidations (e.g, non-user-controlled
> datatypes
> > for playpen / enterprise policy).  If there isn't already a bug for this,
feel
> > free to open one and reference it here in a TODO.  But for now what you are
> > doing here is the right thing.
> 
> OK.  Just to be clear, my intent with this patch is not to create a
system-level
> syncing service.  InvalidationService is still a ProfileKeyedService and is
> still tied to a specific user.  
> 
> I was considering using a separate token here so that we have less
dependencies
> on sync.  For example, I see that Chrome-to-Mobile and cloud policy have their
> own sync tokens.  It made me think that maybe InvalidationService should have
> its own token, too.

So, yes.  I think we're in agreement? In the future we'll want separate tokens
or scopes. We may want one for invalidations, one for system sync, and one for
profile sync, or a different combination; I think this requires its own design
and CL. We may already have a bug for this, I'll keep looking...