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.

Sign up for GitHub

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

Use sourceDocument if set for prefetch lookup #267

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

Use sourceDocument if set for prefetch lookup #267

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d97b00
Use sourceDocument for prefetch lookup in prerendering navigation
hiroshige-g Apr 10, 2023
d3d7be8
Always use sourceDocument if set
hiroshige-g Apr 17, 2023
c7658e8
Remove obsolete anchor
hiroshige-g Apr 17, 2023
dc87b98
Rename to sourceDocumentForPrefetches
hiroshige-g Apr 17, 2023
4474dc5
Plumb via source snapshot params
hiroshige-g Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions prefetch.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ spec: COOKIES; urlPrefix: https://httpwg.org/specs/rfc6265.html
spec: nav-speculation; urlPrefix: prerendering.html
type: dfn
text: getting the supported loading modes; url: get-the-supported-loading-modes
text: loading mode; for: navigable; url: navigable-loading-mode
hiroshige-g marked this conversation as resolved.
Show resolved Hide resolved
text: uncredentialed-prefetch; for: Supports-Loading-Mode; url: supports-loading-mode-uncredentialed-prefetch
spec: resource-timing; urlPrefix: https://w3c.github.io/resource-timing/
type: dfn; for: PerformanceResourceTiming; text: delivery type; url: dfn-delivery-type
Expand Down Expand Up @@ -412,7 +413,7 @@ Update all creation sites to supply an empty string, except for any in this docu
<hr>

<div algorithm="attempt to populate the history entry's document">
In <a spec=HTML>attempt to populate the history entry's document</a>, replace the step which invokes <a spec=HTML>create navigation params by fetching</a> with the following:
In <a spec=HTML>attempt to populate the history entry's document</a>, add an optional {{Document}} |sourceDocument| argument, and replace the step which invokes <a spec=HTML>create navigation params by fetching</a> with the following:
hiroshige-g marked this conversation as resolved.
Show resolved Hide resolved

1. Otherwise, if both of the following are true:
* <var ignore>entry</var>'s [=session history entry/URL=]'s [=url/scheme=] is a [=fetch scheme=]; and
Expand All @@ -422,7 +423,10 @@ Update all creation sites to supply an empty string, except for any in this docu

1. Let |request| be the result of [=creating a navigation request=] given <var ignore>entry</var>, <var ignore>sourceSnapshotParams</var>'s [=source snapshot params/fetch client=], <var ignore>navigable</var>'s [=navigable/container=], and <var ignore>sourceSnapshotParams</var>'s [=source snapshot params/has transient activation=].
1. Set |request|'s [=request/replaces client id=] to <var ignore>navigable</var>'s [=navigable/active document=]'s [=relevant settings object=]'s [=environment/id=].
1. Let |prefetchRecord| be the result of [=finding a matching prefetch record=] given <var ignore>navigable</var>'s [=navigable/active document=], <var ignore>entry</var>'s [=session history entry/URL=], and <var ignore>targetSnapshotParams</var>'s [=target snapshot params/sandboxing flags=].
1. If |sourceDocument| is given, then set |referringDocument| to |sourceDocument|.
1. Otherwise, set |referringDocument| to <var ignore>navigable</var>'s [=navigable/active document=].
<p class="issue">Or should we fallback to null instead? A notable case where this shouldn't be null is <a spec=HTML>reload</a>, but I'm not sure about other cases.</p>
1. Let |prefetchRecord| be the result of [=finding a matching prefetch record=] given |referringDocument|, <var ignore>entry</var>'s [=session history entry/URL=], and <var ignore>targetSnapshotParams</var>'s [=target snapshot params/sandboxing flags=].
1. If <var ignore>documentResource</var> is null and |prefetchRecord| is not null:
1. Set <var ignore>navigationParams</var> to the result of [=creating navigation params from a prefetch record=] given <var ignore>navigable</var>, <var ignore>entry</var>'s [=session history entry/document state=], <var ignore>navigationId</var>, <var ignore>navTimingType</var>, <var ignore>request</var>, |prefetchRecord|, <var ignore>targetSnapshotParams</var>, and <var ignore>sourceSnapshotParams</var>.
1. [=Copy prefetch cookies=] given |prefetchRecord|'s [=prefetch record/isolated partition key=] and <var ignore>navigationParams</var>'s [=navigation params/reserved environment=].
Expand All @@ -432,6 +436,8 @@ Update all creation sites to supply an empty string, except for any in this docu
1. Let |coopEnforcementResult| be the result of [=creating a cross-origin opener policy enforcement result for navigation=] given <var ignore>navigable</var>'s [=navigable/active document=] and <var ignore>entry</var>'s [=session history entry/document state=]'s [=document state/initiator origin=].
1. Set <var ignore>navigationParams</var> to the result of [=creating navigation params by fetching=] given |request|, <var ignore>entry</var>, |coopEnforcementResult|, <var ignore>navigable</var>, <var ignore>sourceSnapshotParams</var>, <var ignore>targetSnapshotParams</var>, <var ignore>cspNavigationType</var>, <var ignore>navigationId</var>, and <var ignore>navTimingType</var>.

