Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What should history.back() do when sync navigations are involved? #6773

Closed
domenic opened this issue Jun 15, 2021 · 11 comments · Fixed by #6315
Closed

What should history.back() do when sync navigations are involved? #6773

domenic opened this issue Jun 15, 2021 · 11 comments · Fixed by #6315
Labels
interop Implementations are not interoperable with each other topic: history

Comments

@domenic
Copy link
Member

domenic commented Jun 15, 2021

Consider the following scenario, due to @natechapin:

// start on index.html
location.hash = "#foo";
history.back();
location.hash = "#bar";

Where do you end up?

Apparently in Firefox you end up on index.html#foo and in Chrome you end up on index.html. Probably similar issues occur for history.pushState().

@jakearchibald, I suspect this has a more-correct answer in the framework you're working on... any thoughts on what it should be?

@domenic domenic added topic: history interop Implementations are not interoperable with each other labels Jun 15, 2021
@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 16, 2021

In my current model:

  1. History is ['index.html'] and current step is 0.
  2. Synchronously update to index.html#foo, and queue TaskA to update the history tree.
  3. Queue TaskB for back().
  4. Synchronously update to index.html#bar, and queue TaskC to update the history tree.
  5. TaskA (index.html#foo):
    1. Update history tree to ['index.html', 'index.html#foo'].
    2. index.html#foo is given a step of 1.
    3. Update current step to 1.
    4. Top level doesn't change, because its current active item index.html#bar has a 'pending' step (as in, the synchronous navigation hasn't been resolved).
  6. TaskB (back()):
    1. Update current step to 0.
    2. Top level doesn't change, because its current active item index.html#bar has a 'pending' step (as in, the synchronous navigation hasn't been resolved).
  7. TaskC (index.html#bar):
    1. Update history tree to ['index.html', 'index.html#bar'] (index.html#foo is removed by clearing forward history)
    2. index.html#bar is given a step of 1.
    3. Update current step to 1.
    4. Top level doesn't need to change, because it's already on index.html#bar.

I feel pretty good about that result (I know, I'm biased) as it's the same result as running those steps one by one. But ugh, if neither Chrome or Firefox do that, does that mean I should change it?

Fwiw, I wouldn't have guessed either the Chrome result or the Firefox result, I can't really make sense of it.

I wouldn't have been surprised if the history result was ['index.html', 'index.html#foo', 'index.html#bar'] because of the index.html#bar navigation aborting the back() traversal. I haven't looked much at aborting yet, but I'm hoping to limit that to navigations that require fetches.

@natechapin
Copy link

Fwiw, I wouldn't have guessed either the Chrome result or the Firefox result, I can't really make sense of it.

Chrome's step 1-5 are identical to your model, with the caveat that all queued tasks are IPCs. The difference starts at step 6:

  1. TaskB (back()):
  • Set 'pending' current step to 0.
  • Queue TaskD (again, an IPC) to traverse to the joint session history entry at step 0.
  1. TaskC (index.html#bar):
  • Update history tree to ['index.html', 'index.html#foo', 'index.html#bar']
  • index.html#bar is given a step of 2.
  • Update current step to 2.
  1. TaskD (back() continuation):
  • Restore state for step 0
  • Queue TaskE to update the history tree
  1. TaskE (back() continuation):
  • Update current step to 0.

If you care about the implementation details, Steps 1, 2, 3, 4, and 8 are in Chrome's renderer process, while Steps 5, 6, 7, and 9 are in the browser process.

@jakearchibald
Copy link
Contributor

Do you feel Chrome's current result is correct by design, or a side-effect of the implementation?

@natechapin
Copy link

I think it's within the range of possible correct designs we could settle on. It's not clear to me how the bookkeeping would work for things like Step 5.iv in your algorithm, because the synchronous processing and the navigation resolution step are in different processes.

I think Chrome could probably get most of the way there if we could get rid of TaskE in my description and have TaskB not do a "pending" step and just immediately update the current step. I don't know whether that would cause unacceptable race conditions, though.

@annevk
Copy link
Member

annevk commented Jun 21, 2021

cc @smaug----

@smaug----
Copy link

Gecko's behavior makes the most sense to me. You just queue a task to do back(). Sync navigations behave from session history point of view as if they were, well, sync.
I'm a bit surprised of Chrome's behavior.

@jakearchibald
Copy link
Contributor

Tasks need to be queued for sync navigations too, to update the history tree (which may be in another process) and things like history.length across all the frames.

Eg, if two iframes in different threads/processes perform sync navigations at the same time, one has to win. Although the document switch is synchronous, the updates to the history tree can't be synchronous without blocking IPCs or a racey mess.

@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 21, 2021

I guess I could hack it so that there are two queues instead of one. One for 'synchronous' navigations and another for the rest.

So:

  1. History is ['index.html'] and current step is 0.
  2. Synchronously update to index.html#foo, and queue TaskA into the 'sync navigation' queue to update the history tree.
  3. Queue TaskB into the 'async navigation' queue for back().
  4. Synchronously update to index.html#bar, and queue TaskC into the 'sync navigation' queue to update the history tree.
  5. 'sync' queue is not empty, so TaskA (index.html#foo):
    1. Update history tree to ['index.html', 'index.html#foo'].
    2. index.html#foo is given a step of 1.
    3. Update current step to 1.
    4. Top level doesn't change, because its current active item index.html#bar has a 'pending' step (as in, the synchronous navigation hasn't been resolved).
  6. 'sync' queue is not empty, so TaskC (index.html#bar):
    1. Update history tree to ['index.html', 'index.html#foo', 'index.html#bar']
    2. index.html#bar is given a step of 2.
    3. Update current step to 2.
    4. Top level doesn't need to change, because it's already on index.html#bar.
  7. 'sync' queue is empty, so we go to the other queue for TaskB (back()):
    1. Update current step to 1.
    2. Top level changes to index.html#foo.

I still prefer the version I've currently spec'd where there's only one queue, but Firefox's result at least seems deliberate and not a side-effect.

@domenic
Copy link
Member Author

domenic commented Jul 30, 2021

Note that in the spec same-document (sync) navigations are supposed to empty any queued-up traversals: https://html.spec.whatwg.org/#navigate-fragid step 2 and https://html.spec.whatwg.org/#url-and-history-update-steps step 2.2.

Apparently no browsers do this, so that's fun.

@domenic
Copy link
Member Author

domenic commented Jul 30, 2021

Firefox behaves differently here for pushState() vs. fragment navigations:

history.pushState(null, "", "/foo");
history.back();
history.pushState(null, "", "/bar");

ends up on /bar instead of on /foo.

Chrome still ends up on index.html.

I'll write some tests that assume the Firefox behavior for fragments carry over to pushState(), even though nobody does that...

@smaug----
Copy link

In that testcase history.pushState() cancels .back() in Gecko.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: history
Development

Successfully merging a pull request may close this issue.

5 participants