Bug 199499 – Make storing cross-origin top-level prefetches in HTTP cache optional

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Rob Buis]

Currently when we navigate we check if there is a cross-origin top-level prefetch we can use for the navigation. The current solution uses the HTTP cache as a way to store the prefetch and to immediately use it for the navigation. However this solution fails in case the prefetch is not cacheable in the first place. Instead of this, skip storing in the HTTP cache and simulate a HTTP Cache entry to process the load.

[Rob Buis]

Created attachment 373466 [details]
Patch

[Rob Buis]

Created attachment 373481 [details]
Patch

[Rob Buis]

Created attachment 373507 [details]
Patch

[youenn fablet]

Comment on attachment 373507 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        not cacheable.

That might be often ok but I wonder whether there are some corner cases which would benefit for some kind of cache validation.
For instance, say the prefetched response has a Vary header and the prefetched request and real navigation request have a different header in this Vary header.
The main case might be if Vary header contains Cookie as the prefetched request will not have it while the navigation request will have it.

With the current model, a page should probably never try to prefetch another page with Vary header Cookie.
If that happens, I am unclear what we should do. The safest approach might be to not use the prefetch at all.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:228
>                      m_cache->store(request, entry->response, entry->releaseBuffer(), nullptr);

It might read better if we have just one !entry->redirectRequest.isNull() check.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:246
> +        loader->retrieveCacheEntryInternal(WTFMove(entry), request);

request can be moved here.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:250
> +void NetworkResourceLoader::retrieveCacheEntryInternal(std::unique_ptr<NetworkCache::Entry> entry, WebCore::ResourceRequest request)

entry should probably be an r-value.
request has well, or maybe a const reference.

> LayoutTests/http/tests/cache/resources/prefetched-main-resource-iframe.php:-3
> -    header('Cache-Control: max-age=3600');

I think it would be good to validate that a prefetched entry that is used and is cacheable actually goes in the cache.
This was guaranteed previously but with this patch, this is no longer the case.

[Kinuko Yasuda]

> For instance, say the prefetched response has a Vary header and the prefetched request and real navigation request have a different header in this Vary header.
> The main case might be if Vary header contains Cookie as the prefetched request will not have it while the navigation request will have it.

Fyi chromium's current impl also checks Vary headers when matching with prefetched requests, while skipping other validations (up to a certain period). I feel not reusing the prefetched resources if a Vary header Cookie exists makes sense, while this might need a spec discussion to conclude.

[Rob Buis]

Created attachment 373827 [details]
Patch

[Rob Buis]

Created attachment 373831 [details]
Patch

[EWS Watchlist]

Comment on attachment 373831 [details]
Patch

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

New failing tests:
http/tests/cache/link-prefetch-main-resource-skip-disk-cache.html

[EWS Watchlist]

Created attachment 373834 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 373835 [details]
Patch

[Rob Buis]

Comment on attachment 373507 [details]
Patch

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

>> Source/WebKit/ChangeLog:12
>> +        not cacheable.
> 
> That might be often ok but I wonder whether there are some corner cases which would benefit for some kind of cache validation.
> For instance, say the prefetched response has a Vary header and the prefetched request and real navigation request have a different header in this Vary header.
> The main case might be if Vary header contains Cookie as the prefetched request will not have it while the navigation request will have it.
> 
> With the current model, a page should probably never try to prefetch another page with Vary header Cookie.
> If that happens, I am unclear what we should do. The safest approach might be to not use the prefetch at all.

This came up in a different discussion thread. I think the best place for this is a new bug.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:228
>>                      m_cache->store(request, entry->response, entry->releaseBuffer(), nullptr);
> 
> It might read better if we have just one !entry->redirectRequest.isNull() check.

Makes sense, done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:246
>> +        loader->retrieveCacheEntryInternal(WTFMove(entry), request);
> 
> request can be moved here.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:250
>> +void NetworkResourceLoader::retrieveCacheEntryInternal(std::unique_ptr<NetworkCache::Entry> entry, WebCore::ResourceRequest request)
> 
> entry should probably be an r-value.
> request has well, or maybe a const reference.

Done.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource-iframe.php:-3
>> -    header('Cache-Control: max-age=3600');
> 
> I think it would be good to validate that a prefetched entry that is used and is cacheable actually goes in the cache.
> This was guaranteed previously but with this patch, this is no longer the case.

I did that by splitting up the test into one prefetched resource with Cache-Control and one without. Let me know if you can think of a better way though....

[youenn fablet]

Comment on attachment 373835 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:218
> +                std::unique_ptr<NetworkCache::Entry> cacheEntry;

I would remove this line and add 'auto cacheEntry = ' below.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:219
> +                ResourceRequest requestCopy { request };

