Navigation API: Always use a method tracker when navigating

[farre]

This solves the issue where we were starting asynchronous history traversals in #commit-a-navigate-event, this was in particular problematic for #resume-applying-the-traverse-history-step.

It also simplifies handling the case when a navigation was intercepted but no intercept handlers were provided.

Fixes #11814

• At least two implementers are interested (and none opposed):
  • This is implemented in a de-facto way in WebKit, Blink, and Gecko:
    • WebKit syncronously updates state before handlers run.
    • Blink syncronously updates state before running handlers.
    • Gecko syncronously updates state before handlers run
• Tests are written and can be reviewed and commented upon at:
  • Tests already exists in wpt/navigation-api (see comment below for tests that would fail in Gecko without the above unspecced behavior).
• Implementation bugs are filed:
  • This is already has an unspecified implementation in some way in WebKit, Blink, and Gecko.
• Corresponding HTML AAM & ARIA in HTML issues & PRs: N/A
• MDN issue is filed: N/A
• The top of this comment includes a clear commit message to use.

---

/nav-history-apis.html ( diff )

[farre]

It's worth to notice that all implementations handles this in some way or another, in various degrees of brokenness. This solution uses existing spec concepts only and makes sure that the expected variants (i.e the navigation has committed and communicated this through the now always present method tracker) holds.

[noamr]

It looks goot to me in general, asked @natechapin to take a look as well.

[farre]

I promised a list of affected tests that would fail if this wasn't already somthing that browsers did. This is that list:

/navigation-api/currententrychange-event/navigation-back-forward-same-doc.html
/navigation-api/focus-reset/autofocus.html
/navigation-api/focus-reset/basic.html
/navigation-api/focus-reset/multiple-intercept.html
/navigation-api/navigate-event/intercept-and-navigate.html
/navigation-api/navigate-event/intercept-navigation-back.html
/navigation-api/navigate-event/intercept-popstate.html
/navigation-api/navigate-event/intercept-same-document-history-back.html
/navigation-api/navigate-event/navigate-destination-after-detach.html
/navigation-api/navigate-event/navigate-destination-dynamic-index.html
/navigation-api/navigate-event/navigate-destination-getState-back-forward.html
/navigation-api/navigate-event/navigate-navigation-back-same-document-in-iframe.html
/navigation-api/navigate-event/navigate-navigation-back-same-document.html
/navigation-api/navigate-event/navigation-traverseTo-navigates-top-and-same-doc-child-and-cross-doc-child.html
/navigation-api/navigation-history-entry/after-detach.html
/navigation-api/navigation-history-entry/entries-after-bfcache-in-iframe.html
/navigation-api/navigation-methods/back-forward-multiple-frames.html
/navigation-api/navigation-methods/disambigaute-back.html
/navigation-api/navigation-methods/disambigaute-forward.html
/navigation-api/navigation-methods/disambigaute-traverseTo-back-multiple.html
/navigation-api/navigation-methods/disambigaute-traverseTo-forward-multiple.html
/navigation-api/navigation-methods/forward-to-pruned-entry.html
/navigation-api/navigation-methods/return-value/back-intercept-rejected.html
/navigation-api/navigation-methods/return-value/back-intercept.html
/navigation-api/navigation-methods/return-value/back.html
/navigation-api/navigation-methods/return-value/forward-intercept-rejected.html
/navigation-api/navigation-methods/return-value/forward-intercept.html
/navigation-api/navigation-methods/return-value/forward.html
/navigation-api/navigation-methods/return-value/traverseTo-intercept-rejected.html
/navigation-api/navigation-methods/return-value/traverseTo-intercept.html
/navigation-api/navigation-methods/return-value/traverseTo-repeated.html
/navigation-api/navigation-methods/return-value/traverseTo.html
/navigation-api/navigation-methods/traverseTo-after-adding-iframe.html
/navigation-api/navigation-methods/traverseTo-after-data-url.html
/navigation-api/navigation-methods/traverseTo-multiple-steps.html
/navigation-api/navigation-methods/traverseTo-same-document.html
/navigation-api/navigation-methods/traverseTo-with-cross-origin-in-history.html
/navigation-api/ordering-and-transition/back-same-document-intercept.html?no-currententrychange
/navigation-api/ordering-and-transition/back-same-document-intercept.html?currententrychange
/navigation-api/ordering-and-transition/back-same-document.html?currententrychange
/navigation-api/ordering-and-transition/back-same-document.html?no-currententrychange
/navigation-api/ordering-and-transition/currententrychange-before-popstate-intercept.html
/navigation-api/ordering-and-transition/navigate-in-transition-finished.html?no-currententrychange
/navigation-api/ordering-and-transition/navigate-in-transition-finished.html?currententrychange
/navigation-api/ordering-and-transition/navigate-intercept.html?currententrychange
/navigation-api/ordering-and-transition/navigate-intercept.html?no-currententrychange
/navigation-api/ordering-and-transition/navigate-same-document-intercept-reject.html?currententrychange
/navigation-api/ordering-and-transition/navigate-same-document-intercept-reject.html?no-currententrychange
/navigation-api/ordering-and-transition/navigate-same-document.html?currententrychange
/navigation-api/ordering-and-transition/navigate-same-document.html?no-currententrychange
/navigation-api/ordering-and-transition/reload-intercept-reject.html?no-currententrychange
/navigation-api/ordering-and-transition/reload-intercept-reject.html?currententrychange
/navigation-api/ordering-and-transition/reload-intercept.html?currententrychange
/navigation-api/ordering-and-transition/reload-intercept.html?no-currententrychange
/navigation-api/ordering-and-transition/reload-no-popstate.html
/navigation-api/per-entry-events/dispose-after-bfcache.html
/navigation-api/per-entry-events/dispose-same-document-intercept.html
/navigation-api/per-entry-events/dispose-same-document-navigate-during.html
/navigation-api/per-entry-events/dispose-same-document.html
/navigation-api/per-entry-events/dispose-skip-current-on-truncate.html
/navigation-api/precommit-handler/precommitHandler-traversal-window-stop-before-commit.html
/navigation-api/precommit-handler/precommitHandler-traverse.html
/navigation-api/precommit-handler/precommitHandler-uncancelable.html
/navigation-api/scroll-behavior/after-transition-basic.html
/navigation-api/scroll-behavior/after-transition-explicit-scroll.html
/navigation-api/scroll-behavior/after-transition-intercept-handler-modifies.html
/navigation-api/scroll-behavior/manual-basic.html
/navigation-api/scroll-behavior/manual-immediate-scroll.html
/navigation-api/scroll-behavior/manual-scroll-after-resolve.html
/navigation-api/scroll-behavior/manual-scroll-in-precommit-handler.html
/navigation-api/scroll-behavior/manual-scroll-repeated.html
/navigation-api/state/same-document-away-and-back-navigation-api.html

