allow event.waitUntil() to be called asynchronously

RESOLVED FIXED in Firefox 53

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bkelly, Assigned: catalinb)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox53 fixed)

Details

(Whiteboard: [tw-dom] btpp-fixlater)

Attachments

(5 attachments, 7 obsolete attachments)

15.87 KB, patch
Details | Diff | Splinter Review
26.46 KB, patch
Details | Diff | Splinter Review
5.94 KB, patch
Details | Diff | Splinter Review
1.35 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
8.86 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
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
Whiteboard: [tw-dom] → [tw-dom] btpp-fixlater
Blocks: 1280413
Priority: -- → P2
Duplicate of this bug: 1280413
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
I ran out of time and am not working on this currently.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Ben is going to work on this!
Assignee: nobody → bhsu
Assignee

Comment 6

3 years ago
(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.
Flags: needinfo?(htsai)

Comment 7

3 years ago
> (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 :)
Assignee: bhsu → nobody
Flags: needinfo?(htsai)
Assignee

Updated

3 years ago
Assignee: nobody → catalin.badea392
Assignee

Comment 8

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

Comment 9

3 years ago
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.
Assignee

Comment 11

3 years ago
This refactoring makes it easier to modify the behaviour of lifetime
extensions by having a single call path for all extendable events.
Attachment #8804249 - Attachment is obsolete: true
Attachment #8805162 - Flags: review?(bkelly)
Assignee

Comment 12

3 years ago
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.
Attachment #8804250 - Attachment is obsolete: true
Attachment #8805163 - Flags: review?(bkelly)
Assignee

Comment 13

3 years ago
Attachment #8804251 - Attachment is obsolete: true
Attachment #8805164 - Flags: review?(bkelly)
Assignee

Comment 14

3 years ago
Posted patch Fix wpt waitUntil() test. (obsolete) — Splinter Review
Attachment #8805165 - Flags: review?(bkelly)
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.
Attachment #8805162 - Flags: review?(bkelly) → review+
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.
Attachment #8805163 - Flags: review?(bkelly) → review+
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.
Attachment #8805164 - Flags: review?(bkelly) → review+
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.
Attachment #8805165 - Flags: review?(bkelly) → review-
Assignee

Comment 20

3 years ago
Addressed comments, with the exception of const WorkerPrivate - can't make
it const because the use of WorkerHolder.
Attachment #8805163 - Attachment is obsolete: true
Assignee

Comment 22

3 years ago
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.
Attachment #8805165 - Attachment is obsolete: true
Attachment #8808714 - Flags: review?(bkelly)
Assignee

Comment 23

3 years ago
I hope this covers everything.
Attachment #8808717 - Flags: review?(bkelly)
Assignee

Comment 24

3 years ago
(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.
Attachment #8808714 - Flags: review?(bkelly) → review+
Comment on attachment 8808717 [details] [diff] [review]
Add wpt tests for async waitUntil().

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

Awesome.  Thanks!
Attachment #8808717 - Flags: review?(bkelly) → review+

Comment 26

3 years ago
Pushed by catalin.badea392@gmail.com:
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
Depends on: 1316811
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39034131&repo=mozilla-inbound
Flags: needinfo?(catalin.badea392)

Comment 28

3 years ago
Backout by cbook@mozilla.com:
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
Assignee

Comment 29

3 years ago
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.
Flags: needinfo?(catalin.badea392)
Assignee

Comment 30

3 years ago
(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.
Depends on: 1317266

Comment 31

3 years ago
Pushed by catalin.badea392@gmail.com:
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
Depends on: 1325381
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
(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.
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)
You need to log in before you can comment on or make changes to this bug.