Ditto here, I would inline it like loader->retrieveCacheEntryInternal(WTFMove(cacheEntry), ResourceRequest { request })

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-221
> -                } else

Could call 'return;' here and remove the else statement.

> LayoutTests/http/tests/cache/link-prefetch-main-resource-skip-disk-cache.html:14
> +<link rel="prefetch" href="http://localhost:8000/cache/resources/prefetched-main-resource-skip-disk-cache.php" onload="loadAfterPrefetch();">

We will probably want to remove unload event handler for prefetch.
I would tend to not use that one if possible.
Also, I would tend to try writing it as a testharness test so that we can later upstream it to WPT.
If testharness is not convenient, we should discuss improvements in WPT GitHub.

[Rob Buis]

Created attachment 374368 [details]
Patch

[EWS Watchlist]

Attachment 374368 [details] did not pass style-queue:


ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/mac-wk1/TestExpectations:736:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rob Buis]

Created attachment 374376 [details]
Patch

[Rob Buis]

Created attachment 374381 [details]
Patch

[EWS Watchlist]

Comment on attachment 374381 [details]
Patch

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

New failing tests:
storage/indexeddb/pending-version-change-on-exit-private.html

[EWS Watchlist]

Created attachment 374395 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 374397 [details]
Patch

[Rob Buis]

Comment on attachment 373835 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:218
>> +                std::unique_ptr<NetworkCache::Entry> cacheEntry;
> 
> I would remove this line and add 'auto cacheEntry = ' below.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:219
>> +                ResourceRequest requestCopy { request };
> 
> Ditto here, I would inline it like loader->retrieveCacheEntryInternal(WTFMove(cacheEntry), ResourceRequest { request })

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-221
>> -                } else
> 
> Could call 'return;' here and remove the else statement.

Done.

>> LayoutTests/http/tests/cache/link-prefetch-main-resource-skip-disk-cache.html:14
>> +<link rel="prefetch" href="http://localhost:8000/cache/resources/prefetched-main-resource-skip-disk-cache.php" onload="loadAfterPrefetch();">
> 
> We will probably want to remove unload event handler for prefetch.
> I would tend to not use that one if possible.
> Also, I would tend to try writing it as a testharness test so that we can later upstream it to WPT.
> If testharness is not convenient, we should discuss improvements in WPT GitHub.

Done, and moved the test to http/wpt. The timeouts are tricky but the best I can think of at the moment. Other ideas? WebDriver? postMessage?

[EWS Watchlist]

Comment on attachment 374397 [details]
Patch

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

New failing tests:
storage/indexeddb/pending-version-change-on-exit-private.html

[EWS Watchlist]

Created attachment 374417 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Rob Buis]

Created attachment 374596 [details]
Patch

[Rob Buis]

