[BackgroundSync] Add browser side support for SyncRegistration.done

[BackgroundSync] Add browser side support for SyncRegistration.done

This CL adds support for the "done" promise of background sync registrations. See
http://slightlyoff.github.io/BackgroundSync/spec/index.html for more detail.

Done resolves with true if the sync event fired successfully and false
if it failed or is unregistered.

Changes:

1. Add and test BackgroundSyncManager::NotifyWhenDone.
2. Plumb NotifyWhenDone to the child process.
3. Test with BrowserTests that done can be called before or after a registration completes.

Depends on:
https://codereview.chromium.org/1353613002/
https://codereview.chromium.org/1348603003/

BUG=502214
Committed: https://crrev.com/6da0b6f3b2159374954c9e77b5f5f468f901a0cb
Cr-Commit-Position: refs/heads/master@{#349660}

[jkarlin, 2015-09-15 15:57:52]

jkarlin@chromium.org changed reviewers:
+ iclelland@chromium.org, michaeln@chromium.org

[jkarlin, 2015-09-15 15:57:53]

iclelland: PTAL at everything, thanks!
michaeln: PTAL at everything, the registration handles are put to use!

[jkarlin, 2015-09-15 15:58:31]

jkarlin@chromium.org changed reviewers:
+ tsepez@chromium.org

[jkarlin, 2015-09-15 15:58:31]

tsepez@chromium.org: Please review changes in the mojom file. Thanks!

[Tom Sepez, 2015-09-15 16:48:54]

mojom LGTM

[jkarlin, 2015-09-16 16:40:56]

Patchset #3 (id:40001) has been deleted

[jkarlin, 2015-09-16 18:49:39]

PTAL, many thanks

[iclelland, 2015-09-16 20:36:53]

On 2015/09/16 18:49:39, jkarlin wrote:
> PTAL, many thanks

At first glance, it seems like there are there are three or four orthogonal
changes in this CL:

 - Moving the status from the protobuf to the mojom file
 - C++ style changes in unrelated methods
 - Cleanup from the registration CL (in BackgroundSyncServiceImpl::Unregister)
 - SyncRegistration.done, NotifyWhenDone, and related tests (the meat of it)

Is it possible to split it up? At least the git history would make more sense if
the commit message for some of these changes matched the actual reason for the
change.

(Or maybe they're actually tied together more tightly than they look, in which
case, I'll just carry on with the rest of the review :) )

[jkarlin, 2015-09-17 00:46:40]

Patchset #4 (id:80001) has been deleted

[jkarlin, 2015-09-17 00:49:10]

On 2015/09/16 20:36:53, iclelland wrote:
> On 2015/09/16 18:49:39, jkarlin wrote:
> > PTAL, many thanks
> 
> At first glance, it seems like there are there are three or four orthogonal
> changes in this CL:
> 
>  - Moving the status from the protobuf to the mojom file

Now in its own CL.

>  - C++ style changes in unrelated methods
>  - Cleanup from the registration CL (in BackgroundSyncServiceImpl::Unregister)

Put the two above in their own CL.

>  - SyncRegistration.done, NotifyWhenDone, and related tests (the meat of it)

That's now what this CL is.

[iclelland, 2015-09-17 13:48:10]

LGTM! This is great (and much easier to follow now)

I left a couple of comments, but nothing blocking.

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1106: // if it was
already out of retry attempts otherwise keep the state as
Add link to bug? crbug.com/501838

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.cc (right):

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.cc:37:
DCHECK_NE(BACKGROUND_SYNC_STATE_UNREGISTERED, sync_state_);
This set of negative tests complements the positive tests in NotifyWhenDoneImpl,
but they could easily get out of sync. Could we switch these around, or add a
comment that adding a state means updating one or the other method?

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.h
(right):

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:51: //
*_STATE_FAILED on final failure, and *_STATE_UNREGISTERED if the
Is * just 'tired of typing', or does it match something else besides
'BACKGROUND_SYNC'?

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:238:
BACKGROUND_SYNC_STATE_FAILED);
Why FAILED here? I expected to see something like UNREGISTERED, unless that
actually implies something different.

https://codereview.chromium.org/1344843003/diff/100001/content/test/data/back...
File content/test/data/background_sync/service_worker.js (right):

https://codereview.chromium.org/1344843003/diff/100001/content/test/data/back...
content/test/data/background_sync/service_worker.js:115: function
sendSyncErrorToClients(error) {
I'm sure there's an opportunity to refactor the rest of the error handing in
this script to use this function. No need to do it right now, though.

[jkarlin, 2015-09-17 17:40:25]

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1106: // if it was
already out of retry attempts otherwise keep the state as
On 2015/09/17 13:48:09, iclelland wrote:
> Add link to bug? crbug.com/501838

Done.

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.cc (right):

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.cc:37:
DCHECK_NE(BACKGROUND_SYNC_STATE_UNREGISTERED, sync_state_);
On 2015/09/17 13:48:09, iclelland wrote:
> This set of negative tests complements the positive tests in
NotifyWhenDoneImpl,
> but they could easily get out of sync. Could we switch these around, or add a
> comment that adding a state means updating one or the other method?

Nice catch. I've added HasCompleted() which uses a switch so we should be good
on future enum changes.

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.h
(right):

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:51: //
*_STATE_FAILED on final failure, and *_STATE_UNREGISTERED if the
On 2015/09/17 13:48:09, iclelland wrote:
> Is * just 'tired of typing', or does it match something else besides
> 'BACKGROUND_SYNC'?

Done.

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1344843003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:238:
BACKGROUND_SYNC_STATE_FAILED);
On 2015/09/17 13:48:09, iclelland wrote:
> Why FAILED here? I expected to see something like UNREGISTERED, unless that
> actually implies something different.

If there is no active handle then the javascript attempted to call done on a
registration that it shouldn't have been able to have an id for.

https://codereview.chromium.org/1344843003/diff/100001/content/test/data/back...
File content/test/data/background_sync/service_worker.js (right):

https://codereview.chromium.org/1344843003/diff/100001/content/test/data/back...
content/test/data/background_sync/service_worker.js:115: function
sendSyncErrorToClients(error) {
On 2015/09/17 13:48:10, iclelland wrote:
> I'm sure there's an opportunity to refactor the rest of the error handing in
> this script to use this function. No need to do it right now, though.

Acknowledged.

[jkarlin, 2015-09-17 18:02:34]

michaeln: PTAL, thank you.

[michaeln, 2015-09-17 21:44:12]

lgtm but please see the comments about thread safety

https://codereview.chromium.org/1344843003/diff/160001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1344843003/diff/160001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:249: //
BackgroundSyncRegistrationHandle::NotifyWhenDone for detailed
this ptr to the doc comments was very helpful

https://codereview.chromium.org/1344843003/diff/160001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.h
(right):

https://codereview.chromium.org/1344843003/diff/160001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:53: //
before it could complete.
maybe comment about what "done" means in the context of periodic vs one-time
registrations

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:151:
scoped_ptr<blink::WebSyncNotifyWhenDoneCallbacks> callbacksPtr(callbacks);
nit: callbacks_ptr

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:216:
blink::WebSyncNotifyWhenDoneCallbacks* callbacks) {
I'm very wary of the thread affinity all blink::WebFoo classes. Is it safe to
delete |callbacks| on the main thread if the worker should exit prior to done
happening?

I'm not sure if deletion on the main thread may be possible with the
CallbackThreadAdapter even when the worker is still running? The bind closure
owns the callbackptr, actually the refcounted bindstate buried inside the
closure owns it. The bindstate is refcounted to as copies of the closure are
made. If at some point the closure is not handled as a move-only type, i think
deletion on the wrong thread could happen even if the worker thread is still
running at done time.

(hopefully soon, per-thread BackgroundSyncProviderIntfPtr to the rescue)

https://codereview.chromium.org/1344843003/diff/160001/content/test/data/back...
File content/test/data/background_sync/service_worker.js (right):

https://codereview.chromium.org/1344843003/diff/160001/content/test/data/back...
content/test/data/background_sync/service_worker.js:44: .then(function
(syncRegistration) {
nit: indent two more spaces?

[jkarlin, 2015-09-18 12:03:20]

Thanks!

https://codereview.chromium.org/1344843003/diff/160001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.h (right):

https://codereview.chromium.org/1344843003/diff/160001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.h:249: //
BackgroundSyncRegistrationHandle::NotifyWhenDone for detailed
On 2015/09/17 21:44:12, michaeln wrote:
> this ptr to the doc comments was very helpful

Acknowledged.

https://codereview.chromium.org/1344843003/diff/160001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration_handle.h
(right):

https://codereview.chromium.org/1344843003/diff/160001/content/browser/backgr...
content/browser/background_sync/background_sync_registration_handle.h:53: //
before it could complete.
On 2015/09/17 21:44:12, michaeln wrote:
> maybe comment about what "done" means in the context of periodic vs one-time
> registrations

Done. Added comment and DCHECK to say that this is for one-shot only.

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:151:
scoped_ptr<blink::WebSyncNotifyWhenDoneCallbacks> callbacksPtr(callbacks);
On 2015/09/17 21:44:12, michaeln wrote:
> nit: callbacks_ptr

Done.

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:216:
blink::WebSyncNotifyWhenDoneCallbacks* callbacks) {
On 2015/09/17 21:44:12, michaeln wrote:
> I'm very wary of the thread affinity all blink::WebFoo classes. Is it safe to
> delete |callbacks| on the main thread if the worker should exit prior to done
> happening?
> 
> I'm not sure if deletion on the main thread may be possible with the
> CallbackThreadAdapter even when the worker is still running? The bind closure
> owns the callbackptr, actually the refcounted bindstate buried inside the
> closure owns it. The bindstate is refcounted to as copies of the closure are
> made. If at some point the closure is not handled as a move-only type, i think
> deletion on the wrong thread could happen even if the worker thread is still
> running at done time.
> 
> (hopefully soon, per-thread BackgroundSyncProviderIntfPtr to the rescue)

Yes, good catch. chasej@ is making good headway into the mojo-per-thread CL.

https://codereview.chromium.org/1344843003/diff/160001/content/test/data/back...
File content/test/data/background_sync/service_worker.js (right):

https://codereview.chromium.org/1344843003/diff/160001/content/test/data/back...
content/test/data/background_sync/service_worker.js:44: .then(function
(syncRegistration) {
On 2015/09/17 21:44:12, michaeln wrote:
> nit: indent two more spaces?

Done.

[jkarlin, 2015-09-18 13:50:02]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-09-18 13:50:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
iclelland@chromium.org, michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1344843003/#ps180001
(title: "Address comments from PS7")

[commit-bot: I haz the power, 2015-09-18 13:50:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1344843003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1344843003/180001

[commit-bot: I haz the power, 2015-09-18 13:55:25]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-09-18 13:56:03]

Patchset 8 (id:??) landed as
https://crrev.com/6da0b6f3b2159374954c9e77b5f5f468f901a0cb
Cr-Commit-Position: refs/heads/master@{#349660}

[michaeln, 2015-09-18 19:18:52]

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:216:
blink::WebSyncNotifyWhenDoneCallbacks* callbacks) {
> Yes, good catch. chasej@ is making good headway into the mojo-per-thread CL.

Glad to hear that. I hope it helps to establish safe best practices for using
mojo in workers. Its just so easy to get introduce threading bugs, having a good
example w/o bugs to follow will be nice.

Can you point me at a cl or other artifacts of work in progress?

[jkarlin, 2015-09-21 13:28:39]

On 2015/09/18 19:18:52, michaeln wrote:
>
https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
> File content/child/background_sync/background_sync_provider_thread_proxy.cc
> (right):
> 
>
https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
> content/child/background_sync/background_sync_provider_thread_proxy.cc:216:
> blink::WebSyncNotifyWhenDoneCallbacks* callbacks) {
> > Yes, good catch. chasej@ is making good headway into the mojo-per-thread CL.
> 
> Glad to hear that. I hope it helps to establish safe best practices for using
> mojo in workers. Its just so easy to get introduce threading bugs, having a
good
> example w/o bugs to follow will be nice.
> 
> Can you point me at a cl or other artifacts of work in progress?

https://code.google.com/p/chromium/issues/detail?id=527601 is the bug, no CL up
for review yet. Last Thursday Jason mentioned that he figured out how to do it
cleanly so I expect a CL in the near future.

[jkarlin, 2015-09-23 17:01:02]

On 2015/09/21 13:28:39, jkarlin wrote:
> On 2015/09/18 19:18:52, michaeln wrote:
> >
>
https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
> > File content/child/background_sync/background_sync_provider_thread_proxy.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1344843003/diff/160001/content/child/backgrou...
> > content/child/background_sync/background_sync_provider_thread_proxy.cc:216:
> > blink::WebSyncNotifyWhenDoneCallbacks* callbacks) {
> > > Yes, good catch. chasej@ is making good headway into the mojo-per-thread
CL.
> > 
> > Glad to hear that. I hope it helps to establish safe best practices for
using
> > mojo in workers. Its just so easy to get introduce threading bugs, having a
> good
> > example w/o bugs to follow will be nice.
> > 
> > Can you point me at a cl or other artifacts of work in progress?
> 
> https://code.google.com/p/chromium/issues/detail?id=527601 is the bug, no CL
up
> for review yet. Last Thursday Jason mentioned that he figured out how to do it
> cleanly so I expect a CL in the near future.

https://codereview.chromium.org/1358063004/ is the WIP CL.