[BackgroundSync]: Resolve the register promise before firing sync events

[BackgroundSync]: Resolve the register promise before firing sync events

Previously register promise resolution and sync event dispatch was
racy. This patch moves the sync event dispatch to after the register
promise callback is invoked.

BUG=671980

[falken, 2016-12-15 06:59:40]

falken@chromium.org changed reviewers:
+ shimazu@chromium.org

[falken, 2016-12-15 06:59:41]

shimazu@ I wrote this code but then realized maybe this is not sufficient if the
register promise and sync event dispatch are using different mojo "pipes". WDYT?

[shimazu, 2016-12-15 07:07:25]

On 2016/12/15 06:59:41, falken wrote:
> shimazu@ I wrote this code but then realized maybe this is not sufficient if
the
> register promise and sync event dispatch are using different mojo "pipes".
WDYT?

I think this patch makes sense, but I'm wondering how it's spec-ed.
BackgroundSyncManagerPtr should be associated with the event dispatcher if the
ordering is defined by the spec.

[falken, 2016-12-15 07:10:32]

On 2016/12/15 07:07:25, shimazu wrote:
> On 2016/12/15 06:59:41, falken wrote:
> > shimazu@ I wrote this code but then realized maybe this is not sufficient if
> the
> > register promise and sync event dispatch are using different mojo "pipes".
> WDYT?
> 
> I think this patch makes sense, but I'm wondering how it's spec-ed.
> BackgroundSyncManagerPtr should be associated with the event dispatcher if the
> ordering is defined by the spec.

I believe according to
https://wicg.github.io/BackgroundSync/spec/#dom-syncmanager-register the promise
must be resolved before the sync events for the new registration are fired:
5. Resolve promise.
6. If the user agent is currently online, fire a sync event for newRegistration.

Are you saying in order to ensure the ordering, we'd need to "associate"
BackgroundSyncManagerPtr with the event dispatcher, i.e., this patch is still
not doing enough?

[shimazu, 2016-12-15 08:03:31]

On 2016/12/15 07:10:32, falken wrote:
> On 2016/12/15 07:07:25, shimazu wrote:
> > On 2016/12/15 06:59:41, falken wrote:
> > > shimazu@ I wrote this code but then realized maybe this is not sufficient
if
> > the
> > > register promise and sync event dispatch are using different mojo "pipes".
> > WDYT?
> > 
> > I think this patch makes sense, but I'm wondering how it's spec-ed.
> > BackgroundSyncManagerPtr should be associated with the event dispatcher if
the
> > ordering is defined by the spec.
> 
> I believe according to
> https://wicg.github.io/BackgroundSync/spec/#dom-syncmanager-register the
promise
> must be resolved before the sync events for the new registration are fired:
> 5. Resolve promise.
> 6. If the user agent is currently online, fire a sync event for
newRegistration.
> 
> Are you saying in order to ensure the ordering, we'd need to "associate"
> BackgroundSyncManagerPtr with the event dispatcher, i.e., this patch is still
> not doing enough?

Yes, this is not enough to ensure the ordering. Currently
ServiceWorkerEventDispatcher and BackgroundSyncManager use different pipes, so
the order between them has no guarantee.

[falken, 2016-12-15 08:12:08]

On 2016/12/15 08:03:31, shimazu wrote:
> On 2016/12/15 07:10:32, falken wrote:
> > On 2016/12/15 07:07:25, shimazu wrote:
> > > On 2016/12/15 06:59:41, falken wrote:
> > > > shimazu@ I wrote this code but then realized maybe this is not
sufficient
> if
> > > the
> > > > register promise and sync event dispatch are using different mojo
"pipes".
> > > WDYT?
> > > 
> > > I think this patch makes sense, but I'm wondering how it's spec-ed.
> > > BackgroundSyncManagerPtr should be associated with the event dispatcher if
> the
> > > ordering is defined by the spec.
> > 
> > I believe according to
> > https://wicg.github.io/BackgroundSync/spec/#dom-syncmanager-register the
> promise
> > must be resolved before the sync events for the new registration are fired:
> > 5. Resolve promise.
> > 6. If the user agent is currently online, fire a sync event for
> newRegistration.
> > 
> > Are you saying in order to ensure the ordering, we'd need to "associate"
> > BackgroundSyncManagerPtr with the event dispatcher, i.e., this patch is
still
> > not doing enough?
> 
> Yes, this is not enough to ensure the ordering. Currently
> ServiceWorkerEventDispatcher and BackgroundSyncManager use different pipes, so
> the order between them has no guarantee.

OK. Let's continue discussion in the bug.

[falken, 2016-12-16 01:22:40]

Description was changed from

==========
[BackgroundSync]: Resolve the register promise before firing sync events

Previously register promise resolution and sync event dispatch was
racy. This patch moves the sync event dispatch to after the register
promise callback is invoked.

BUG=671980
==========

to

==========
[BackgroundSync]: Resolve the register promise before firing sync events

Previously register promise resolution and sync event dispatch was
racy. This patch moves the sync event dispatch to after the register
promise callback is invoked.

BUG=671980
==========

[falken, 2016-12-16 01:22:48]

Closing this based on bug discussion.