[BackgroundSync] Fire one-shot sync events

[BackgroundSync] Fire one-shot events

Fires one-shot events.

* Checks for ready events to fire on network change
* Checks for ready events to fire on registration
* A successful event will unregister itself after firing
* A failed event will not fire again until a controlled page load (that is for a future CL)
* Lots and lots of tests!

BUG=479665
Committed: https://crrev.com/81e977f894c3c8d8f7aec029112ea63b4f7a4def
Cr-Commit-Position: refs/heads/master@{#327911}

[jkarlin, 2015-04-24 19:08:39]

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

[jkarlin, 2015-04-24 19:09:41]

michaeln@ Please look at how the service worker registrations and versions are
being used in BackgroundSyncManager.

johnme@ Please look at everything.

Thanks!

[jkarlin, 2015-04-27 11:27:08]

jkarlin@chromium.org changed reviewers:
+ nhiroki@chromium.org
- michaeln@chromium.org

[jkarlin, 2015-04-27 11:28:10]

On 2015/04/24 19:09:41, jkarlin wrote:
> michaeln@ Please look at how the service worker registrations and versions are
> being used in BackgroundSyncManager.
> 
> johnme@ Please look at everything.
> 
> Thanks!

michaeln@ is unavailable, nihroki: Please verify that the use of service worker
registrations and versions is correct. Thanks!

[jkarlin, 2015-04-27 18:29:25]

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

[jkarlin, 2015-04-27 18:29:26]

+r iclelland@, PTAL at everything, thanks!

[nhiroki, 2015-04-28 00:04:27]

On 2015/04/27 11:28:10, jkarlin wrote:
> On 2015/04/24 19:09:41, jkarlin wrote:
> > michaeln@ Please look at how the service worker registrations and versions
are
> > being used in BackgroundSyncManager.
> > 
> > johnme@ Please look at everything.
> > 
> > Thanks!
> 
> michaeln@ is unavailable, nihroki: Please verify that the use of service
worker
> registrations and versions is correct. Thanks!

LGTM!

[iclelland, 2015-04-28 02:48:53]

This looks great -- I added a couple of inline questions about the sync state
transitions.

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.cc:677:
base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback));
Will this happen if the service worker is still activating when this code
executes? Does this leave the sync registration in the FIRING state in that
case, and if so, can the system recover without a restart?

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.cc:706: void
BackgroundSyncManager::EventCompleteImpl(
It looks like there are situations here where the event is complete, but that
fact is not stored. Is that expected?

[jkarlin, 2015-04-28 14:37:55]

Patchset #6 (id:100001) has been deleted

[jkarlin, 2015-04-28 14:41:00]

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.cc:677:
base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback));
On 2015/04/28 02:48:53, iclelland wrote:
> Will this happen if the service worker is still activating when this code
> executes? Does this leave the sync registration in the FIRING state in that
> case, and if so, can the system recover without a restart?

Ah, good point. I'll check for active registration in ::RegisterImpl() in a
followup CL so that we know before registering that the service worker is
already active. For this CL I'll remove the check.

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.cc:706: void
BackgroundSyncManager::EventCompleteImpl(
On 2015/04/28 02:48:53, iclelland wrote:
> It looks like there are situations here where the event is complete, but that
> fact is not stored. Is that expected?

Are you referring to the early returns below? In all of those cases either the
registration is already gone (disabled_ and !registration) or a new registration
has replaced it (registration->id != sync_registration_id). 

The non-early returns will either remove the registration or set its state to
FAILED.

[jkarlin, 2015-04-28 14:41:10]

johnme@, PTAL. Thanks!

[jkarlin, 2015-04-28 16:39:22]

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.cc:677:
base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback));
On 2015/04/28 14:41:00, jkarlin wrote:
> On 2015/04/28 02:48:53, iclelland wrote:
> > Will this happen if the service worker is still activating when this code
> > executes? Does this leave the sync registration in the FIRING state in that
> > case, and if so, can the system recover without a restart?
> 
> Ah, good point. I'll check for active registration in ::RegisterImpl() in a
> followup CL so that we know before registering that the service worker is
> already active. For this CL I'll remove the check.

