Navigation API: upcoming API method tracker is not null if `navigate` is called from `onnavigateerror`

[jnjaeschke]

What is the issue with the HTML Standard?

I encountered this investigating why Gecko crashes in navigate-multiple-navigation-navigate.html. Gecko implements this assert as assert in the code, which gets hit when navigate #3 gets called.

As I understand, the assert is hit because the navigate to #2 aborts all ongoing navigations, which aborts navigate #1, which then synchronously runs navigate to #3, which maybe creates a new upcoming API tracker, which asserts because there already exists one (the one for the navigate to #2). This all happens before promoting the upcoming API tracker to ongoing in the inner navigate event firing algorithm.

cc @noamr @zcorpan @farre

[noamr]

Can you check if #11725 fixes this for you? Seems like looping in the abort algorithm might work for this.

[jnjaeschke]

(Sorry, forgot to name the issue 😅 )

I think the changes in #11725 don't change anything here. Still, firing the push/replace event aborts the ongoing event, which triggers a new navigate, which tries to set the upcoming API method tracker, before the original push/replace event code enters the inner navigate firing algorithm, which promotes the upcoming API method tracker to ongoing.

[noamr]

Yes I'm seeing that chromium doesn't assert this: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/navigation_api/navigation_api.cc;l=585;drc=eca8e9ffa43954933a88393cb15959eff13efde0

@natechapin do you have any context on this?
Otherwise I think this assert should be removed.

[jnjaeschke]

That doesn't sound right. Wouldn't this imply that the third navigate would set the upcoming API method tracker, would then get aborted by the second one as soon as its navigate() returns, and then the second one would continue with the API method tracker of the (aborted) third one? That would be a broken state.

I think this is a deeper issue. The abort loop allows to run script, but the internal navigation state is not "clean" -- therefore the script could mess up the internal navigation state, which is what happens in this test. Maybe we can solve this by moving the promoting step to run before the abort loop (but this could have other implications I don't see atm). Or we could move the current state into a temporary before running the abort loop (that sounds a bit hacky tho).

[natechapin]

Maybe we can solve this by moving the promoting step to run before the abort loop (but this could have other implications I don't see atm).

Yeah, this causes a bunch of WPT to assert in chromium, and is enough of an ordering change that I'm jumpy about it.

Or we could move the current state into a temporary before running the abort loop (that sounds a bit hacky tho).

Agreed that it's a bit hacky, but it fixes the issue. This WIP CL transfers the tracker to a tempoary and adds the assert to chromium, and it passes WPT. Though it makes promote an upcoming API method tracker to ongoing look strange enough that maybe it should be inlined at its sole invocation.

[noamr]

Maybe we can solve this by moving the promoting step to run before the abort loop (but this could have other implications I don't see atm).

Yeah, this causes a bunch of WPT to assert in chromium, and is enough of an ordering change that I'm jumpy about it.

Or we could move the current state into a temporary before running the abort loop (that sounds a bit hacky tho).

Agreed that it's a bit hacky, but it fixes the issue. This WIP CL transfers the tracker to a tempoary and adds the assert to chromium, and it passes WPT. Though it makes promote an upcoming API method tracker to ongoing look strange enough that maybe it should be inlined at its sole invocation.

I don't have enough context to tell if this works for all cases but sounds like it is an improvement @jnjaeschke?
If so, I'm happy to make this into a spec PR.

[jnjaeschke]

Thanks @noamr, I think this should fix the immediate issue, and our implementation did not break any other WPTs.

[noamr]

Thanks @noamr, I think this should fix the immediate issue, and our implementation did not break any other WPTs.

#11762