1263304 - allow event.waitUntil() to be called asynchronously

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

We decided this was a good idea at the last face-to-face and chrome is getting ready to implement it.  We should do so soon as well to maintain compat.

https://github.com/slightlyoff/ServiceWorker/issues/771

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

The solution here should do these changes as well:

https://github.com/slightlyoff/ServiceWorker/issues/936#issuecomment-236027059
https://github.com/slightlyoff/ServiceWorker/issues/935#issuecomment-236026141

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I ran out of time and am not working on this currently.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Ben is going to work on this!

Comment 5

[Jake Archibald]

In the meantime, here's a polyfill https://github.com/jakearchibald/async-waituntil-polyfill

Comment 6

[Cătălin Badea (:catalinb)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> Ben is going to work on this!

I've been looking into this bug last week and would like to take it.

Comment 7

[Ben Hsu [:HoPang]]

> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> > Ben is going to work on this!
> 
> I've been looking into this bug last week and would like to take it.

Not actually start on it yet, please feel free to take it :)

Comment 8

[Cătălin Badea (:catalinb)]

Attachment: Move Service Worker MessageEvent dispatching code to ServiceWorkerPrivate.

This refactoring makes it easier to modify the behaviour of lifetime
extensions by having a single call path for all extendable events.

Comment 9

[Cătălin Badea (:catalinb)]

Attachment: Allow waitUntil() to be called asynchronously.

The behaviour is changed in two ways:
1. waitUntil() will now wait for each promise separately, even if one of
   them is rejected.
2. Extensions can be added asynchronously as long there is a pending
   waitUntil promise.

Comment 10

[Cătălin Badea (:catalinb)]

Attachment: Add a mochitest for asynchronously calling waitUntil().

Comment 11

[Cătălin Badea (:catalinb)]

Attachment: Move Service Worker MessageEvent dispatching code to ServiceWorkerPrivate.

This refactoring makes it easier to modify the behaviour of lifetime
extensions by having a single call path for all extendable events.

Comment 12

[Cătălin Badea (:catalinb)]

Attachment: Allow waitUntil() to be called asynchronously.

The behaviour is changed in two ways:
1. waitUntil() will now wait for each promise separately, even if one of
   them is rejected.
2. Extensions can be added asynchronously as long there is a pending
   waitUntil promise.

Comment 13

[Cătălin Badea (:catalinb)]

Attachment: Add a mochitest for asynchronously calling waitUntil().

Comment 14

[Cătălin Badea (:catalinb)]

Attachment: Fix wpt waitUntil() test.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8805162 [details] [diff] [review]
Move Service Worker MessageEvent dispatching code to ServiceWorkerPrivate.

