1409979 - ExtendableEvent.waitUntil() shall be valid in microtask-checkpoint while event dispatch is still active

Status: VERIFIED FIXED

(Core :: DOM: Service Workers, enhancement, P3)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

According to "The steps to clean up after running script" in HTML spec[1], the test result of 'no-current-extension-different-task'[2] shall be 'OK' instead of 'InvalidStateError', because after DOMEvent dispatched at DispatchExtendableEventOnWorkerScope()[3], the microtask check-point shall be performed [1][4] before we set KeepAliveHandler to done[5].

Hence, I'd like to land a patch to revise the test case to 'OK' with expected failure specified in the .ini file and leave this bug open until bug 1193394 is fixed.

[1] https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script
[2] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/testing/web-platform/tests/service-workers/service-worker/resources/extendable-event-async-waituntil.js#18
[3] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/workers/ServiceWorkerPrivate.cpp#463
[4] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/bindings/CallbackObject.cpp#355-360
[5] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/workers/ServiceWorkerPrivate.cpp#471

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Similar comments available in bug 1263304 comment 35.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: ExtendableEvent.waitUntil() shall be valid during microtask-checkpoint.

1. Correct the expectation of the ExtendableEvent.waitUntil() according to the behavior of microtask-checkpoint explained in comment 0.
2. Add .ini file to leave this bug open after landing for tracking the fix of bug 1193394 on this problem.

Hi Ben,

May I have your review for this change?

Thanks!

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8920079 [details] [diff] [review]
(v1) Patch: ExtendableEvent.waitUntil() shall be valid during microtask-checkpoint.

I believe the test expectations are written to match the SW spec:

https://w3c.github.io/ServiceWorker/#wait-until-method

Chrome passes this test, so they have conformed to these expectations as well.

I think we probably need to fix the ServiceWorker code to expect the correct microtask behavior instead of changing the test.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Ben Kelly [:bkelly] from comment #3)
> I believe the test expectations are written to match the SW spec:
> 
> https://w3c.github.io/ServiceWorker/#wait-until-method

The argument is 
"
Note: If no lifetime extension promise has been added in the task that called the event handlers, calling waitUntil() in subsequent asynchronous tasks will throw.
"
I think this doesn't conflict to SW spec because permform-microtask-checkpoint shall be part of the lifetime of of the task that called the event handler according to HTML spec:
https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script

What both Firefox and Chrome respect for waitUntil() is the microtask-checkpoint defined in the event-loop:
https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model
instead of the one in #clean-up-after-running-script.

Besides, the statement in SW spec doesn't mention too much about forbidding the call if it is called from the a microtask.

> Chrome passes this test, so they have conformed to these expectations as
> well.
In Chrome, it seems intended to invalidate the call of waitUntil() if it comes from microtask-checkpoint to meet the test criteria:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp?l=145&rcl=c2665c0a9bfec6379c90d3ea36199ee6a3e9f024
In FF, Promise::PerformWorkerMicroTaskCheckpoint() is only done at #event-loop-processing-model, i.e. CycleCollectedJSContext::AfterProcessTask() which is expected to be changed in bug 1193394 to meet the spec in #clean-up-after-running-script.

> I think we probably need to fix the ServiceWorker code to expect the correct
> microtask behavior instead of changing the test.

I'd like to NI you and Olli again to see if we should fire a spec issue SW or fix this with similar solution that Chrome has done.

Comment 5

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

I have no idea about SW spec and what behavior it might want. But it is odd that Chromium has some MicrotasksScope::IsRunningMicrotasks checks there. Why? That looks like a bug.

Calling waitUntil during a microtask checkpoint right after the event listener call should be treated as if it was called during the event listener call, per spec.
"If the pending promises count is zero and the dispatch flag is unset, throw an "InvalidStateError" DOMException."
But dispatch flag _is_ set here, once we've fixed our Promise scheduling to use microtasks.
Right now we do throw because promises are resolved later.

The test doesn't test what the spec says, and implementations don't follow the spec.
And IMO the spec here is sane, but whatever reason implementations have decided to do something else.
In our case the bug comes of course from the broken Promise handling, but I have no idea why
Chromium has decided to explicitly inject that bug. Is it because they didn't pass the test we contributed, and thought the test was right?

Comment 6

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

And another point. If there are several event listeners, there is microtask checkpoint after each listener call. It wouldn't, IMO, make sense to not be able to call waitUntil right after the first event listener call but before the second one, yet be able to call waitUntil during the second event listener call.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Ok.  Let me contact the chrome team and see if they are ok changing this.  Its not really a spec issue, though, so I guess I'll just email them.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

You could also file the WPT test change as an upstream PR so we can get explicit review by the chrome team there.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Bevis, what do you think about these test changes:

https://github.com/w3c/ServiceWorker/issues/1213#issuecomment-338304564

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

