Separate local and remote sync invalidations

Separate local and remote sync invalidations

The old design used ChromeSyncNotificationsBridge to register for both
NOTIFICATION_SYNC_REFRESH_LOCAL and NOTIFICATION_SYNC_REFRESH_REMOTE.
The first of these is issued when some component decides that the syncer
ought to perform an unsolicited GetUpdates for some type, while the
second is an Android-specific mechanism for receiving remote
invalidations.  In either case, the ChromeSyncNotificationsBridge would
forward the invalidations to the SyncManager through the
BridgedInvalidator as though they had come from any other invalidator.

In this new design, the local and remote invalidations sources are kept
distinct.  The SyncBackendHost is now responsible for listening to and
forwarding NOTIFICATION_SYNC_REFRESH_LOCAL.  Note that there is no
longer any type-specific filtering performed as we receive
notifications; it will be up to the SyncManager to ignore notifications
for disbaled types.

The successor to ChromeSyncNotificationsBridge,
AndroidInvalidatorBridge, is responsible only for forwarding
NOTIFICATION_SYNC_REFRESH_REMOTE notificaitons.  It is passed to the
SyncManager only if OS_ANDROID is defined.

Since there is only one source of remote invalidations, the
BridgedInvalidator would seem to be no longer necessary.  Unfortunately,
this is not the case.  The BridgedInvalidator was useful in part because
it wrapped the UI-thread-owned ChromeSyncNotificationsBridge without
owning it, so the SyncManager's destruction would not affect it.  This
wrapping functionality is still needed by the AndroidInvalidatorBridge.
The BridgedInvalidator has been modified to only wrap a single
AndroidInvalidatorBridge, and renamed to AndroidInvalidatorBridgeProxy.

BUG=124143
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179546

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

Hi Everyone,

Here's an attempt at crbug.com/124137.

Most of what I'm planning is described in the commit message, so feel free to
read that and ignore the code.  The patch is there in case you're interested in
more details.  

I'd like to know if anyone has strong opinions about this change.  I'm still
learning about the invalidations system, so it's certainly possible I've
misunderstood some of the features of the current design.

The code is currently in rough shape.  The tests won't even compile, the problem
of what to do with android notifications remains unsolved, and there are style
violations everywhere.  However, assuming there are no objections, I'm confident
I could polish this into something more worthy of being committed.

[rlarocque, 2013-01-23 20:32:45]

Sounds like there are no objections.

I've cleaned up the patch, added android support (with something similar to
solution #2 described earlier), and updated the change message.  

Once the trybots (especially linux_android) have had a chance to test this, I'll
start asking for reviews.

[rlarocque, 2013-01-24 01:52:22]

Looks like the tests are OK.  

Nicolas, could you review this change?

Tommy: Please take a look to make sure I won't be causing too many headaches for
the Android build.

Brett: Please do the OWNERS review for chrome/

[Nicolas Zea, 2013-01-25 02:08:09]

On 2013/01/24 01:52:22, rlarocque wrote:
> Looks like the tests are OK.  
> 
> Nicolas, could you review this change?
> 
> Tommy: Please take a look to make sure I won't be causing too many headaches
for
> the Android build.
> 
> Brett: Please do the OWNERS review for chrome/

Looks like the sync unit tests are having issues on android:
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

[rlarocque, 2013-01-25 23:42:58]

On 2013/01/25 02:08:09, Nicolas Zea wrote:
> On 2013/01/24 01:52:22, rlarocque wrote:
> > Looks like the tests are OK.  
> > 
> > Nicolas, could you review this change?
> > 
> > Tommy: Please take a look to make sure I won't be causing too many headaches
> for
> > the Android build.
> > 
> > Brett: Please do the OWNERS review for chrome/
> 
> Looks like the sync unit tests are having issues on android:
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

I added a NOTREACHED() that was a bit too aggressive.  It turns out that unit
tests will exercise that code, even though non-test code would not.  

I've removed the check, which should fix the tests.

PTAL.

[Nicolas Zea, 2013-01-28 23:48:43]

Would it be better to not have a proxy, and instead just have the SyncManager
not take ownership of its invalidator? It'll still unregister at shutdown, and
the SBH has ownership of both, so I don't think the'll be a race condition (but
I may be missing something).

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

https://codereview.chromium.org/12022041/diff/25001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:177: // Ask the syncer check for
updates for the specified types.
"syncer to check for server updates"

https://codereview.chromium.org/12022041/diff/25001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/12022041/diff/25001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:1285:
ModelTypeSetToInvalidationMap(types, "");
handle the empty type invalidation map case here too?

[rlarocque, 2013-01-29 18:48:29]

On 2013/01/28 23:48:43, Nicolas Zea wrote:
> Would it be better to not have a proxy, and instead just have the SyncManager
> not take ownership of its invalidator? It'll still unregister at shutdown, and
> the SBH has ownership of both, so I don't think the'll be a race condition
(but
> I may be missing something).