The |sourceDocument| argument should be plumbed from <a spec=HTML>navigate</a>'s |sourceDocument| and <a spec=HTML>traverse the history by a delta</a>'s |sourceDocument|.
<p class="issue">How about other callers, i.e. <a spec=HTML>apply pending history changes</a>, <a spec=HTML>finalize a same-document navigation</a> and <a spec=HTML>finalize a cross-document navigation</a>?</p>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • apply pending history changes: only if we want to support prefetching reloads. Which I'm not sure we do?
  • finalize a same-document navigation: no, prefetching is not relevant there
  • finalize a cross-document navigation: yes, plumbing it through here is necessary for the path navigate -> finalize a cross-document navigation -> apply the history step.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

support prefetching reloads

There is a WPT already for reload (#212 (comment)), but maybe just to check the Chromium behavior?
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/speculation-rules/prefetch/prefetch-traverse-reload.sub.html

navigate -> finalize a cross-document navigation -> apply the history step

Do you have examples/test cases where prefetched results would be used in this code path? (I'm not familiar with how "finalize a cross-document navigation" works)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a WPT already for reload (#212 (comment)), but maybe just to check the Chromium behavior?

Ah OK, I am happy to keep it then.

Do you have examples/test cases where prefetched results would be used in this code path? (I'm not familiar with how "finalize a cross-document navigation" works)

This is the most basic code path. E.g. this is the code path which clicking a link or doing location.href goes through.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://html.spec.whatwg.org/multipage/browsing-the-web.html#reload says:

It is intentional that the resulting call to apply the history step does not pass sourceSnapshotParams or initiatorToCheck.

So probably we shouldn't pass sourceDocumentForPrefetches as well?

Also, how about passing sourceDocumentForPrefetches (or its prefetch records) via sourceSnapshotParams?

Pros: It might be consistent with the existing decisions whether we should pass sourceDocument-related informatio n (sourceSnapshotParams or sourceDocumentForPrefetches) or not

Cons?: this means not passing sourceDocumentForPrefetches in finalize a cross-document navigation. IIUC sourceDocumentForPrefetches will be still passed for ordinal cross-document navigations via navigate -> attempt to populate the history entry's document -> creating navigation params by fetching, even if we don't pass sourceDocumentForPrefetches via navigate -> finalize a cross-document navigation though (I'm not sure the differences between the two paths, but probably the path via attempt to populate the history entry's document is the primary one?).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay; I got sick :(.

So probably we shouldn't pass sourceDocumentForPrefetches as well?

Hmm, I'm not sure. Mostly, sourceSnapshotParams and initiatorToCheck are used for security checks which (for some reason) browsers decide to not perform on reloads. The only thing that's kind of similar to how sourceDocumentForPrefetches is used is sourceSnapshotParams's fetch client, which is used to determine referrer and such. (E.g. for reloads, it always looks like the page is reloading itself, according to the Referer header.)

Cons?: this means not passing sourceDocumentForPrefetches in finalize a cross-document navigation. IIUC sourceDocumentForPrefetches will be still passed for ordinal cross-document navigations via navigate -> attempt to populate the history entry's document -> creating navigation params by fetching, even if we don't pass sourceDocumentForPrefetches via navigate -> finalize a cross-document navigation though (I'm not sure the differences between the two paths, but probably the path via attempt to populate the history entry's document is the primary one?).

Sorry, I realize I was misleading in what I said earlier. The paths are:

  • push/replace navigation: navigate -> attempt to populate -> finalize a cross-document navigation -> apply the history step. In apply the history step, targetEntry's document will be present, because of the earlier attempt to populate step.

  • reload navigation: reload -> apply pending history changes -> apply the history step -> attempt to populate. This time we reach attempt to populate from apply the history step, because targetEntry's document state's reload pending is true

  • back/forward navigation, bfcache: traverse the history by a delta -> apply the history step. In apply the history step, targetEntry's document will be present, because bfcache.

  • back/forward navigation, non-bfcache: traverse the history by a delta -> apply the history step -> attempt to populate. This time we reach attempt to populate from apply the history step, because targetEntry's document is null, because no bfcache.

Does that make sense? Feel free to grab me in person or in realtime if it doesn't; I know this stuff is complicated and I don't want to be confusing about it again.

So, I think your idea of putting sourceDocumentForPrefetches or the prefetch records in source snapshot params mostly works, and feels elegant. The consequence would be that reloads can only use prefetch records from their own document though, e.g. if you had this setup:

  • Parent loads a.html in an iframe
  • Later, parent prefetches a.html
  • Parent calls frames[0].reload()

then the prefetch would not be used. Maybe that's fine, because it's congruent with how the fetch client is set to frames[0] instead of to the parent?

Another elegant part of putting the prefetch records (not sourceDocumentForPrefetches) in source snapshot params is that if we start storing the prefetch records on a wider scope, per #266, it will not change very much: it will just change the snapshotting call to grab them from a different place.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, thank you for inputs and clarifications! Then I'll draft the spec to put prefetch records to source snapshot params.

I'm also less sure about reload's behavior here, but I feel we can prioritize (at least for now) the elegance/consistency of the spec architecture by putting the prefetch records to source snapshot params, and then later (after #266) we can revisit the reload behavior if actually needed in the wild.

If we make prefetch records scoped by network partition keys, I imagine prefetch records would be partitioned by the result of determine the network partition key given request’s client (because sourceDocumentForPrefetches basically corresponds to source snapshot params' client), while HTTP caches for navigation requests are partitioned by the result of determine the network partition key given request’s reserved client, which is perhaps simple.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use source snapshot params.

</div>

<hr>
Expand Down