According to "The steps to clean up after running script" in HTML spec, the test result of 'no-current-extension-different-task' shall be 'OK' instead of 'InvalidStateError', because after DOMEvent dispatched at DispatchExtendableEventOnWorkerScope(), the microtask check-point shall be performed  before we set KeepAliveHandler to done. 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.  https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script  http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/testing/web-platform/tests/service-workers/service-worker/resources/extendable-event-async-waituntil.js#18  http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/workers/ServiceWorkerPrivate.cpp#463  http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/bindings/CallbackObject.cpp#355-360  http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/workers/ServiceWorkerPrivate.cpp#471
Similar comments available in bug 1263304 comment 35.
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!
Attachment #8920079 - Flags: review?(bkelly)
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.
Attachment #8920079 - Flags: review?(bkelly) → review-
(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.
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?
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.
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.
You could also file the WPT test change as an upstream PR so we can get explicit review by the chrome team there.
Bevis, what do you think about these test changes: https://github.com/w3c/ServiceWorker/issues/1213#issuecomment-338304564
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
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.
Sure, we can do this because this is not a blocker to bug 1193394.
Summary: ExtendableEvent.waitUntil() shall be valid in microtask-checkpoint → ExtendableEvent.waitUntil() shall be valid in microtask-checkpoint while event dispatch is still active
You need to log in before you can comment on or make changes to this bug.