Navigation API: Inconsistency between spec and tests about WaitForAll

[jnjaeschke]

What is the issue with the HTML Standard?

The spec uses a #wait-for-all when running the intercept handlers, which synchronously runs the success/failure steps. This is different from #waiting-for-all-promise, which queues success/failure into a promise.

However, location-href-intercept-reject.html seems to expect the navigateerror to run in a promise (it is queued after "promise microtask"), which I think is wrong. It should run before, because it's running sync with the rejected handler.

Chromium seems to use #waiting-for-all-promise.

cc @noamr @natechapin @farre

[noamr]

Hmm this doesn't sound right. Everything in "wait for all" works behind microtasks.
Wait for all calls PreformPromiseThen which calls HostEnqueuePromiseJob which queues a microtask.

[jnjaeschke]

I missed that -- you're right, it does not run sync. But it still runs in the same microtask, no? So the PerformPromiseThen should execute in step 12 of #commit-a-navigate-event, which is before location.href returns.

[noamr]

I missed that -- you're right, it does not run sync. But it still runs in the same microtask, no? So the PerformPromiseThen should execute in step 12 of #commit-a-navigate-event, which is before location.href returns.

No, not the same microtask... PerformPromiseThen queues a new microtask to run the success/failure steps. The tests are correct IMO

[smaug----]

WaitForAll queues one microtask in this case. https://webidl.spec.whatwg.org/#wait-for-all step 9.4
That is now in the queue, and that microtask will handle failureStep and fire navigateerror.
Now, execution in the test continues, and Promise.resolve().then(() => recorder.record("promise microtask")); runs. That is another microtask in the queue. Now, await happens and both microtasks run and navigateerror is fired before "promise microtask"

[noamr]

OK gotcha. So changing to https://webidl.spec.whatwg.org/#waiting-for-all-promise would fix this because it would require another then on the returned promise which would flip the order. Did I get it right?

[smaug----]

I think the spec is fine. That gives the least surprising behavior to web developers. Chromium needs to change to follow the spec.

[noamr]

I think the spec is fine. That gives the least surprising behavior to web developers. Chromium needs to change to follow the spec.

That's fine with me. @natechapin?

[natechapin]

I think the spec is fine. That gives the least surprising behavior to web developers. Chromium needs to change to follow the spec.

That's fine with me. @natechapin?

I think that's ok. However, I don't think chromium is currently set up to differentiate between wait for all and get a promise for waiting for all. This might be a slightly more involved change.

[noamr]

A few things about this:

• There is a note in the spec precisely about this.
• The only other place which uses "wait for all" is WebBluetooth (the navigation API handlers/precommitHandlers is the only place in the HTML spec that does this)
• There are a few other callers that reference "get a promise waiting for all"

Given the above, I think changing the spec would actually be better for everyone. Perhaps we can retire "wait for all" at some point and only have "get a promise waiting for all".

[farre]

I'd argue that using #wait-for-all produces the most consistent timing behavior, as @smaug---- notes. The fact that something that immediately rejects gets scheduled after something that hasn't even been scheduled yet is very surprising.

[noamr]

Counter argument: this is not consistent with how developers can create this flow themselves.

// This resolves to "after"
Promise.race([
  Promise.all([Promise.resolve("success")]),
  Promise.resolve("after")
])

// This resolves to "success"
Promise.race([
  Promise.resolve("success"),
  Promise.resolve("after")
])

Isn't it a bit weird that the platform emulates the second behavior in some cases?

[farre]

It's neither weird nor an emulation.

The major difference is that we're not working with promises, we're working with callbacks, which we can expect to not be bound by how Promise.all works. A callback based comparison with how this algorithm works would instead be more similar to:

const order = [];
// order becomes ["callback", "promise"]
Promise.all([
  Promise.all([Promise.reject().finally(() => order.push("callback")), 
               new Promise(resolve => setTimeout(resolve, 1000)).finally(() => order.push("timeout"))]),
  Promise.resolve().finally(() => order.push("promise"))
])

const order = [];
// order becomes ["callback", "promise", "timeout"]
Promise.all([
  Promise.all([Promise.resolve().finally(() => order.push("callback")), 
               new Promise(resolve => setTimeout(resolve, 1000)).finally(() => order.push("timeout"))]),
  Promise.resolve().finally(() => order.push("promise"))
])

I'm intentionally focusing on the rejection case, because in the Promise.all example above:

Promise.race([
  Promise.all([Promise.reject("reject")]),
  Promise.resolve("after")
])

would resolve to after, not capturing rejection at all. This makes this example not at all behave as the navigation callbacks do, since we'll never not report a failure.

If a developer would want to re-create this flow, they would basically just implement Promise.waitForAll. In particular, using #wait-for-all allows us to capture a potential error case sooner which enables users to have more control over the flow of navigations.

[noamr]

OK this is fine with me but I want to check in with @natechapin re. implementability in chrome given that this is the first case in the web platform where the difference is web observable.