Bug 199162 - Cross-origin ongoing prefetches should be reused for top-level navigations
Summary: Cross-origin ongoing prefetches should be reused for top-level navigations
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-24 08:43 PDT by Rob Buis
Modified: 2019-10-10 18:29 PDT (History)
12 users (show)

See Also:


Attachments
Patch (12.95 KB, patch)
2019-06-24 08:49 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (20.28 KB, patch)
2019-06-25 13:08 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (20.32 KB, patch)
2019-06-26 01:25 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.25 MB, application/zip)
2019-06-26 02:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.98 MB, application/zip)
2019-06-26 03:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews212 for win-future (13.66 MB, application/zip)
2019-06-26 03:34 PDT, Build Bot
no flags Details
Patch (22.44 KB, patch)
2019-06-26 10:56 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (21.62 KB, patch)
2019-07-01 01:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (23.13 KB, patch)
2019-07-01 06:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (23.21 KB, patch)
2019-07-03 00:10 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (23.24 KB, patch)
2019-07-03 06:10 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (22.56 KB, patch)
2019-07-27 11:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (29.66 KB, patch)
2019-08-22 08:24 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (29.66 KB, patch)
2019-08-22 08:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (17.69 KB, patch)
2019-08-22 08:55 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (21.57 KB, patch)
2019-08-23 00:18 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (21.48 KB, patch)
2019-08-23 12:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (21.21 KB, patch)
2019-10-10 01:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (22.40 KB, patch)
2019-10-10 02:43 PDT, Rob Buis
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.80 MB, application/zip)
2019-10-10 18:29 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2019-06-24 08:43:42 PDT
Cross-origin prefetches should be reused for top level navigations.
Comment 1 Rob Buis 2019-06-24 08:49:46 PDT
Created attachment 372766 [details]
Patch
Comment 2 Rob Buis 2019-06-25 13:08:29 PDT
Created attachment 372854 [details]
Patch
Comment 3 Rob Buis 2019-06-26 01:25:01 PDT
Created attachment 372908 [details]
Patch
Comment 4 Build Bot 2019-06-26 02:40:26 PDT
Comment on attachment 372908 [details]
Patch

Attachment 372908 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12578887

New failing tests:
http/wpt/prefetch/navigate-reuse-prefetch.html
Comment 5 Build Bot 2019-06-26 02:40:28 PDT
Created attachment 372909 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 6 Build Bot 2019-06-26 03:18:42 PDT
Comment on attachment 372908 [details]
Patch

Attachment 372908 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12578911

New failing tests:
http/wpt/prefetch/navigate-reuse-prefetch.html
Comment 7 Build Bot 2019-06-26 03:18:44 PDT
Created attachment 372910 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 8 Build Bot 2019-06-26 03:34:47 PDT
Comment on attachment 372908 [details]
Patch

Attachment 372908 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12578978

