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

Allow document.open() to desync session history and document URLs #6649

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

domenic
Copy link
Member

@domenic domenic commented May 4, 2021

Closes #6556. In particular, reverts document.open() to only update the document's URL, and not the session history entry's URL, like it did before ae7cf0c. Now that they can mismatch, we need to audit the cases where this might be important, which leads to the following changes:

This also modernizes and makes a bit more precise the location.reload() method steps. The user-initiated reload steps remain vague; #6600 will tackle those.

(See WHATWG Working Mode: Changes for more details.)


/dynamic-markup-insertion.html ( diff )
/history.html ( diff )

Closes #6556. In particular, reverts document.open() to only update the document's URL, and not the session history entry's URL, like it did before ae7cf0c. Now that they can mismatch, we need to audit the cases where this might be important, which leads to the following changes:

* Changes location.reload() to reload the current session history entry's URL, instead of the document's URL. This ensures that post-document.open() reload behavior is aligned with WebKit and Gecko, as tested by https://wpt.fyi/results/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/reload.window.html.

* Changes history.pushState()/history.replaceState() with no URL argument to default to the document's URL, instead of the current session history's URL. This ensures that post-document.open() pushState()/replaceState() behavior is aligned with all engines, as tested by web-platform-tests/wpt#28826.

This also modernizes and makes a bit more precise the location.reload() method steps. The user-initiated reload steps remain vague; #6600 will tackle those.
@domenic domenic added normative change interop Implementations are not interoperable with each other topic: document.open() labels May 4, 2021
@domenic
Copy link
Member Author

domenic commented May 4, 2021

Changes location.reload() to reload the current session history entry's URL, instead of the document's URL. This ensures that post-document.open() reload behavior is aligned with WebKit and Gecko, as tested by https://wpt.fyi/results/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/reload.window.html.

Ooops, I think this is wrong. The test is actually testing that when you document.open(), the new overwritten URL is the one that gets reloaded. So I think the spec doesn't need to change here, and instead Chrome just needs to match the existing spec.

I will revert all location.reload() parts of the patch (including the editorial updates)

@domenic
Copy link
Member Author

domenic commented May 4, 2021

No, wait, reverting is probably not going to be enough, because the guts of the session history traversal algorithm operate on the current entry's URL in the reload case. OK, this is not yet ready for review; I need to work on it some more tomorrow.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label May 5, 2021
@domenic
Copy link
Member Author

domenic commented May 6, 2021

OK, so we want to fix the spec to give the Safari/Firefox behavior, where reloading a document.open()ed page reloads the URL of the caller.

The main problem, I think, is the following line in traverse the history:

  1. Set newDocument's URL to entry's URL.

which will mean that even if the reload does get the right page from the network, document.URL will not exhibit Safari/Firefox behavior (see test).

Stepping back, this line seems really weird to me. Consider the following scenario:

  1. You navigate to /a.
  2. You navigate to /b. Bfcache evicts /a.
  3. The server operator updates /a to do a redirect to /c.
  4. You traverse back.
    • This traverses the history back to the /a entry; when it finds a null document it invokes the "navigate" algorithm with the /a URL, bailing out in step 1.
    • This navigate algorithm follows redirects and retrieves a new document. That document's URL is /c.
    • Once we get back into "traverse the history", this time we reach step 5. But we update the document's URL to /a???

Is that how browsers behave? Seems pretty unlikely?

Somewhat more plausible is a situation like the following:

  1. You navigate to /a#foo.
  2. You navigate to /b. Bfcache evicts /a#foo.
  3. You traverse back.
    • As before this ends up navigating and fetching
    • Maybe some parts of the spec ecosystem (Fetch/HTTP/etc.) drop the #foo, so the resulting document only ends up with URL /a??
    • So maybe the point of this step is to restore the #foo?

@jakearchibald, have you encountered this line in your rewrite? /cc @domfarolino since he's interested in this area.

@jakearchibald
Copy link
Contributor

I hadn't tested this, but now I have! (and a bunch of similar cases) #6680

@domenic
Copy link
Member Author

domenic commented May 13, 2021

OK, so my reading of your first test case in #6680 is that browsers do not rewrite /c (in my example) to /a. But, they do like to tag fragments back on somehow, even if those fragments were originally for another URL!

I see a couple options here:

  • Delay all action on this problem until the session history rewrite lands. It's the only way to be safe.

  • Delete the line "Set newDocument's URL to entry's URL." since it mostly doesn't apply. Instead replace it with an XXX box saying something like "It appears fragments from the entry's URL are often grafted onto newDocument's URL; see Document sharing across session history entries following redirects & traversal #6680." Or maybe just spec that fragment-grafting.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment interop Implementations are not interoperable with each other normative change topic: document.open() topic: history
Development

Successfully merging this pull request may close these issues.

document.open() URL updates can cause recursive loading problems on history traversal
3 participants