I'll file a PR later for the WPT test change.
Thanks for the email and initiation of the discussion in https://github.com/w3c/ServiceWorker/issues/1213

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Can we just mark this test expected fail for now and then fix it once the spec discussion is complete?  I don't want this to block fixing our Promise implementation.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Sure, we can do this because this is not a blocker to bug 1193394.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

I think the current status in regards to this bug and https://github.com/w3c/ServiceWorker/issues/1213 is that after falken's pending changes in https://github.com/web-platform-tests/wpt/pull/15852 we fail:

in wpt/service-workers/service-worker/extendable-event-async-waituntil.https.html

promise_test(msg_event_test.bind(this, 'current-extension-expired-same-microtask-turn-extra'),
  'Test calling waitUntil in a microtask at the end of an existing extension promise handler succeeds (event is still being dispatched)');

promise_test(function(t) {
    var testBody = function(worker) {
      return with_iframe('./resources/respondwith-microtask-async-waituntil');
    }
    return runTest(t, 'respondwith-microtask-async-waituntil', testBody);
  }, 'Test calling waitUntil asynchronously inside microtask of respondWith promise (event is being dispatched).');

which corresponds to the test logic cases:

in a "message" handler:

      case 'current-extension-expired-same-microtask-turn-extra':
        waitPromise = Promise.resolve();
        event.waitUntil(waitPromise);
        waitPromise.then(() => { return async_microtask_waituntil(event); })
          .then(reportResultExpecting('OK'))
        break;

in a "fetch" handler:

    case 'respondwith-microtask-async-waituntil':
      response = Promise.resolve(new Response('RESP'));
      event.respondWith(response);
      response.then(() => { return async_microtask_waituntil(event); })
        .then(reportResultExpecting('OK'));
    break;

where the helper is:

function async_microtask_waituntil(event) {
  return new Promise((res, rej) => {
    Promise.resolve().then(() => {
      try {
        event.waitUntil(Promise.resolve());
        res('OK');
      } catch (error) {
        res(error.name);
      }
    });
  });
}

The basic issue appears to be that we're throwing when the dispatch flag is still set. The spec calls for us to throw if not active, where active is defined at https://w3c.github.io/ServiceWorker/#extendableevent-active as "An ExtendableEvent object is said to be active when its timed out flag is unset and either its pending promises count is greater than zero or its dispatch flag is set."

Our implementation implements the wait-another-microtask-turn heuristic, but MaybeDone doesn't handle the possibility that we're still in the initial microtask checkpoint that happens as part of event dispatch and where event dispatch is still happening. In general, we expect that when MaybeDone() is called in DispatchExtendableEventOnWorkerScope[1], event dispatch will be completed and the microtask checkpoint will have occurred (as long as their isn't JS on the stack), but it's more correct to do a check of the event's EventPhase() like FetchEvent::RespondWith() does[2].

1: https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/dom/serviceworkers/ServiceWorkerPrivate.cpp#427
2: https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/dom/serviceworkers/ServiceWorkerEvents.cpp#778

Comment 15

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1409979 - ExtendableEvent.waitUntil should account for dispatch flag r?asuth

This patch allows ExtendableEvent.waitUntil to be successfully called while
an ExtendableEvent is being dispatched. Initially, calls would fail if the
internal "pending promises count" reached zero during a microtask checkpoint
in between two event handler invocation tasks. This patch add the additional
condition that the dispatch flag must be unset for subsequent calls to fail.

Comment 16

[Perry Jiang [:perry] [no longer employee, use ni?]]

This is seen on a real website via https://m.indiamart.com's ServiceWorker, as reported by https://bugzilla.mozilla.org/show_bug.cgi?id=1593156#c0.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39add523bc26
ExtendableEvent.waitUntil should account for dispatch flag r=asuth

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/39add523bc26

Comment 19

[Alexandru Trif, Desktop Test Engineering [:atrif] [PTO 15-29 Aug, please ni? :mboldan]]

I reproduced the issue with Firefox 74.0a1 (20200113204142) on Windows 10x64 using STR from 1593156#c0 and 1593156#c8.
The issue is verified fixed with Firefox 74.0a1 (20200120154412) on Windows 10x64, macOS 10.15 and Ubuntu 18.04. I can no longer reproduce the issue using the mentioned STR.

Comment 20

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 9120886 [details]
Bug 1409979 - ExtendableEvent.waitUntil should account for dispatch flag r?asuth

Beta/Release Uplift Approval Request

• User impact if declined: https://m.indiamart.com won't work on GeckoView. It'll crash when trying to visit it after the first time. There are probably other websites that would be broken, given that this bug is not specific to a particular web application.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This changes the ExtendableEvent behavior to match that the specification says.
• String changes made/needed:

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9120886 [details]
Bug 1409979 - ExtendableEvent.waitUntil should account for dispatch flag r?asuth

Fixes a bug causing web compat issue for GV-based browsers. Approved for 73.0b9.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d28702c59e9a