New failing tests:
http/wpt/prefetch/navigate-reuse-prefetch.html
Comment 9 Build Bot 2019-06-26 03:34:50 PDT
Created attachment 372911 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 10 Rob Buis 2019-06-26 10:56:23 PDT
Created attachment 372936 [details]
Patch
Comment 11 youenn fablet 2019-06-28 12:07:54 PDT
Comment on attachment 372936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372936&action=review

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:421
> +        if (auto session = networkProcess().networkSession(loadParameters.sessionID)) {

auto*

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:422
> +            auto url = loadParameters.request.url();

const auto&

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:423
> +            if (session->waitingForPendingPrefetch(url)) {

I would rename waitingForPendingPrefetch to hasOngoingPrefetch.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:936
> +void NetworkConnectionToWebProcess::prefetchFinished(const PAL::SessionID& sessionID, const URL& url, WebCore::ResourceResponse&& response, RefPtr<WebCore::SharedBuffer>&& buffer)

We are not consistent in const PAL::SessionID& vs. PAL::SessionID.
I slightly prefer PAL::SessionID.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:942
> +                m_networkResourceLoaders.add(crossOriginPrefetch->identifier(), crossOriginPrefetch.releaseNonNull());

I am not clear why we need this.
If the crossOriginPrefetch is finished, why are we keeping the NetworkResourceLoader in the map? Is it for being able to clear it shortly after?

If session->hasPendingNavigation(url) is true, session->takeCrossOriginPrefetchLoad(url) should be true as well in most cases but maybe not all?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:943
> +            auto loadParameters = session->takePendingNavigation(url);

We are doing two HashMap queries hasPendingNavigation and takePendingNavigation. With find, we could do just one.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:946
> +            loader->start();

Could be rewritten as something like m_networkResourceLoaders.add(loadParameters.identifier, WTFMove(loader)).iterator->value->start();

> Source/WebKit/NetworkProcess/NetworkSession.cpp:237
> +    if (auto ret = m_pendingCrossOriginPrefetchLoads.take(url))

s/ret/result

> LayoutTests/http/wpt/prefetch/resources/navigate-redirect.html:9
> +  document.body.appendChild(link);

You can probably use .sub.html to write it directly as
<link rel="prefetch" href=""http://{{host[alt][]}}:{{ports[http][0]}}/WebKit/prefetch/resources/prefetched-main-resource-redirect.py"
Comment 12 Rob Buis 2019-07-01 01:09:17 PDT
Created attachment 373213 [details]
Patch
Comment 13 Rob Buis 2019-07-01 06:32:16 PDT
Created attachment 373229 [details]
Patch
Comment 14 Rob Buis 2019-07-01 08:09:27 PDT
Comment on attachment 372936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372936&action=review

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:421
>> +        if (auto session = networkProcess().networkSession(loadParameters.sessionID)) {
> 
> auto*

Done.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:422
>> +            auto url = loadParameters.request.url();
> 
> const auto&

Done.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:423
>> +            if (session->waitingForPendingPrefetch(url)) {
> 
> I would rename waitingForPendingPrefetch to hasOngoingPrefetch.

Done.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:936
>> +void NetworkConnectionToWebProcess::prefetchFinished(const PAL::SessionID& sessionID, const URL& url, WebCore::ResourceResponse&& response, RefPtr<WebCore::SharedBuffer>&& buffer)
> 
> We are not consistent in const PAL::SessionID& vs. PAL::SessionID.
> I slightly prefer PAL::SessionID.

Done.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:942
>> +                m_networkResourceLoaders.add(crossOriginPrefetch->identifier(), crossOriginPrefetch.releaseNonNull());
> 
> I am not clear why we need this.
> If the crossOriginPrefetch is finished, why are we keeping the NetworkResourceLoader in the map? Is it for being able to clear it shortly after?
> 
> If session->hasPendingNavigation(url) is true, session->takeCrossOriginPrefetchLoad(url) should be true as well in most cases but maybe not all?

Yes, we need it because it will be cleared in NetworkConnectionToWebProcess::didCleanupResourceLoader. I did not like that code though, so I solved it differently, a lot like keep alive code.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:943
>> +            auto loadParameters = session->takePendingNavigation(url);
> 
> We are doing two HashMap queries hasPendingNavigation and takePendingNavigation. With find, we could do just one.

Done.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:946
>> +            loader->start();
> 
> Could be rewritten as something like m_networkResourceLoaders.add(loadParameters.identifier, WTFMove(loader)).iterator->value->start();

Done.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:237
>> +    if (auto ret = m_pendingCrossOriginPrefetchLoads.take(url))
> 
> s/ret/result

Done.

>> LayoutTests/http/wpt/prefetch/resources/navigate-redirect.html:9
>> +  document.body.appendChild(link);
> 
> You can probably use .sub.html to write it directly as
> <link rel="prefetch" href=""http://{{host[alt][]}}:{{ports[http][0]}}/WebKit/prefetch/resources/prefetched-main-resource-redirect.py"

Done.
Comment 15 Rob Buis 2019-07-01 08:10:52 PDT
Comment on attachment 373229 [details]
Patch

Note that I think we could/should make this work for same-origin prefetches as well, but it seems non trivial, so I am hoping this can go in first.
Comment 16 Ryosuke Niwa 2019-07-01 08:50:50 PDT
Comment on attachment 373229 [details]
Patch

This feature seems problematic from fingerprinting standpoint.

Wouldn't this feature allow domain A.com to prefetch tracker.com and then the user visits B.com, it can cycle through tracker.com to detect whether the user had visited A.com or not by detecting whether the page load had hit the prefetch cache or not?
Comment 17 Ryosuke Niwa 2019-07-01 08:51:38 PDT
+John, Maciej for assessing the fingerprinting issue I mention above.
Comment 18 youenn fablet 2019-07-02 11:04:23 PDT
> Wouldn't this feature allow domain A.com to prefetch tracker.com and then
> the user visits B.com, it can cycle through tracker.com to detect whether
> the user had visited A.com or not by detecting whether the page load had hit
> the prefetch cache or not?

Only the main frame is allowed to use the prefetch cache.
A.com can prefetch tracker.com so that A.com -> tracker.com -> B.com will be faster than it is now.
Comment 19 youenn fablet 2019-07-02 11:12:39 PDT
Comment on attachment 373229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373229&action=review

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:430
> +                session->addPendingNavigation(url, WTFMove(loadParameters));

It seems that load will hang in case the prefetch loading fails?
We should probably trigger the load with the load path.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:946
> +        if (Optional<NetworkResourceLoadParameters> loadParameters = session->takePendingNavigation(url)) {

auto

> Source/WebKit/NetworkProcess/NetworkSession.cpp:250
> +        return m_pendingNavigations.take(url);

This does two search on the map, we should be able with find/remove to make just one.
Comment 20 Rob Buis 2019-07-03 00:10:33 PDT
Created attachment 373384 [details]
Patch
Comment 21 Ryosuke Niwa 2019-07-03 00:32:52 PDT
(In reply to youenn fablet from comment #18)
> > Wouldn't this feature allow domain A.com to prefetch tracker.com and then
> > the user visits B.com, it can cycle through tracker.com to detect whether
> > the user had visited A.com or not by detecting whether the page load had hit
> > the prefetch cache or not?
> 
> Only the main frame is allowed to use the prefetch cache.
> A.com can prefetch tracker.com so that A.com -> tracker.com -> B.com will be
> faster than it is now.

Yeah, so this will create a new tracking mechanism because you can detect that it was fast (and therefore prefetch'ed). Basically, A.com can contain a tracking script which inserts <link rel=prefetch href=tracker.com?url=a.com> to the main page. When B.com is loaded, it can immediately navigate to tracker.com?url=a.com to detect that the user had visited A.com based on how fast tracker.com loads.

I'm pretty certain there is a clever trick to make this tracking mechanism more powerful. In general, this feature seems to circumvent the whole partitioning of our disk cache based on top-level domain so there could be other privacy holes.
Comment 22 Rob Buis 2019-07-03 06:10:29 PDT
Created attachment 373388 [details]
Patch
Comment 23 Maciej Stachowiak 2019-07-03 14:35:14 PDT
Is a page limited in how many pages it can prefetch? Does <link rel=prefetch> have any signal for whether the prefetch succeeded or how long it took, such as load or error events?
Comment 24 Maciej Stachowiak 2019-07-03 14:38:14 PDT
Also, just to confirm, is `<link rel=prefetch>` supposed to bypass cache partitioning, and is it supposed to use the first party cookies for the prefetched resource?
Comment 25 Maciej Stachowiak 2019-07-03 14:42:01 PDT
Hmmm, if `<link rel=prefetch>` is allowed to load with first party cookies and gets Referer, then it could be used a tracking pixel that bypasses ITP, even without redirect tricks.

BTW, we know motivated trackers *will* use redirect tricks to get a user ID that then gets boosted into first party storage if they are able to. They did it with HSTS.

I'd rather not see this land until there is a clear privacy story (unless there's a WebKit port that really wants to ship prefetch regardless of the privacy problems).
Comment 26 youenn fablet 2019-07-03 14:42:25 PDT
(In reply to Maciej Stachowiak from comment #23)
> Is a page limited in how many pages it can prefetch? Does <link
> rel=prefetch> have any signal for whether the prefetch succeeded or how long
> it took, such as load or error events?

Currently, there is no such limitation in the implementation.
This seems reasonable to me as well as the possibility to scope which pages can get access to which prefetches.

As for load/error events, this is under discussion on GitHub.

(In reply to Maciej Stachowiak from comment #24)
> Also, just to confirm, is `<link rel=prefetch>` supposed to bypass cache
> partitioning, and is it supposed to use the first party cookies for the
> prefetched resource?

The idea is that it is supposed to bypass cache partitioning in a specific way, no data persistency, short lifetime and restricted access to specific loads.
The prefetch load is no credentials and manual redirect mode.
Comment 27 youenn fablet 2019-07-03 14:46:16 PDT
GitHub links: https://github.com/whatwg/fetch/pull/881 and https://github.com/whatwg/html/pull/4115
Comment 28 Maciej Stachowiak 2019-07-03 18:01:10 PDT
The Resource Hints spec that is linked by the HTML spec currently does not say anything about loads being without credentials, and even seems to allow CORS to read back the contents of a load with-credentials. And it also doesn't say to suppress load and error events, which happen for other link types that load a resource.

Is that Resource Hints spec obsolete and planned to be replaced with material inline in Fetch and HTML?

https://w3c.github.io/resource-hints/
Comment 29 youenn fablet 2019-07-03 18:07:24 PDT
The prefetch part is being specced as PRs that are not merged right now.
These PRs require the notion of partitioning, a new kind of load maybe so they are not straightforward.
Yoav knows more about the current state of the PRs.

For instance, I think the fetch PR states that credentials more is same origin.
Comment 30 Maciej Stachowiak 2019-07-03 18:13:34 PDT
If prefetching is without credentials, does that mean it can not safely be used for main resources in general? Only for sub resources known not to depend on cookies?
Comment 31 Maciej Stachowiak 2019-07-03 18:14:31 PDT
BTW doing a privacy review of something only described by PRs to multiple specs is pretty hard. it would be nice to have an explainer of how the whole thing is supposed to work.
Comment 32 Rob Buis 2019-07-04 10:08:32 PDT
Comment on attachment 373229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373229&action=review

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:430
>> +                session->addPendingNavigation(url, WTFMove(loadParameters));
> 
> It seems that load will hang in case the prefetch loading fails?
> We should probably trigger the load with the load path.

I think it depends on the load fail. It seems 4xx and 5xx responses go through  NetworkResourceLoader::didFinishLoading so they are on the right track. I found out however that the code to re-use the prefetch in NetworkResourceLoader::retrieveCacheEntry does not work for non-cacheable responses. To try to fix this I created https://bugs.webkit.org/show_bug.cgi?id=199499.

The rest of the failures I expect to go through NetworkResourceLoader::didFailLoading, where we could add a hook similar to NetworkConnectionToWebProcess::prefetchFinished, but for load failures (authentication, bad port etc.).

I would be curious to know if there are more failure code paths though.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:946
>> +        if (Optional<NetworkResourceLoadParameters> loadParameters = session->takePendingNavigation(url)) {
> 
> auto

Done.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:250
>> +        return m_pendingNavigations.take(url);
> 
> This does two search on the map, we should be able with find/remove to make just one.

Done.
Comment 33 Yoav Weiss 2019-07-08 02:30:00 PDT
(In reply to youenn fablet from comment #29)
> The prefetch part is being specced as PRs that are not merged right now.
> These PRs require the notion of partitioning, a new kind of load maybe so
> they are not straightforward.
> Yoav knows more about the current state of the PRs.

The intent is indeed to integrate Prefetch's processing model into HTML and obsolete the parts of the Resource Hints spec which refer to it.


> 
> For instance, I think the fetch PR states that credentials more is same
> origin.
Comment 34 Rob Buis 2019-07-27 11:07:12 PDT
Created attachment 375031 [details]
Patch
Comment 35 Dominic Farolino 2019-07-30 21:21:52 PDT
Just an update to this thread: Chromium is interested in implementing the changes to prefetch requests proposed by Youenn in [1]. I've filed a request for a TAG review [2] as a part of our Intent to Implement, but ultimately having an explainer for these changes and their suspected effects would be ideal, as Maciej mentioned.

[1]: https://github.com/w3c/resource-hints/issues/82
[2]: https://github.com/w3ctag/design-reviews/issues/398
Comment 36 Rob Buis 2019-08-22 08:24:51 PDT
Created attachment 377007 [details]
Patch
Comment 37 Rob Buis 2019-08-22 08:39:41 PDT
Created attachment 377008 [details]
Patch
Comment 38 Rob Buis 2019-08-22 08:55:53 PDT
Created attachment 377011 [details]
Patch
Comment 39 Rob Buis 2019-08-23 00:18:46 PDT
Created attachment 377106 [details]
Patch
Comment 40 Darin Adler 2019-08-23 10:43:53 PDT
Comment on attachment 377106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377106&action=review

> Source/WebCore/platform/network/ResourceRequestBase.cpp:275
> +    return httpHeaderField(HTTPHeaderName::Purpose) == "prefetch";

Is this case sensitive, or should be it equalLettersIgnoringASCIICase?
Comment 41 Rob Buis 2019-08-23 12:45:41 PDT
Created attachment 377150 [details]
Patch
Comment 42 Rob Buis 2019-08-23 14:22:13 PDT
(In reply to Darin Adler from comment #40)
> Comment on attachment 377106 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377106&action=review
> 
> > Source/WebCore/platform/network/ResourceRequestBase.cpp:275
> > +    return httpHeaderField(HTTPHeaderName::Purpose) == "prefetch";
> 
> Is this case sensitive, or should be it equalLettersIgnoringASCIICase?

Unfortunately AFAIK it is not specified (yet), but I am fine with case insensitive, so I updated the patch.
Comment 43 Frédéric Wang (:fredw) 2019-08-29 03:49:49 PDT
(In reply to Rob Buis from comment #42)
> Unfortunately AFAIK it is not specified (yet), but I am fine with case
> insensitive, so I updated the patch.

Do you know if there is any plan to specify it? I think this should be reported to the spec authors and we should have tests for it.
Comment 44 Dominic Farolino 2019-10-01 16:00:24 PDT
It is specified. This isn't a prefetch-specific thing; HTML Standard notes that link types are specified by link "keywords", and all keywords are ASCII case-insensitive [1].

[1]: https://html.spec.whatwg.org/multipage/links.html#linkTypes
Comment 45 Rob Buis 2019-10-10 01:45:39 PDT
Created attachment 380610 [details]
Patch
Comment 46 Rob Buis 2019-10-10 02:43:48 PDT
Created attachment 380617 [details]
Patch
Comment 47 Build Bot 2019-10-10 18:28:59 PDT
Comment on attachment 380617 [details]
Patch

Attachment 380617 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13116694

New failing tests:
http/tests/preload/picture-type.html
Comment 48 Build Bot 2019-10-10 18:29:02 PDT
Created attachment 380711 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit