async waitUntil microtask issue

[wanderview]

As you may be aware the firefox Promise implementation has not been using proper microtasks and we're trying to fix that. Part of that effort uncovered an issue with a test for async waitUntil():

https://bugzilla.mozilla.org/show_bug.cgi?id=1409979

Apparently the spec is correct, but the test follows an incorrect interpretation based on our bogus microstask handling. Chrome matched the test expectation by doing this workaround:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp?l=145&rcl=c2665c0a9bfec6379c90d3ea36199ee6a3e9f024

We would like to correct the test to follow the spec and real microtask definition, but wanted to make sure everyone was ok with that since chrome matches behavior.

We think this example from the original issue is incorrect: #1039 (comment)

And this WPT test condition should return OK and not throw an InvalidStateError:

https://github.com/w3c/web-platform-tests/blob/a16738c524a65946b0ff7df6be37ae076ff5e860/service-workers/service-worker/resources/extendable-event-async-waituntil.js#L20

[mfalken]

I'm OK with changing the test as I suspect you're right about the spec. Just not sure why we got it wrong in the first place. Did we all just read the spec wrong when discussing #1039 ? It's very hard for me to read the Service Worker spec and DOM spec and piece together what the expected behavior is.. easier to understand via WPT tests and examples.

[aliams]

I also agree with changing the test. Thanks for spotting this @wanderview!

[wanderview]

Actually, now I'm confused again. I would think step 5 here would mean we throw since we should queue the microtask to decrement the number of pending promises:

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

[wanderview]

Olli suggests that the test should be saying "OK" there because the dispatch flag is set, I think:

https://bugzilla.mozilla.org/show_bug.cgi?id=1409979#c4

Because we are running the test from an event handler.

Would we throw InvalidStateError if we tried to do it from a setTimeout()?

I'm not sure I understand why the dispatch flag matters here.

[smaug----]

Yes, setTimeout creates its own task, so the event isn't being dispatched during that time.

[wanderview]

I think the intent of checking the dispatch flag is to allow the initial waitUntil() to be called on first dispatch of the functional event.

[wanderview]

I guess the setTimeout() case is already tested here:

https://github.com/w3c/web-platform-tests/blob/a16738c524a65946b0ff7df6be37ae076ff5e860/service-workers/service-worker/resources/extendable-event-async-waituntil.js#L17

So I guess we can just mark this test case as returning OK:

https://github.com/w3c/web-platform-tests/blob/a16738c524a65946b0ff7df6be37ae076ff5e860/service-workers/service-worker/resources/extendable-event-async-waituntil.js#L20

Maybe with a comment that its ok since the dispatching flag is set.

If we really wanted to we could add a 3rd test case that adds a setTimeout() before the async_microtask_waituntil to show that it should fail if the dispatch flag is not set.

[mfalken]

Just a question: is the dispatch flag set whenever you are in a microtask "chained" to the original event dispatch? e.g., does this throw?

onfetch = e => {
  Promise.resolve().then(_ => Promise.resolve()).then(_ => Promise.resolve()).then(_ => e.waitUntil(p));
};

Similarly how about this one:

onfetch = e => {
  e.waitUntil(some_promise);
  some_promise.then(_ => Promise.resolve()).then(_ => Promise.resolve()).then(_ => e.waitUntil(another_promise));
};

[smaug----]

If more microtask stuff is added during a microtask checkpoint, they all are handled during that checkpoint. Microtasks are synchronous from browser point of view.
See the while loop in https://html.spec.whatwg.org/#perform-a-microtask-checkpoint

[jungkees]

So I guess we can just mark this test case as returning OK:

https://github.com/w3c/web-platform-tests/blob/a16738c524a65946b0ff7df6be37ae076ff5e860/service-workers/service-worker/resources/extendable-event-async-waituntil.js#L20

I think the current test is what we settled on. I.e., it should throw in that case.

We think this example from the original issue is incorrect: #1039 (comment)

I think #1039 (comment) is the expected behavior we agreed. That is, we wanted to disallow a further lifetime extension in any async task when there's no holding extension promise.

Olli suggests that the test should be saying "OK" there because the dispatch flag is set, I think:

I don't think the dispatch flag is set when the waitUntil is called in the microtask. The microtask checkpoint is performed after the event-dispatcher task is finished.