Followup CL is here: https://crrev.com/1110993003

[iclelland, 2015-04-28 18:50:39]

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.cc:706: void
BackgroundSyncManager::EventCompleteImpl(
On 2015/04/28 14:41:00, jkarlin wrote:
> On 2015/04/28 02:48:53, iclelland wrote:
> > It looks like there are situations here where the event is complete, but
that
> > fact is not stored. Is that expected?
> 
> Are you referring to the early returns below? In all of those cases either the
> registration is already gone (disabled_ and !registration) or a new
registration
> has replaced it (registration->id != sync_registration_id). 

Yes, those ones. So if I'm following the code correctly, the event has been
found, set to state FIRING (in memory only), has run to completion, and now we
discover that it has been replaced. Does the old event need to be deleted from
storage, or will that have been taken care of already?

> 
> The non-early returns will either remove the registration or set its state to
> FAILED.

[jkarlin, 2015-04-28 21:00:09]

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1106523002/diff/60001/content/browser/backgro...
content/browser/background_sync/background_sync_manager.cc:706: void
BackgroundSyncManager::EventCompleteImpl(
On 2015/04/28 18:50:39, iclelland wrote:
> On 2015/04/28 14:41:00, jkarlin wrote:
> > On 2015/04/28 02:48:53, iclelland wrote:
> > > It looks like there are situations here where the event is complete, but
> that
> > > fact is not stored. Is that expected?
> > 
> > Are you referring to the early returns below? In all of those cases either
the
> > registration is already gone (disabled_ and !registration) or a new
> registration
> > has replaced it (registration->id != sync_registration_id). 
> 
> Yes, those ones. So if I'm following the code correctly, the event has been
> found, set to state FIRING (in memory only), has run to completion, and now we
> discover that it has been replaced. Does the old event need to be deleted from
> storage, or will that have been taken care of already?
> 
> > 
> > The non-early returns will either remove the registration or set its state
to
> > FAILED. 
> 

We should be good. If it's disabled_ then all memory registrations have been
wiped from memory and disk in DisableAndClearManager() so there is nothing left
with FIRING. If a registration id has changed then the old FIRING registration
has been replaced in memory and on disk by the new one in RegisterImpl().

[jkarlin, 2015-04-28 21:02:03]

jkarlin@chromium.org changed reviewers:
+ mvanouwerkerk@google.com
- johnme@chromium.org

[jkarlin, 2015-04-28 21:02:04]

-r johnme@ +r mvanouwekerk@ as this is similar to push messaging interactions
with service workers.

[jkarlin, 2015-04-28 21:02:23]

On 2015/04/28 21:02:04, jkarlin wrote:
> -r johnme@ +r mvanouwekerk@ as this is similar to push messaging interactions
> with service workers.

mvanouwekerk@ PTAL at everything, thank you!

[jkarlin, 2015-04-30 10:38:44]

On 2015/04/28 21:02:23, jkarlin wrote:
> On 2015/04/28 21:02:04, jkarlin wrote:
> > -r johnme@ +r mvanouwekerk@ as this is similar to push messaging
interactions
> > with service workers.
> 
> mvanouwekerk@ PTAL at everything, thank you!

Ping mvanouwekerk! Thanks :)

[jkarlin, 2015-05-01 11:29:32]

jkarlin@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org
- mvanouwerkerk@google.com

[Michael van Ouwerkerk, 2015-05-01 12:44:29]

lgtm

My only (paranoia) nit is that if one method in a class checks the thread it is
on, I like all methods in the class to do that, or have a comment why it does
not care.

[jkarlin, 2015-05-01 13:08:51]

Thanks, added the thread checks.

[jkarlin, 2015-05-01 14:45:44]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-05-01 14:45:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/1106523002/#ps220001
(title: "Rebase fix")

[commit-bot: I haz the power, 2015-05-01 14:46:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1106523002/220001

[commit-bot: I haz the power, 2015-05-01 15:53:19]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-05-01 15:54:10]

Patchset 11 (id:??) landed as
https://crrev.com/81e977f894c3c8d8f7aec029112ea63b4f7a4def
Cr-Commit-Position: refs/heads/master@{#327911}