Yes, that's probably possible.  I haven't looked into it too much because the
invalidator passed to sync is a NonBlockingInvalidationNotifier, which is
NonThreadSafe, so it must be constructed and destructed on the thread that owns
it.  I'm not sure if that assertion is actually necessary for correctness, or
just something that the object happened to inherit from NonThreadSafe.  

I figure it's easier to not touch any thread-related code for now.  The plan is
to eventually move notifications off the sync thread entirely; we can move both
the thread ownership and creation/destruction responsibility when we implement
that change.  

> 
>
https://codereview.chromium.org/12022041/diff/25001/chrome/browser/sync/glue/...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
https://codereview.chromium.org/12022041/diff/25001/chrome/browser/sync/glue/...
> chrome/browser/sync/glue/sync_backend_host.cc:177: // Ask the syncer check for
> updates for the specified types.
> "syncer to check for server updates"
> 
>
https://codereview.chromium.org/12022041/diff/25001/sync/internal_api/sync_ma...
> File sync/internal_api/sync_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/12022041/diff/25001/sync/internal_api/sync_ma...
> sync/internal_api/sync_manager_impl.cc:1285:
> ModelTypeSetToInvalidationMap(types, "");
> handle the empty type invalidation map case here too?

[rlarocque, 2013-01-29 18:48:58]

Patch updated.

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

https://codereview.chromium.org/12022041/diff/25001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:177: // Ask the syncer check for
updates for the specified types.
On 2013/01/28 23:48:43, Nicolas Zea wrote:
> "syncer to check for server updates"

Done.

https://codereview.chromium.org/12022041/diff/25001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/12022041/diff/25001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:1285:
ModelTypeSetToInvalidationMap(types, "");
On 2013/01/28 23:48:43, Nicolas Zea wrote:
> handle the empty type invalidation map case here too?

Fixed.  I'm not sure that it's necessary, but it can't hurt to maintain the
current behaviour.

[Nicolas Zea, 2013-01-30 00:04:11]

I think I'm a bit confused then. Doesn't this code hit the same threading issues
that having the SyncManagerImpl not take ownership of the invalidator would hit?
Namely, the sync manager is accessing the invalidator from the sync thread (via
the proxy), while it's in fact created and destroyed from the UI thread by the
SBH?

On 2013/01/29 18:48:58, rlarocque wrote:
> Patch updated.
> 
>
https://codereview.chromium.org/12022041/diff/25001/chrome/browser/sync/glue/...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
https://codereview.chromium.org/12022041/diff/25001/chrome/browser/sync/glue/...
> chrome/browser/sync/glue/sync_backend_host.cc:177: // Ask the syncer check for
> updates for the specified types.
> On 2013/01/28 23:48:43, Nicolas Zea wrote:
> > "syncer to check for server updates"
> 
> Done.
> 
>
https://codereview.chromium.org/12022041/diff/25001/sync/internal_api/sync_ma...
> File sync/internal_api/sync_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/12022041/diff/25001/sync/internal_api/sync_ma...
> sync/internal_api/sync_manager_impl.cc:1285:
> ModelTypeSetToInvalidationMap(types, "");
> On 2013/01/28 23:48:43, Nicolas Zea wrote:
> > handle the empty type invalidation map case here too?
> 
> Fixed.  I'm not sure that it's necessary, but it can't hurt to maintain the
> current behaviour.

[rlarocque, 2013-01-30 00:16:30]

On 2013/01/30 00:04:11, Nicolas Zea wrote:
> I think I'm a bit confused then. Doesn't this code hit the same threading
issues
> that having the SyncManagerImpl not take ownership of the invalidator would
hit?
> Namely, the sync manager is accessing the invalidator from the sync thread
(via
> the proxy), while it's in fact created and destroyed from the UI thread by the
> SBH?
> 

I'm not sure which invalidator you're talking about here.

The non-Android case invalidator is owned and managed entirely by the sync
thread, since it doesn't get constructed until the initialization process
reaches SBH::Core.

The Android invalidator is a bit more complicated.  It is designed to be used on
the sync thread while being created and destroyed on the UI thread.  The Proxy
has no effect except to prevent the sync thread from taking ownership of the
AndroidInvalidatorBridge.

[Nicolas Zea, 2013-01-30 00:50:50]

LGTM

https://codereview.chromium.org/12022041/diff/32002/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/android_invalidator_bridge.cc (right):

https://codereview.chromium.org/12022041/diff/32002/chrome/browser/sync/glue/...
chrome/browser/sync/glue/android_invalidator_bridge.cc:204: =
syncer::REMOTE_INVALIDATION;
move '=' onto end of previous line

[commit-bot: I haz the power, 2013-01-30 01:33:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/12022041/24024

[commit-bot: I haz the power, 2013-01-30 06:25:05]

Change committed as 179546