[smaug----]

The microtask checkpoint is performed after the event-dispatcher task is finished.

No it isn't, except if there is some other JS on stack too (like, if the event is dispatched from JS).
Microtask checkpoint happens at the end of the outermost script execution and at the end of a task
(+ couple of special cases, like right before <script> execution)

[jungkees]

I thought if dispatch is invoked in a task, any microtasks queued in the task wouldn't be scheduled until the dispatch steps run to completion. Am I understanding it incorrectly?

[smaug----]

If there isn't any other script on stack, microtask checkpoint happens at the end of the each event listener call. So, if there are several listeners for an event, there will be several microtask checkpoints per event dispatch.

[jungkees]

I think those microtask checkpoints are preceded by the completion of the dispatch algorithm (where the dispatch flag's unset) regardless how many other tasks would be queued in the interim.

[bevis-tseng]

No, the dispatch flag is unset after microtask checkpoints of event listeners are performed, according to https://dom.spec.whatwg.org/#dispatching-events:

1. the dispatch flag is set at step 1 and is unset and step 14.
2. the listener callbacks could be invoked in step 12.4 and step 13.6.
3. in https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke, each callback invocation (step 16.2 of [1]) contains a microtask checkpoint specified in [2].
   [1] https://heycam.github.io/webidl/#call-a-user-objects-operation
   [2] https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script

[jungkees]

3. in https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke, each callback invocation (step 16.2 of [1]) contains a microtask checkpoint specified in [2].

I didn't realize the execution of each listener callback consumes the microtasks. That considered, I agree the WPT test (calling waitUntil() in a microtask) returns OK and doesn't throw an InvalidStateError. Thanks for the pointers. Sorry for my confusion.

[jungkees]

Reviewing the other cases again, I'm confused about the current-extension-expired-same-microtask-turn-extra case. Shouldn't it return OK because all the microtasks in the example will be consumed before the dispatch flag is unset?

[wanderview]

Reviewing the other cases again, I'm confused about the current-extension-expired-same-microtask-turn-extra case. Shouldn't it return OK because all the microtasks in the example will be consumed before the dispatch flag is unset?

Per the spec language I think you are right.

Looking at why our implementationdoesn't do that, we end up removing the "keep alive token" via a microtask:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#386
https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#356

This then means when the next WaitUntil() is called in a later microtask in the same dispatching task the "keep alive token" is gone.

I was thinking maybe we could make it respect the dispatch flag here, but the "keep alive token" is created on another thread. It would be very difficult to do this in firefox.

What would people think about adding another flag that says "extension promises were added and now all consumed"? If that flag is set, then we don't allow new extension promises to be added even if we are still dispatching the event. This might meet our original intent here?

[jakearchibald]

I think our spec wording is a little wrong here.

Upon fulfillment or rejection of f, queue a microtask to run these substeps:

Fulfillment already queues a microtask, so we may be doing something redundant. Unless we're intentionally adding a microtask to a microtask, which I don't think we are.

I'm struggling to get my head around this. I don't think this should work:

const p = doSomething();
event.waitUntil(p);
p.then(() => event.waitUntil(doSomethingElse()));

Because the pending promises count will be 0 when event.waitUntil is called. This is because the spec's microtask is in the queue before the p.then handler. Whereas:

const p = doSomething();
p.then(() => event.waitUntil(doSomethingElse()));
event.waitUntil(p);

…would work, since the p.then handler is in the queue before the spec's microtask.

I guess it's a bit of a gotcha, but it'll fail consistently.

[jakearchibald]

F2F: My comment above is wrong. The double microtask is intentional. The use of the dispatch flag is intentional and catches an edge case with multiple event listeners:

addEventListener('fetch', event => {
  event.waitUntil(Promise.resolve());
});

addEventListener('fetch', event => {
  // This should work, and it's the dispatch flag that makes it work.
  event.waitUntil(whatever());
});

Action: Review the tests for this.

https://github.com/w3c/web-platform-tests/blob/a16738c524a65946b0ff7df6be37ae076ff5e860/service-workers/service-worker/resources/extendable-event-async-waituntil.js#L20 - waitUntil shouldn't throw here.

[aliams]

Does this not also affect https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/resources/extendable-event-async-waituntil.js#L33

and

https://github.com/w3c/web-platform-tests/blob/master/service-workers/service-worker/resources/extendable-event-async-waituntil.js#L66

[jungkees]

Thanks for clarifying with the tests, @aliams. I think we mostly agree on the current spec behavior for waitUntil(), but would like to clarify some points:

I was thinking maybe we could make it respect the dispatch flag here, but the "keep alive token" is created on another thread. It would be very difficult to do this in firefox.

@wanderview, do you still have this issue or is it implementable? I thought of using event's extend lifetime promises for checking "extension promises were added" condition in your comment, but we still don't solve #1213 (comment) case without the dispatch flag.

Having thought on it, (throwing when the dispatch flag is unset && count == 0) explains the desired behavior well I think. That is, the event is basically extendable during its own task turn and during when the extension opportunity isn't closed.

Other than that, I'm a bit uncomfortable with the following cases. What do you think about them?

const p = async_task_waituntil();
event.waitUntil(p);
p.then(() => { Promise.resolve().then(() => event.waitUntil(q)) }); // Throw.

This throws since the count decrement microtask queued by waitUntil step 5 will run before the microtask queued by p.then callback.

const p = async_task_waituntil();
p.then(() => { Promise.resolve().then(() => event.waitUntil(q)) }); // OK.
event.waitUntil(p);

This runs OK since the count decrement microtask queued by waitUntil step 5 will run after the microtask queued by p.then callback.

[aliams]

Is that to say that the test calling waitUntil in a different microtask without an existing extension should still throw. And that the test calling waitUntil at the end of the microtask turn still throws too? I believe due to the dispatch flag, these should actually be succeeding.

[aliams]

@jungkees, are these what the tests would look like for what you are describing in your examples?

1.  waitPromise = async_task_waituntil(event);
    event.waitUntil(waitPromise);
    waitPromise.then(() => {
        return Promise.resolve().then(() => sync_waituntil(event))
    }).then(reportResultExpecting('InvalidStateError'));

2.  waitPromise = async_task_waituntil(event);
    waitPromise.then(() => {
        return Promise.resolve().then(() => sync_waituntil(event))
    }).then(reportResultExpecting('OK'));
    event.waitUntil(waitPromise);

[jungkees]

@aliams, yes, the tests look equivalent to the examples I gave above. I was talking about the cases where the promise (waitPromise in your example) resolves in an async task. The result of the waitUtil called in the extra microtask turn depends on whether the microtask for decrementing the pending lifetime promise count was called or not. The question was whether this would be expected behaviors?

[mfalken]

Sorry no that can't be right. p could take a long time and it doesn't make sense for the event dispatch flag to still be set in that case. I wonder if it matters that the test is using p = Promise.resolve() instead of a longer promise like one that resolves after setTimeout.

[mfalken]

To anyone interested, I have a patch with proposed test changes in https://chromium-review.googlesource.com/c/chromium/src/+/1524393.

[wanderview]

Sorry no that can't be right. p could take a long time and it doesn't make sense for the event dispatch flag to still be set in that case. I wonder if it matters that the test is using p = Promise.resolve() instead of a longer promise like one that resolves after setTimeout.

FWIW, I think this analysis here is correct and your test looks good. Thanks!

[annevk]

I wonder if it matters

As far as I know you cannot trigger this situation in other ways (except via similar promise APIs that have near-identical semantics, if those exist). Certainly not with setTimeout.

[wanderview]

I think in theory something like a fetch() to a data URL could resolve in a single microtask like Promise.resolve(). It wouldn't be guaranteed, though, of course.

[asutherland]

Thanks for the clarification and the test changes! I think the initially referenced Firefox bug https://bugzilla.mozilla.org/show_bug.cgi?id=1409979 and the failures in that WPT PR for Firefox are now just about Firefox's ExtendableEvent.waitUntil() failing to check the dispatch flag.

[annevk]

@wanderview no, that should at the very least queue a task from the fetch layer.

[mfalken]

Thanks all! the WPT tests have been updated. The tricky part was indeed that some tests were using Promise.resolve() which meant the event dispatch flag was still set when the test intended to test the pending promises count instead. I've added test cases for those now.

I think we can now close this.