All of these tests fail if I remove the sync state and await committed promise steps from Gecko.

[noamr]

Isn't this a different way to add a microtask, similar to what we've discussed in #11845, except instead of adding a microtask at the end of the handler list we add a microtask after committed by waiting on it?

[farre]

The difference is subtle, but it doesn't change timing regardless of if you're using Promise.all or #wait-for-all. doing #wait-for-all([committedPromise, handlerPromise1, ..., handlerPromiseN], onnavigatesuccess, ...) will immediately call the onnavigatesuccess callback if all the promises are in the resolved state, whereas Promise.all([committedPromise, handlerPromise1, ..., handlerPromiseN]).then(onnavigatesuccess) won't.

[noamr]

OK, looking at this again I'm a bit confused about why this changes anything. The list of handlers for wait for all was [resolved-empty-promise] or [...handler-list] before, and now it's [resolved-committed-promise, ...handler-list]. Why does this change anything, and in particular the case when there is no handler list?

Also it feels a bit odd to create an API method tracker when there is no API. But perhaps answering the first question would help me understand the reasoning.

[farre]

There are two problems:

When we have a tracker, but no handlers

Imagine that we have the following:

await navigation.navigate("#1").finished;
navigation.onnavigate = e => {
  e.intercept(/* no handlers */);
};
await navigation.back().finished;

In the current state of the spec this will have an empty handler list and trigger the path that goes through the "traverse" casel in #commit-a-navigate-event . That switch branch will append resuming the #apply-the-history-step to session history traversal queue, which is a parallel queue. After that we will continue by waiting on the promisesList.

This is where trouble begins, regardless of if #wait-for-all or Promise.all is used, and the reason is the following: the success steps will call #resolve-the-finished-promise, which expects that there is a #navigation-api-method-tracker-committed-to-entry, but since we're setting that from #apply-the-history-step in parallel there is no guarantee that that has happened yet. But we do know that this step will do it, because that will eventually call #update-the-navigation-api-entries-for-a-same-document-navigation that calls #notify-about-the-committed-to-entry. #notify-about-the-committed-to-entry also resolves the #navigation-api-method-tracker-committed promise.

Let me stress the issue again for clarity: #navigation-api-method-tracker-committed promise can resolve at any time since it's resolved by an algorithm that runs in parallel.

By adding the #navigation-api-method-tracker-committed promise to the list of promises we wait for we can guarantee that the #navigation-api-method-tracker-committed-to-entry has been set before we call #resolve-the-finished-promise. Just waiting for a pre-resolved promise doesn't cut it, we need to wait for exactly #navigation-api-method-tracker-committed.

When we have handlers, but not a tracker

await navigation.navigate("#1").finished;
navigation.onnavigate = e => {
  e.intercept(/* no handlers */);
};
navigation.onnavigatesuccess = () => {
  // Do something that expects entry being committed
}
history.back();

The reasoning is roughly the same, but now we won't have a method tracker at all, but we'll end up in onnavigatesuccess before #resume-applying-the-traverse-history-step has executed (again because #resume-applying-the-traverse-history-step runs in parallel).

The major reason for creating an internal method tracker is that doing so re-uses existing functionality. The alternative would be to introduce a separate committed promise that we'd create when there is no tracker, and that we'd check and resolve at exactly the same places that we check and resolve the method trackers committed promise. Sure there is some overhead in the internal since it has a couple of fields that are essentially unused, but it simplifies the algorithm a lot.

If it helps, we could do a follow up of this where we just rename #navigation-api-method-tracker and instead call it just #navigation-tracker if that helps.

Summary

#resume-applying-the-traverse-history-step can run at any time after intercepting a history traversal regardless if there are intercept handlers or method trackers., This means that when waiting on the promises in promisesList we also need to have a barrier of sorts that wait for #resume-applying-the-traverse-history-step. This barrier is exactly the #navigation-api-method-tracker-committed in case of method trackers, and by introducing an internal tracker when we're not returning a method tracker we can merge the synchronization of the two cases.

[noamr]

Thanks for the explanation. So in short, waiting on the "committed" promise in particular synchronises the parallel steps for the traverse case. SGTM!