"apply the history step" fails if multiple sync navigations happened

[kalenikaliaksandr]

Steps to reproduce:

1. "URL and history update steps" is invoked for the first time. it updates active SHE and schedules "apply the history step".
2. (session history queue is not processed) "URL and history update steps" is invoked for the second time. it updates active SHE and schedules "apply the history step".
3. Now the history queue is eventually processed, but "apply the history step" does not reach step 12.4 as supposed to in sync navigation, because active SHE were changed twice.
4. some assertion in 12.5 fails depending of type of navigation.

[domenic]

@jakearchibald or @domfarolino, would you be able to help with this one? Or @kalenikaliaksandr, do you have any suggestions for a fix?

[kalenikaliaksandr]

as I see it could be solved by adding following steps in "URL and history update steps":

1. force processing of session history traversal queue (make sure "apply history step" of previous "URL and history update steps" took effect).
2. assert(current SHE = active SHE).

not sure how to describe the first step in spec terminology though.

[domfarolino]

3. Now the history queue is eventually processed, but "apply the history step" does not reach step 12.4 as supposed to in sync navigation, because active SHE were changed twice.

I assume you mean 12.4 is indeed at least reached, but just that its substeps are not run because the condition it poses fails?

4. some assertion in 12.5 fails depending of type of navigation.

Can you clarify whether you think the assertions fail during the first "apply the history step" invocation, or the second one? I think from reading only "apply the history step", I would think that assertions could fail during the first time "apply the history step" runs. For example:

history.pushState(…);
history.pushState(…);
history.pushState(…);

By the time the first "apply the history step" finally asynchronously runs for the first time, step 12's displayedEntry and targetEntry are both sufficiently different such that we would not satisfy the 12.4 condition:

If displayedEntry is targetEntry: [...]

3. Abort these steps.

... and we would accidentally continue to 12.5 and fail this assert:

Assert: targetEntry's step is displayedEntry's step + 1 and targetEntry's document state's ever populated is false.

But I don't think that "apply the history step" would ever even run for the first history.pushState() call above in the first place, because by the time finalize a same-document navigation gets scheduled in the first place, outdated instances of it will silently die, due to this condition:

If targetNavigable's active session history entry is not targetEntry, then return.

So the only time "apply the history step" would actually run during all of these history.pushState()s would be for the final pushState(), where things would work correctly, right?

---

With that said, even if my analysis is correct (which it might not be), we still might have an issue. Above, I propose that things are "fine" in the case where we do a bunch of synchronous back-to-back pushStates()s, but it's possible for the following sequence of events to happen:

1. Main thread: history.pushState()
2. SHT queue: "finalize same-document navigation" is invoked; step 2 succeeds --> proceed to "apply the history step"
3. Asynchronously later, on main thread: history.pushState(); history.pushState(); history.pushState()
4. SHT queue: "apply the history state" from the first invocation finally runs (because "finalize a same-document navigation" early-return condition was passed). We make it to state 12.4 which is supposed to catch and early-return sync navigations, but it fails because "active session history" (i.e., displayedEntry) has changed a number of times asyncly in between "finalize a same-document navigation" and "apply the history step"

This could cause the failed assertions that I think you're concerned about.

Do you agree with all of this?

[kalenikaliaksandr]

@domfarolino my bad, let me describe steps to reproduce in more details. let's assume all steps are execute within one thread:

• history.pushState()
• finalize a same-document navigation is executed.
• this condition is not true so we proceed:

  If targetNavigable's active session history entry is not targetEntry, then return.

• execute apply the history step until step 19 "Wait for completedNonchangingJobs to equal totalNonchangingJobs."
• by this time there is a task on event loop queued by apply the history step on step 12. this task is not yet executed.
• history.pushState()
• finalize a same-document navigation is not yet executed for the second history.pushState()
• now task queued by apply the history step on step 12 is executed.
• condition on step 12.4 does not evaluate to true because active SHE was changed twice by this time.
• following assertion fails:

  Assert: targetEntry's step is displayedEntry's step + 1 and targetEntry's document state's ever populated is false.

I assume you mean 12.4 is indeed at least reached, but just that its substeps are not run because the condition it poses fails?

yes, I meant this condition is evaluated to false.

Can you clarify whether you think the assertions fail during the first "apply the history step" invocation, or the second one? I think from reading only "apply the history step", I would think that assertions could fail during the first time "apply the history step" runs.

it fails during the first invocation.

... and we would accidentally continue to 12.5 and fail this assert:

correct.

But I don't think that "apply the history step" would ever even run for the first history.pushState() call above in the first place, because by the time finalize a same-document navigation gets scheduled in the first place, outdated instances of it will silently die, due to this condition:

it won't silently die if finalize a same-document navigation is executed between the first and second history.pushState() invocations.

With that said, even if my analysis is correct (which it might not be), we still might have an issue. Above, I propose that things are "fine" in the case where we do a bunch of synchronous back-to-back pushStates()s, but it's possible for the following sequence of events to happen:

yes, that is pretty precise description of what happens in my implementation.

Do you agree with all of this?

yes, I do. thank you for doing this thorough analysis.

[kalenikaliaksandr]

actually problem could be reproduced much easier using replaceState():

1. history.replaceState()
2. URL and history update steps updates active SHE and queues "Finalize a same-document navigation" with found entryToReplace = active SHE
3. session history traversal queue is not processed yet
4. history.replaceState()
5. URL and history update steps updates active SHE and queues "Finalize a same-document navigation" with found entryToReplace = active SHE
6. Finalize a same-document navigation queued queued by the first history.replaceState() is executed. it early returns on step 2 "If targetNavigable's active session history entry is not targetEntry, then return.".
7. Finalize a same-document navigation queued queued by the second history.replaceState() is executed, but it fails to find entryToReplace in targetEntries "Replace entryToReplace with targetEntry in targetEntries." because, active SHE was changed for another time by the second history.replaceState().