Bug 197371 – Store prefetch redirects in the prefetch cache

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

[Rob Buis]

See https://bugs.webkit.org/show_bug.cgi?id=195623, comment 176.

[Rob Buis]

Created attachment 368556 [details]
Patch

[EWS Watchlist]

Comment on attachment 368556 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 368559 [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

[EWS Watchlist]

Comment on attachment 368556 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 368560 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

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

[EWS Watchlist]

Comment on attachment 368556 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 368565 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 368556 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 368566 [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

[EWS Watchlist]

Comment on attachment 368556 [details]
Patch

Attachment 368556 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12040732

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

[EWS Watchlist]

Created attachment 368568 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.4

[Rob Buis]

Created attachment 369382 [details]
Patch

[EWS Watchlist]

Comment on attachment 369382 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 369384 [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

[EWS Watchlist]

Comment on attachment 369382 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 369386 [details]
Archive of layout-test-results from ews115 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 369382 [details]
Patch

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

New failing tests:
http/tests/cache/memory-cache-pruning.html
http/tests/cache/link-prefetch-main-resource-redirect.html

[EWS Watchlist]

Created attachment 369387 [details]
Archive of layout-test-results from ews214 for win-future

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

[Rob Buis]

Created attachment 369388 [details]
Patch

[EWS Watchlist]

Comment on attachment 369388 [details]
Patch

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

New failing tests:
svg/repaint/remove-border-property-on-root.html
js/dom/custom-constructors.html

[EWS Watchlist]

Created attachment 369418 [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

[Alex Christensen]

Comment on attachment 369388 [details]
Patch

This does more than just add tests.  I think the title is wrong.

[Rob Buis]

(In reply to Alex Christensen from comment #22)
> Comment on attachment 369388 [details]
> Patch
> 
> This does more than just add tests.  I think the title is wrong.

Fair enough, will fix.

[Rob Buis]

Created attachment 369504 [details]
Patch

[youenn fablet]

Comment on attachment 369504 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:695
> +            session->prefetchCache().store(m_networkLoad->currentRequest().url(), WTFMove(m_response), WTFMove(m_bufferedDataForCache));

If we navigate to that url, we should probably call m_cache->storeRedirect as done in NetworkResourceLoader::willSendRedirectedRequest.

> LayoutTests/http/tests/cache/resources/prefetched-main-resource-redirect.php:3
> +    header('HTTP/1.1 ' . 200);

It seems this script is not testing what it supposed to do.
What the script could do:
- Always return a 302, with max-age 3600
- In case of ($_SERVER["HTTP_PURPOSE"] == "prefetch"), redirect to another script B. Script B would return FAIL in case $_SERVER["HTTP_PURPOSE"] == "prefetch" and PASS otherwise.
- Otherwise, redirect to a file containing FAIL.

There is probably then no need for <html> content in this specific script.

I also think we should testharnessify these tests to upload them to WPT as other browsers might find them useful when implementing this.

[Rob Buis]

Created attachment 369733 [details]
Patch

[Rob Buis]

Comment on attachment 369504 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:695
>> +            session->prefetchCache().store(m_networkLoad->currentRequest().url(), WTFMove(m_response), WTFMove(m_bufferedDataForCache));
> 
> If we navigate to that url, we should probably call m_cache->storeRedirect as done in NetworkResourceLoader::willSendRedirectedRequest.

Good point, I added a new storeRedirect method to store the redirect in the prefetch cache, and call m_cache->storeRedirect now when navigating.

>> LayoutTests/http/tests/cache/resources/prefetched-main-resource-redirect.php:3
>> +    header('HTTP/1.1 ' . 200);
> 
> It seems this script is not testing what it supposed to do.
> What the script could do:
> - Always return a 302, with max-age 3600
> - In case of ($_SERVER["HTTP_PURPOSE"] == "prefetch"), redirect to another script B. Script B would return FAIL in case $_SERVER["HTTP_PURPOSE"] == "prefetch" and PASS otherwise.
> - Otherwise, redirect to a file containing FAIL.
> 
> There is probably then no need for <html> content in this specific script.
> 
> I also think we should testharnessify these tests to upload them to WPT as other browsers might find them useful when implementing this.

I rewrote the test according to above bullet points. However not too sure how to testharnessify, does that involve rewriting the scripts from .php to .py? Would the notifyDone be replaced by onload handlers? I wonder if fetch/ has some inspiration.

[youenn fablet]

Comment on attachment 369733 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:690
>      didReceiveResponse(WTFMove(redirectResponse), [] (auto) { });

In case it is a cross origin prefetch, we probably do not want to expose redirectResponse to WebProcess.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.h:61
> +    void storeRedirect(const URL&, WebCore::ResourceResponse&&, WebCore::ResourceRequest &&);

s/ &&/&&/

> LayoutTests/http/tests/cache/resources/main-resource-redirect-no-prefetch.php:14
> +if ($_SERVER["HTTP_PURPOSE"] == "prfetch") {

prefetch.

> LayoutTests/http/tests/cache/resources/prefetched-main-resource-redirect.php:7
> +        print('FAIL');

In that case, maybe just return a 200 and not a 302 without Location.

[youenn fablet]

> I rewrote the test according to above bullet points. However not too sure
> how to testharnessify, does that involve rewriting the scripts from .php to
> .py? Would the notifyDone be replaced by onload handlers? I wonder if fetch/
> has some inspiration.

Yes, move the tests and scripts to LayoutTests/http/wpt/XXX
They will be served from localhost::8800/WebKit/XXX
And move from php to python.

[Rob Buis]

Created attachment 371728 [details]
Patch

[youenn fablet]

Comment on attachment 371728 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:701
> +    if (isCrossOriginPrefetch()) {

Maybe move the code below as an else to if(!isCrossOriginPrefetch())...

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:703
> +            redirectRequest.clearPurpose();

I would tend to move this to prefetch cache implementation.

> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:90
> +    m_sessionPrefetches->set(requestUrl, std::make_unique<PrefetchCache::Entry>(WTFMove(redirectResponse), WTFMove(redirectRequest)));

PrefetchCache::store uses add, here it is using set.
I guess set makes more sense so that we only keep the freshest one.
A test would be good around there for redirections and other responses.

> LayoutTests/http/tests/cache/link-prefetch-main-resource-redirect.html:3
> +if (window.testRunner) {

Other browsers are interested in that functionality.
It would be nice as a testharness based test so that we can easily upstream it to WPT.

[Rob Buis]

Comment on attachment 371728 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:90
>> +    m_sessionPrefetches->set(requestUrl, std::make_unique<PrefetchCache::Entry>(WTFMove(redirectResponse), WTFMove(redirectRequest)));
> 
> PrefetchCache::store uses add, here it is using set.
> I guess set makes more sense so that we only keep the freshest one.
> A test would be good around there for redirections and other responses.

Actually I remember the add in PrefetchCache::store was done to limit the number of prefetches for the same url to 1. I guess the same thing needs to be done for redirects?

[Rob Buis]

Created attachment 371854 [details]
Patch

[Rob Buis]

Comment on attachment 371728 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:701
>> +    if (isCrossOriginPrefetch()) {
> 
> Maybe move the code below as an else to if(!isCrossOriginPrefetch())...

Done.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:703
>> +            redirectRequest.clearPurpose();
> 
> I would tend to move this to prefetch cache implementation.

Done.

>> LayoutTests/http/tests/cache/link-prefetch-main-resource-redirect.html:3
>> +if (window.testRunner) {
> 
> Other browsers are interested in that functionality.
> It would be nice as a testharness based test so that we can easily upstream it to WPT.

Done. Turned out to be quite hard to write...

[WebKit Commit Bot]

Comment on attachment 371854 [details]
Patch

Rejecting attachment 371854 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 371854, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=371854&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=197371&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 371854 from bug 197371.
Fetching: https://bugs.webkit.org/attachment.cgi?id=371854
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 16 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/network/ResourceRequestBase.cpp
patching file Source/WebCore/platform/network/ResourceRequestBase.h
patching file Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
patching file Source/WebKit/NetworkProcess/NetworkResourceLoader.h
patching file Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp
patching file Source/WebKit/NetworkProcess/cache/PrefetchCache.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/http/wpt/prefetch/link-prefetch-main-resource-redirect-expected.txt
patching file LayoutTests/http/wpt/prefetch/link-prefetch-main-resource-redirect.html
patching file LayoutTests/http/wpt/prefetch/resources/main-resource-redirect-no-prefetch.py
patching file LayoutTests/http/wpt/prefetch/resources/navigate.html
patching file LayoutTests/http/wpt/prefetch/resources/prefetched-main-resource-redirect.py
patching file LayoutTests/platform/mac-wk1/TestExpectations
Hunk #1 FAILED at 701.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk1/TestExpectations.rej
patching file LayoutTests/platform/win/TestExpectations
Hunk #1 succeeded at 4416 (offset 16 lines).

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/12483924

[Rob Buis]

Created attachment 372194 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 372194 [details]
Patch

Clearing flags on attachment: 372194

Committed r246466: <https://trac.webkit.org/changeset/246466>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51778522>