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

NEW
Unassigned

Status

()

enhancement
P3
normal
2 years ago
3 months ago

People

(Reporter: bevis, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
Similar comments available in bug 1263304 comment 35.
See Also: → 1263304
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.
Flags: needinfo?(bugs)
Flags: needinfo?(bkelly)
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?
Flags: needinfo?(bugs)
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
Flags: needinfo?(bkelly)
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.
Assignee: bevistseng → nobody

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

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.