Properly assign history policy container

[domfarolino]

Consider the cross-document navigation case before the navigation & session history rewrite. The #navigate algorithm took a manually-supplied historyPolicyContainer that was only supplied by history traversals, so in the normal navigation flow it is null. Then, navigation params had a non-null policy container member that was assigned in #process-a-navigate-fetch to the result of determining which of the four usual policy containers is suitable, one of which is the manually-supplied historyPolicyContainer (that again, was only set for traversals).

Later, we'd end up in #update-the-session-history-with-the-new-page where we create for the first time the new session history entry, its policy container populated (#navigating-across-documents:she-policy-container-2 in the pre-rewrite spec) with the navigation params's iff the URL required so.

After #6315, the new spec is broken in several ways as described by #8725.

To fix the new spec's document state's history policy container, this PR:

• Makes document state's policy container a policy container or null, initially null
• Stops fetching navigation requests with the manually-supplied document state's history policy container
  • Doing so just seems wrong — if anything we should be manually-supplying some other kind of policy container, but not one that is only meant to be stored on the SHE/document state for "local" scheme resources. This PR reverts to the pre-rewrite prose where we didn't manually set the navigation request's policy container.
  • But even that seems weird too, so let me sanity check that it's OK: If we never set a navigation request's policy container, then Fetch will try to get it from the client (which is null for navigation requests; we set reserved client instead) or create a brand new one from scratch. That means no policy container ever applies to navigation requests. But the only effect that a request's policy container has is on its referrer policy, per the references in Fetch, but a navigation request's referrer policy is always manually set in HTML (see https://html.spec.whatwg.org/#beginning-navigation:document-state-request-referrer-policy & https://html.spec.whatwg.org/#populating-a-session-history-entry:document-state-request-referrer-policy) to at least the platform default policy (not the empty string), so I think this change is good.
• On redirects in #create-a-navigation-params-by-fetching, conditionally clone the old document state's policy container (if it is non-null). We "clone" conditionally because the old state's policy container may be null
• Finally, we set the document state's history policy container to the navigation params's policy container. I'm doing this in https://html.spec.whatwg.org/#attempt-to-populate-the-history-entry's-document just after we load the new document.
  • I think this is consistent with where this logic used to live in the old spec. Previously, we'd assign the entry's history policy container after "loading" the new document, and just as we (a) cleared all forward history ("default" case), and (b) appended the session history entry to the list of entries. With this PR, the new spec does it then as well.
  • At first, I thought this logic belonged in #finalize-the-cross-document-navigation, but that'd be a little tedious to make work since that algorithm doesn't take a navigation params. One reason I thought that algorithm would make for bad placement was that it's only called in the navigation case, not the traversal case, and if you traversed to a URL that redirected to one that would require history policy container storage, you'd never actually end up storing the history policy container; but then I remembered that redirects to the kinds of URLs that would require history policy container storage are illegal, so that's a moot point.

---

/browsing-the-web.html ( diff )

[domenic]

This looks very reasonable to me. Thanks a lot for the extensive justification and archeology!!

@antosart, your review would also be useful.

[domfarolino]

I think we still need to evaluate whether or not https://html.spec.whatwg.org/#the-javascript:-url-special-case:document-state-history-policy-container is a valid cloning of the history policy container. If a history policy container should only ever be stored on documents whose URLs are local schemes, and javascript: URLs are not local schemes, then we probably shouldn't be cloning here.

On the other hand, if I understand correctly, the URL of documents resulting from javascript: URLs is not a javascript: URL, but just a copy of the active document's URL, which may indeed be a URL that requires storing history policy container in the document state, so I think we need the same conditional cloning there as I added in the #create-navigation-params-by-fetching algorithm.

[antosart]

Thanks for the detailed explanation and for the quick fix. This all makes sense to me,, also the story about javascript URLs. I think this does what we want it to do.

[antosart]

Note: we could change "clone a policy container" to return null if the argument is null, since you are doing this already twice in this PR. I don't know what the spec editors would prefer here, though.

[domfarolino]

Yeah that's a good point. Since there are only two occurrences, and I like the simplicity of shielding the "clone" algorithm from handling null input, I'm fine with what's currently here, but will change it if @domenic has opinions.

[domenic]

I'm on the fence personally. Let's keep it as-is, but also keep an eye out for a third case.