Navigation API: Navigation's ongoing navigate event is not null in #inner-navigate-event-firing-algorithm

[farre]

What is the issue with the HTML Standard?

In step 25 of #inner-navigate-event-firing-algorithm we assert that #ongoing-navigate-event is null, but I don't think this is necessarily true.

The current partial implementation in Gecko asserts at that step for the following test:

<script>
  addEventListener(
    "load",
    async () => {
      navigation.onnavigate = () => {};
      history.pushState(1, null, "#1");
      history.pushState(1, null, "#2");
    },
    { once: true }
  );
</script>

Looking at Chromium source, we see that #inform-the-navigation-api-about-aborting-navigation is called in NavigationApi::DispatchNavigateEvent, and indeed if I add a call to #inform-the-navigation-api-about-aborting-navigation in Gecko's implementation of #inner-navigate-event-firing-algorithm the assert obviously stops being a problem.

Thing is that I'm not entirely sure that this is the correct solution, but as far as I've been able to glean from the spec, there is nowhere a call to #abort-the-ongoing-navigation when doing two consecutive calls to history.pushState.

[smaug----]

It is unclear to me what happens in Chrome if the inform-the-navigation-api-about-aborting-navigation which is triggered from the second pushState triggers third pushState call via navigateerror event listener. Stack then unwinds an continues executing, but in which state are we in then? Edit: Chrome's renderer process crashes

@noamr

[natechapin]

I believe the divergence between chromium and the spec is that chromium runs #set-the-ongoing-navigation as part of the #inner-navigate-event-firing-algorithm. According to a TODO I left myself, the spec is right and chromium is wrong.

I think what's missing in the spec is that #set-the-ongoing-navigation should be included in #shared-history-push/replace-state-steps?

It is unclear to me what happens in Chrome if the inform-the-navigation-api-about-aborting-navigation which is triggered from the second pushState triggers third pushState call via navigateerror event listener. Stack then unwinds an continues executing, but in which state are we in then? Edit: Chrome's renderer process crashes

I think this is only possible because navigateerror is fired synchronously inside #abort-the-ongoing-navigation? So we could fire navigateerror on a microtask instead, or early-exit somewhere (probably in #inner-navigate-event-firing-algorithm) if the ongoing navigate event is not null? I'd lean toward the early-exit, since changing navigateerror timing all but guarantees that the navigateerror will fire after the next navigate.

[domenic]

Nice catch. The root cause here is that the navigation API's notion of "ongoing navigation" is always async (i.e., it takes at least until a microtask checkpoint for the finished promise to settle). Whereas, pushState() is completely sync. Mostly this mismatch is OK, but in the case of overlapping navigations and transitions we can get into trouble

For this reason, @natechapin's suggestion.

I think what's missing in the spec is that #set-the-ongoing-navigation should be included in #shared-history-push/replace-state-steps?

doesn't quite work. Per #6927 (comment) , to match browsers, we want the results for

location.href = "foo.html";
history.pushState(1, null, "#1");

to be a navigation to #1 and then a navigation to foo.html. (This is the "XDN then SDN" case.) The way the spec currently accomplishes this is by having history.pushState() not touch the "ongoing navigation" machinery at all.

If we implemented Nate's suggestion, then step 21.2 of the navigate algorithm for foo.html would notice that the ongoing navigation has changed, and abort the navigation to foo.html. (Which sounds like pretty sensible behavior to me! But is not at all what browsers do, so, oh well.)

---

The fix @farre mentions

Looking at Chromium source, we see that #inform-the-navigation-api-about-aborting-navigation is called in NavigationApi::DispatchNavigateEvent, and indeed if I add a call to #inform-the-navigation-api-about-aborting-navigation in Gecko's implementation of #inner-navigate-event-firing-algorithm the assert obviously stops being a problem.

is one reasonable way of patching over the inconsistency. It basically says "for the purposes of the navigation API and its promises and events, let's abort the ongoing navigation". (Whereas for the purposes of the rest of the navigation stack, we don't abort.) Although, it sounds like we need some further patches to avoid a crash.

I think this direction is probably correct. I started mentally sketching some version where we allow multiple ongoing navigate events, so that developers could get success signals for both pushState()s in the OP. But it quickly became nightmareish. Ultimately, the navigation API asks the web developer to buy into a paradigm where there is only one navigation ongoing at a time, and overlapping ones are treated as errors.

It's true that some of those "errors" (like SDN-then-SDN or XDN-then-SDN) result in two history entries, and some (like XDN-then-XDN) only result in one. But that's just a reflection of the fact that browsers' existing handling of overlapping navigations and traversals is very complicated. And it's further evidence that developers should really prefer queuing up navigations, using await navigation.navigate(...).finished, to avoid this complexity.

---

Regarding @smaug----'s case,

navigation.onnavigate = () => {};
navigation.onnavigateerror = e => {
  if (e.destination.url.endsWith("#1") {
    history.pushState(1, null, "#3");
  }
};
history.pushState(1, null, "#1");
history.pushState(1, null, "#2");

I can't tell what the full event sequence @natechapin is proposing with the early-exit fix is. I think a reasonable sequence would be:

• navigate to #1,
• navigateerror to #1
• navigate to #3
• navigateerror to #3
• navigate to #2
• navigatesuccess to #2

with the intuition being that you always fire navigateerror on the navigation that's getting aborted before you fire navigate for the navigation that's replacing it.

But this stuff is stupid-complicated and I'm willing to be convinced of other ideas that make our lives simpler. Because as I said above, overlapping navigations are just not the happy path.

[farre]

I realised one thing. In this case we have a call to pushState, requiring #inform-the-navigation-api-about-aborting-navigation to be called to ensure #ongoing-navigate-event is null. But we have non-sync callers as well, and we really want the assertion to mean something there. So maybe the call to #inform-the-navigation-api-about-aborting-navigation should really happen in #fire-a-push/replace/reload-navigate-event before calling #inner-navigate-event-firing-algorithm