Abort in-flight navigation API traversals to pruned entries - #12846
Conversation
|
I asked Claude if this matches Chromium: I checked this against the Chromium implementation. The core of the change matches, but three of the four pieces don't, and one of them introduces a fresh assertion failure. In Chromium the whole mechanism lives in one place, if (IsBackForwardOrRestore(params->frame_load_type) &&
params->event_type == NavigateEventType::kFragment &&
!keys_to_indices_.Contains(key)) {
TraverseCancelled(key, mojom::blink::TraverseCancelledReason::kAbortedBeforeCommit);
return DispatchResult::kAbort;
}
The new bail in "fire a traverse The dispose-time rejections have no counterpart
They also make the observable error non-deterministic. If the entry is pruned before step 12.2 of "perform a navigation API traversal" runs, that step already rejects:
...and that matches Chromium's browser-side The dispose-time rejections can trigger an assertionRejecting the tracker does not cancel the steps already appended by step 12 of "perform a navigation API traversal". Once the dispose loop has rejected and cleaned up the tracker, those steps still run, and step 12.2 (quoted above) or 12.3 unconditionally rejects the same apiMethodTracker a second time. Step 3 of "reject the finished promise" then runs "clean up a navigation API method tracker", whose step 3.3 asserts:
...which is now false, because the first rejection removed it. So this can replace one assertion failure with another. The new step in "fire a traverse Descendant navigables aren't abortedStep 12.7.2 of "apply the history step" ignores the return value of "fire a traverse Chromium returns Removing the assertion is weaker than Chromium
Incidentally, the dispose-loop step is dead in the "replace" branch: the disposed entry is the current one, and "perform a navigation API traversal" returns early for the current key, so no tracker can exist for it. Suggested shapeKeep the bail in "fire a traverse Testsweb-platform-tests/wpt#62214 is the existing (Only Chrome Canary was tested here.) |
|
I think we can go with the suggestions of Claude here and potentially follow up with the dispose-time rejection, (as an additional hardening), that could come with its own test(s). By just having the fire navigate event abortion we would keep the same scope of change as with #12574 too. So the suggestion:
is fine to me. The 12.7.2 steps it is referring to may need to be re-written somehow. Because, should we really be firing navigate events in other frames if we are to ultimately fail anyhow? Wild speculation: Maybe we could somehow track that "this was the navigable that started the history traversal, and if that fails, bail" |
b464905 to
57b0b62
Compare
|
since 12.7.2 is mentioned - it's actually unreachable in the current specification, see: #12859 |
When a traversal is in flight and an intervening same-document navigation (e.g.,
history.pushState()) prunes the target entry fromnavigation.entries(), the spec previously hit an assertion failure during same-document entry updates.This change aligns with chromium implementation by:
AbortErrorwhen theirtarget entry is disposed or missing during traverse navigate event firing.
updates and failing silently, relying on the above rejection to notify the developer.
Closes #12574.
navigation-api/navigation-methods/forward-to-pruned-entry.htmlalso tests this(See WHATWG Working Mode: Changes for more details.)
/browsing-the-web.html ( diff )
/nav-history-apis.html ( diff )