Review of attachment 8805162 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.  I never liked all the "if service worker" stuff in the WorkerPrivate paths there.

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +478,5 @@
> +ServiceWorkerPrivate::SendMessageEvent(JSContext* aCx,
> +                                       JS::Handle<JS::Value> aMessage,
> +                                       const Optional<Sequence<JS::Value>>& aTransferable,
> +                                       UniquePtr<ServiceWorkerClientInfo>&& aClientInfo)
> +{

Please add an AssertIsOnMainThread() here to clarify where this runs.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8805163 [details] [diff] [review]
Allow waitUntil() to be called asynchronously.

Review of attachment 8805163 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.  Thanks!

::: dom/workers/ServiceWorkerEvents.h
@@ +71,5 @@
> +  bool
> +  WaitOnPromise(Promise& aPromise)
> +  {
> +    MOZ_ASSERT(mExtensionsHandler);
> +    return mExtensionsHandler->WaitOnPromise(aPromise);

nit:  Personally I prefer to keep all method bodies in the cpp file.  It help improve build speed, makes it easier to find code since everything is in the same spot, and typically doesn't effect perf at all.  But obviously we have lots of existing header code here, so this is just a nit.

@@ -58,5 @@
>    ~ExtendableEvent() {}
>  
>  public:
>    NS_DECL_ISUPPORTS_INHERITED
> -  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ExtendableEvent, Event)

Why is this no longer cycle collected?

@@ +84,5 @@
>  
> +  void
> +  SetKeepAliveHandler(ExtensionsHandler* aExtensionsHandler)
> +  {
> +    mExtensionsHandler = aExtensionsHandler;

MOZ_ASSERT(!mExtensionsHandler);

Also, please add a thread assertion.

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +182,5 @@
>  
> +class ExtendableEventCallback {
> +public:
> +  virtual void
> +  FinishedWithResult(bool aRejected) = 0;

Please use an enum like Resolved or Rejected instead of the bare boolean.

@@ +202,5 @@
> +  // which releases the token and prevents further extensions. By doing this,
> +  // we give other pending microtasks a chance to continue adding extensions.
> +
> +  nsMainThreadPtrHandle<KeepAliveToken> mKeepAliveToken;
> +  WorkerPrivate* mWorkerPrivate;

This can be const.

@@ +207,5 @@
> +  bool mWorkerHolderAdded;
> +
> +  // We don't actually care what values the promises resolve to, only whether
> +  // any of them were rejected.
> +  bool mRejected;

nit: To encourage good packing it would be nice to declare all the members that are less than a word at the end.

@@ +213,5 @@
> +
> +  // We start holding a self reference when the first extension promise is
> +  // added. As far as I can tell, the only case where this is useful is when
> +  // we're waiting indefinitely on a promise that's no longer reachable
> +  // and will never be settled.

Yes, which is tested in:

  register-wait-forever-in-install-worker.https.html

We added this behavior in bug 1273920 because that test started to fail when we GC'd the install event.

We could change this to let it GC and cancel the install.  We should have a spec discussion first, though.  If you want, can you file a follow-up and a spec issue?

@@ +255,3 @@
>      }
> +    if (!mSelfRef) {
> +      mSelfRef = this;

MOZ_ASSERT(!mPendingPromiseCount)

@@ +266,5 @@
> +  void
> +  ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override
> +  {
> +    MOZ_ASSERT(mWorkerPrivate);
> +    mWorkerPrivate->AssertIsOnWorkerThread();

Lets move the assertions into RemovePromise().  It makes it clear what thread we are on where we are manipulating the data structures.  Also, it will reduce some duplicate code.

@@ +292,5 @@
> +    return true;
> +  }
> +
> +  void
> +  SetCallback(ExtendableEventCallback* aCallback)

Please make this a param to the constructor instead of a separate setter.

@@ +299,5 @@
> +  }
> +
> +  void
> +  MaybeDone()
> +  {

Please add thread assertion in MaybeDone() since you manipulate data structures.

@@ +335,3 @@
>  
> +  void
> +  RemovePromise(bool aRejected)

Bare booleans are confusing at the call site.  Can you just define a small enum here instead?  So its RemovePromise(Resolved) or RemovePromise(Rejected).

@@ +338,2 @@
>    {
> +    MOZ_ASSERT(mPendingPromisesCount > 0);

Lets make this a MOZ_DIAGNOSTIC_ASSERT() so we get some coverage in beta.  It would be bad if the count ever got out of sync.

Also, lets MOZ_ASSERT(mSelfRef) and MOZ_ASSERT(mKeepAliveToken).

@@ +341,2 @@
>  
> +    if (--mPendingPromisesCount) {

nit: Can you separate the decrement from the if-statement conditional?  I find it easier to read when the statements are not combined.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8805164 [details] [diff] [review]
Add a mochitest for asynchronously calling waitUntil().

Review of attachment 8805164 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/workers/test/serviceworkers/async_waituntil_worker.js
@@ +32,5 @@
> +    }, 0);
> +  });
> +  e.respondWith(respondWithPromise);
> +  respondWithPromise.then(() => {
> +    e.waitUntil(clients.matchAll().then((cls) => {

Add a comment here that this is testing that we have until the next micro-task turn to call waitUntil().

::: dom/workers/test/serviceworkers/test_async_waituntil.html
@@ +60,5 @@
> +  ok(navigator.serviceWorker.controller, "Controlled");
> +
> +  yield SpecialPowers.pushPrefEnv({"set": [
> +    ["dom.serviceWorkers.idle_timeout", 0],
> +    ["dom.serviceWorkers.idle_extended_timeout", 299999]]});

Add a comment that this will make the service worker die immediately when there are no more promises keeping it alive, but will prevent it from timing out while there is a promise alive.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8805165 [details] [diff] [review]
Fix wpt waitUntil() test.

Review of attachment 8805165 [details] [diff] [review]:
-----------------------------------------------------------------

Can you provide some explanation about what this change is fixing?  Its not clear to me what was failing.

Also, I think we need to add test cases for:

1) async waitUntil() with an existing waitUntil()
2) async waitUntil() with an existing respondWith()
3) async waitUntil() fails after previous keep alive ends and a microtask turn passes

I know we have the mochitests, but its important to add these wpt tests so we get compat with chrome.

Thanks.

Comment 19

[Cătălin Badea (:catalinb)]

Attachment: Move Service Worker MessageEvent dispatching code to ServiceWorkerPrivate.

Comments addressed.

Comment 20

[Cătălin Badea (:catalinb)]

Attachment: Allow waitUntil() to be called asynchronously.

Addressed comments, with the exception of const WorkerPrivate - can't make
it const because the use of WorkerHolder.

Comment 21

[Cătălin Badea (:catalinb)]

Attachment: Add a mochitest for asynchronously calling waitUntil().

Comment 22

[Cătălin Badea (:catalinb)]

Attachment: Fix wpt waitUntil() test.

The test checks that if one of the waitUntil promises rejects, the install
fails immediately even if there is a pending promise - this is no longer the case.
The change causes the pending promise to be resolved eventually so we don't wait
indefinitely. I think the test is still valid as it now checks that a rejecting
promise will indeed fail the install event.

Comment 23

[Cătălin Badea (:catalinb)]

Attachment: Add wpt tests for async waitUntil().

I hope this covers everything.

Comment 24

[Cătălin Badea (:catalinb)]

(In reply to Ben Kelly [:bkelly] from comment #16)
> Comment on attachment 8805163 [details] [diff] [review] 
> >  public:
> >    NS_DECL_ISUPPORTS_INHERITED
> > -  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ExtendableEvent, Event)
> 
> Why is this no longer cycle collected?

ExtendableEvent now only holds a ref to KeepAliveHandler, which can only keep a self reference.
Thus, ExtendableEvent can't introduce cycles other than by being exposed to javascript, but that should
be covered in the super class.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8808717 [details] [diff] [review]
Add wpt tests for async waitUntil().

Review of attachment 8808717 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome.  Thanks!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f03cd54afbc1
Move Service Worker MessageEvent dispatching code to ServiceWorkerPrivate. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d3c6629a37
Allow waitUntil() to be called asynchronously. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c423ac723b6
Add a mochitest for asynchronously calling waitUntil(). r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ded216ab58b
Fix wpt waitUntil() test. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/15f15b242acd
Add wpt tests for async waitUntil(). r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/829e818cd4c3
Enable debugging statements in test_has_permissions.html r=me a=testonly

Comment 27

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39034131&repo=mozilla-inbound

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7687c3f7a82d
Backed out changeset 829e818cd4c3 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad94ec48c1f
Backed out changeset 15f15b242acd 
https://hg.mozilla.org/integration/mozilla-inbound/rev/167f1634a22a
Backed out changeset 3ded216ab58b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/41f7289afbe3
Backed out changeset 2c423ac723b6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a10fce0b6fc8
Backed out changeset b2d3c6629a37 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f4c3608bc0
Backed out changeset f03cd54afbc1 for causing frequent timeouts in test_has_permissions.html

Comment 29

[Cătălin Badea (:catalinb)]

Ok, so I think I managed to reproduce this on local by running test_error_reporting.html and test_has_permissions.html on repeat. The issue is that the worker gets stuck in the installed state due to the previous test's service worker. Using different scopes fixes the issue, investigating to see what actually happens.

Comment 30

[Cătălin Badea (:catalinb)]

(In reply to Cătălin Badea (:catalinb) from comment #29)
> Ok, so I think I managed to reproduce this on local by running
> test_error_reporting.html and test_has_permissions.html on repeat. The issue
> is that the worker gets stuck in the installed state due to the previous
> test's service worker. Using different scopes fixes the issue, investigating
> to see what actually happens.

That's not what happens :).

Apparently, the worker becomes activated but no statechange event is received because there are no ServiceWorker dom objects attached to the ServiceWorkerInfo. See bug 1317266.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85a6e5e89b42
Move Service Worker MessageEvent dispatching code to ServiceWorkerPrivate. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/509e98eec0ce
Allow waitUntil() to be called asynchronously. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecf72a6a2da
Add a mochitest for asynchronously calling waitUntil(). r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1554e11453e
Fix wpt waitUntil() test. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/b322495cb537
Add wpt tests for async waitUntil(). r=bkelly

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/85a6e5e89b42
https://hg.mozilla.org/mozilla-central/rev/509e98eec0ce
https://hg.mozilla.org/mozilla-central/rev/3ecf72a6a2da
https://hg.mozilla.org/mozilla-central/rev/d1554e11453e
https://hg.mozilla.org/mozilla-central/rev/b322495cb537

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

I think 'Test calling waitUntil in a different microtask without an existing extension throws' is wrong here, but I'll be fixing that as part of make-Promises-to-use-microtasks

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #33)
> I think 'Test calling waitUntil in a different microtask without an existing
> extension throws' is wrong here, but I'll be fixing that as part of
> make-Promises-to-use-microtasks

Why?  We explicitly are trying to support this behavior.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Because, if my interpretation of the wanted behavior in the spec is right, message event dispatching should behave as other lifetime extenders, 'Extend Service Worker Lifetime' should happen after event dispatch.
Given our current Promise implementation (which is not using microtasks, but end-of-task model), our code behave very differently, since we do http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/dom/workers/ServiceWorkerPrivate.cpp#450,458
and then call Promise callbacks at the end of the task, when the right model is to call the callbacks during event dispatch (assuming there are event listeners)

Comment 36

[Chris Mills [:cmills]]

I've updated the reference page browser support info to mention this:

https://developer.mozilla.org/en-US/docs/Web/API/ExtendableEvent#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/ExtendableEvent/waitUntil#Browser_compatibility

I've also added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#Service_Workers

Let me know if you think anything else needs doing. Thanks!