Use sourceDocument if set for prefetch lookup

[hiroshige-g]

• Spec issue: Speculation rules prefetch results are not used for prerendering #261
• WPT change: [WPT] Use sourceDocument for prefetch record lookup web-platform-tests/wpt#39518

[github-actions]

Preview:

• prefetch.html (diff base)

[domenic]

This looks good, but what do you think about unconditionally using sourceDocument, instead of only using it for prerendering?

For example, I think that would be necessary to make <a href="prefetched.html" target="_blank"> work, which is desirable.

[hiroshige-g]

unconditionally using sourceDocument

Perhaps this makes sense.
See https://chromium-review.googlesource.com/c/chromium/src/+/4422475 for behavior changes.

Yesterday I and @nyaxt discussed not to make this generalization because this would dissappear after #266, but the updates after that are:

• The issue for <a href="prefetched.html" target="_blank"> remains even after Make prefetch records not Document-scoped? #266 (at least for cross-site URLs) because the navigable's active document anyway doesn't belong to the initiator's site.
• I found this is implementable, as the sourceDocument seems to correspond to NavigationRequest::initiator_document_. See https://chromium-review.googlesource.com/c/chromium/src/+/4372403/.

[hiroshige-g]

Uploaded a commit that uses sourceDocument if set, but as also I added as inline issue comments in the commit, I'm not so sure how to plumb sourceDocument in some cases -- maybe we just don't plumb sourceDocument and fallback to navigable's active document instead?

[domenic]

Overall strategy looks good, just some ideas on how to make it cleaner.

[domenic]

• 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.

[hiroshige-g]

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)

[domenic]

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.

[hiroshige-g]

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?).

[domenic]

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.

[hiroshige-g]

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.

[hiroshige-g]

Updated to use source snapshot params.

[hiroshige-g]

@ArthurSonzogni FYI.
sourceSnapshotParams fallbacks to navigable's active document's snapshot at Step 12.6.4 of https://html.spec.whatwg.org/multipage/browsing-the-web.html#apply-the-history-step, which might be similar fallback you mentioned in the codereview comment https://chromium-review.googlesource.com/c/chromium/src/+/4372403/comment/bc534a9a_458b492c/.

[domenic]

Probably make a [=list/clone=] here (https://infra.spec.whatwg.org/#list-clone) to reflect the idea we're snapshotting.

[hiroshige-g]

I'm not so sure: in general creating a snapshot would be a good idea to avoid unintentional race conditions, while in this case not taking a clone might be also an option,

• As [=list/clone=] is a shallow copy, this isn't a complete frozen snapshot of the prefetch records.
• When a prefetch is still ongoing at the time of navigation start and we want to use the prefetch result after response is received after navigation start, we might need intentional interactions via (non-snapshot) prefetch records (IIUC such case is not yet specified so though)

[domenic]

Yeah, I can see your concerns. However, I think it'd most fit the spirit of source snapshot params to start with a shallow clone for now, reflecting the idea that any prefetches added after the prerender navigation starts, should not influence the prerender navigation. Similar to how, e.g., changes to the page's origin or sandboxing flags after navigation starts, should not affect the (normal, non-prerender) navigation.

I agree that it's a little strange that we're not doing a deep clone. But that's kind of the nature of prefetch records being things that are updated over time. I think that part is OK.