Bug 197371 - Store prefetch redirects in the prefetch cache
Summary: Store prefetch redirects in the prefetch cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on: 195623
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-29 08:57 PDT by Rob Buis
Modified: 2019-06-15 11:21 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.79 KB, patch)
2019-04-30 06:35 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.07 MB, application/zip)
2019-04-30 07:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.63 MB, application/zip)
2019-04-30 07:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (12.92 MB, application/zip)
2019-04-30 08:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.99 MB, application/zip)
2019-04-30 08:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (8.30 MB, application/zip)
2019-04-30 08:37 PDT, Build Bot
no flags Details
Patch (4.49 KB, patch)
2019-05-08 07:04 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.10 MB, application/zip)
2019-05-08 08:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.91 MB, application/zip)
2019-05-08 08:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews214 for win-future (13.59 MB, application/zip)
2019-05-08 08:55 PDT, Build Bot
no flags Details
Patch (5.86 KB, patch)
2019-05-08 09:01 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (13.35 MB, application/zip)
2019-05-08 13:40 PDT, Build Bot
no flags Details
Patch (6.03 KB, patch)
2019-05-09 11:44 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (15.29 KB, patch)
2019-05-13 08:18 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (16.05 KB, patch)
2019-06-10 01:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (16.83 KB, patch)
2019-06-11 10:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (16.71 KB, patch)
2019-06-15 09:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Rob Buis 2019-04-30 06:35:17 PDT
Created attachment 368556 [details]
Patch
Comment 2 Build Bot 2019-04-30 07:39:59 PDT
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
Comment 3 Build Bot 2019-04-30 07:40:00 PDT
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
Comment 4 Build Bot 2019-04-30 07:55:04 PDT
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
Comment 5 Build Bot 2019-04-30 07:55:06 PDT
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
Comment 6 Build Bot 2019-04-30 08:17:11 PDT
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
Comment 7 Build Bot 2019-04-30 08:17:22 PDT
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
Comment 8 Build Bot 2019-04-30 08:22:48 PDT
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
Comment 9 Build Bot 2019-04-30 08:22:50 PDT
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
Comment 10 Build Bot 2019-04-30 08:37:24 PDT
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
Comment 11 Build Bot 2019-04-30 08:37:26 PDT
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
Comment 12 Rob Buis 2019-05-08 07:04:53 PDT
Created attachment 369382 [details]
Patch
Comment 13 Build Bot 2019-05-08 08:10:59 PDT
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
Comment 14 Build Bot 2019-05-08 08:11:01 PDT
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
Comment 15 Build Bot 2019-05-08 08:53:43 PDT
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
Comment 16 Build Bot 2019-05-08 08:53:45 PDT
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
Comment 17 Build Bot 2019-05-08 08:55:41 PDT
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
Comment 18 Build Bot 2019-05-08 08:55:45 PDT
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
Comment 19 Rob Buis 2019-05-08 09:01:23 PDT
Created attachment 369388 [details]
Patch
Comment 20 Build Bot 2019-05-08 13:40:40 PDT
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
Comment 21 Build Bot 2019-05-08 13:40:42 PDT
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
Comment 22 Alex Christensen 2019-05-09 10:28:27 PDT
Comment on attachment 369388 [details]
Patch

This does more than just add tests.  I think the title is wrong.
Comment 23 Rob Buis 2019-05-09 11:43:52 PDT
(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.
Comment 24 Rob Buis 2019-05-09 11:44:26 PDT
Created attachment 369504 [details]
Patch
Comment 25 youenn fablet 2019-05-10 15:55:29 PDT
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.
Comment 26 Rob Buis 2019-05-13 08:18:05 PDT
Created attachment 369733 [details]
Patch
Comment 27 Rob Buis 2019-05-13 10:41:33 PDT
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.
Comment 28 youenn fablet 2019-05-13 13:28:28 PDT
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.
Comment 29 youenn fablet 2019-05-13 13:28:51 PDT
> 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.
Comment 30 Rob Buis 2019-06-10 01:27:43 PDT
Created attachment 371728 [details]
Patch
Comment 31 youenn fablet 2019-06-10 08:26:12 PDT
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.
Comment 32 Rob Buis 2019-06-10 12:39:19 PDT
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?
Comment 33 Rob Buis 2019-06-11 10:45:03 PDT
Created attachment 371854 [details]
Patch
Comment 34 Rob Buis 2019-06-11 13:58:27 PDT
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...
Comment 35 WebKit Commit Bot 2019-06-15 09:18:52 PDT
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 commit-queue@webkit.org...
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
Comment 36 Rob Buis 2019-06-15 09:39:22 PDT
Created attachment 372194 [details]
Patch
Comment 37 WebKit Commit Bot 2019-06-15 11:20:33 PDT
Comment on attachment 372194 [details]
Patch

Clearing flags on attachment: 372194

Committed r246466: <https://trac.webkit.org/changeset/246466>
Comment 38 WebKit Commit Bot 2019-06-15 11:20:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Radar WebKit Bug Importer 2019-06-15 11:21:29 PDT
<rdar://problem/51778522>