(In reply to youenn fablet from comment #4)
> That might be often ok but I wonder whether there are some corner cases
> which would benefit for some kind of cache validation.
> For instance, say the prefetched response has a Vary header and the
> prefetched request and real navigation request have a different header in
> this Vary header.
> The main case might be if Vary header contains Cookie as the prefetched
> request will not have it while the navigation request will have it.
> 
> With the current model, a page should probably never try to prefetch another
> page with Vary header Cookie.
> If that happens, I am unclear what we should do. The safest approach might
> be to not use the prefetch at all.

I started a bug for this (https://bugs.webkit.org/show_bug.cgi?id=200000).

[youenn fablet]

Comment on attachment 374596 [details]
Patch

Looks almost ready to me.
Some comments below.

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:220
>              if (auto entry = session->prefetchCache().take(request.url())) {

Let's add a FIXME with the bugzilla link to do some validation for the prefetch entry, for instance the Vary header.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:223
> +                    loader->retrieveCacheEntryInternal(WTFMove(cacheEntry), ResourceRequest {request});

s/ {request}/ { request }/

> LayoutTests/http/tests/cache/resources/prefetched-main-resource-iframe.php:3
>      header("Access-Control-Allow-Origin: http://127.0.0.1:8000");

Why do we need Access-Control-Allow-Origin header for the prefetch case?

> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:3
> +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");

Why do we need ACAO here?
Isn't prefetched-main-resource.php be loaded using http://localhost8000/cache/resources/prefetched-main-resource.php URL?

> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:13
> +      document.getElementById('log').innerText = 'FAIL';

Maybe we should make sure to have this 'FAIL' be different from below 'FAIL'.
Change it to 'FAIL: resource is not in the disk cache.' might help debugging.

> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:2
> +    headers = [("Content-Type", "text/html")]

This might depend on how we do cache validation, setting Cache-Control: no-store would probably defeat using the prefetch.
In that case, the prefetch code should probably cancel the load whenever receiving the response.

> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:7
> +  var ret = '%(prefetch)s';

s/ret/result/

> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:9
> +    fetch('%(url)s').then(function(response) {

You need to await here so that ret can go to 'FAIL'.
Otherwise you will call postMessage and fetch will then change ret.

> LayoutTests/http/wpt/prefetch/resources/navigate-skip-disk-cache.html:10
> +  setTimeout(() => { window.location = get_host_info().HTTP_REMOTE_ORIGIN + "/WebKit/prefetch/resources/main-resource-skip-disk-cache.py" }, 500);

If we would like to move away from 500, one possibility would be to add an Internals API that would tell when the link prefetch is done.

[Rob Buis]

Created attachment 374769 [details]
Patch

[Rob Buis]

Created attachment 374773 [details]
Patch

[Rob Buis]

Comment on attachment 374596 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:220
>>              if (auto entry = session->prefetchCache().take(request.url())) {
> 
> Let's add a FIXME with the bugzilla link to do some validation for the prefetch entry, for instance the Vary header.

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:223
>> +                    loader->retrieveCacheEntryInternal(WTFMove(cacheEntry), ResourceRequest {request});
> 
> s/ {request}/ { request }/

Done.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource-iframe.php:3
>>      header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> 
> Why do we need Access-Control-Allow-Origin header for the prefetch case?

You are right, I removed it.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:3
>> +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> 
> Why do we need ACAO here?
> Isn't prefetched-main-resource.php be loaded using http://localhost8000/cache/resources/prefetched-main-resource.php URL?

Ditto.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource.php:13
>> +      document.getElementById('log').innerText = 'FAIL';
> 
> Maybe we should make sure to have this 'FAIL' be different from below 'FAIL'.
> Change it to 'FAIL: resource is not in the disk cache.' might help debugging.

Done.

>> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:2
>> +    headers = [("Content-Type", "text/html")]
> 
> This might depend on how we do cache validation, setting Cache-Control: no-store would probably defeat using the prefetch.
> In that case, the prefetch code should probably cancel the load whenever receiving the response.

Do you mean, when a prefetch gets a response with Cache-Control: no-store, it should cancel the prefetch?

>> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:7
>> +  var ret = '%(prefetch)s';
> 
> s/ret/result/

Done.

>> LayoutTests/http/wpt/prefetch/resources/main-resource-skip-disk-cache.py:9
>> +    fetch('%(url)s').then(function(response) {
> 
> You need to await here so that ret can go to 'FAIL'.
> Otherwise you will call postMessage and fetch will then change ret.

Done.

>> LayoutTests/http/wpt/prefetch/resources/navigate-skip-disk-cache.html:10
>> +  setTimeout(() => { window.location = get_host_info().HTTP_REMOTE_ORIGIN + "/WebKit/prefetch/resources/main-resource-skip-disk-cache.py" }, 500);
> 
> If we would like to move away from 500, one possibility would be to add an Internals API that would tell when the link prefetch is done.

I added such an API. Unfortunately that makes it WPT incompatible. Still not sue how to fix that, WebDriver maybe?

[youenn fablet]

Comment on attachment 374773 [details]
Patch

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

> Source/WebCore/testing/Internals.h:32
> +#include "EventListener.h"

We probably do not need that one.

> Source/WebCore/testing/Internals.h:36
> +#include "JSEventListener.h"

Is this one needed to build JSInternals.cpp?
Could we instead do "class EventListener"?

[Rob Buis]

Created attachment 374887 [details]
Patch

[Rob Buis]

Created attachment 374888 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374888 [details]
Patch

Clearing flags on attachment: 374888

Committed r247821: <https://trac.webkit.org/changeset/247821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53543632>

[Truitt Savell]

Looks like the new test http/wpt/prefetch/link-prefetch-skip-disk-cache.html

added in https://trac.webkit.org/changeset/247821/webkit

is crashing along with another test: http/tests/cache/link-prefetch-main-resource.html

Combined history:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcache%2Flink-prefetch-main-resource.html%20http%2Fwpt%2Fprefetch%2Flink-prefetch-skip-disk-cache.html

Logs:
https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r247821%20(3754)/http/tests/cache/link-prefetch-main-resource-crash-log.txt
https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r247821%20(3754)/http/wpt/prefetch/link-prefetch-skip-disk-cache-crash-log.txt

[Truitt Savell]

Reverted r247821 for reason:

Caused two crashing Layout Tests

Committed r247825: <https://trac.webkit.org/changeset/247825>

[Rob Buis]

Created attachment 374898 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374898 [details]
Patch

Clearing flags on attachment: 374898

Committed r247860: <https://trac.webkit.org/changeset/247860>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.