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
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
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
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
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
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
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
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
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
(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 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 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 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.
> 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 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 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 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...
2019-04-30 06:35 PDT, Rob Buis
2019-04-30 07:40 PDT, EWS Watchlist
2019-04-30 07:55 PDT, EWS Watchlist
2019-04-30 08:17 PDT, EWS Watchlist
2019-04-30 08:22 PDT, EWS Watchlist
2019-04-30 08:37 PDT, EWS Watchlist
2019-05-08 07:04 PDT, Rob Buis
2019-05-08 08:11 PDT, EWS Watchlist
2019-05-08 08:53 PDT, EWS Watchlist
2019-05-08 08:55 PDT, EWS Watchlist
2019-05-08 09:01 PDT, Rob Buis
2019-05-08 13:40 PDT, EWS Watchlist
2019-05-09 11:44 PDT, Rob Buis
2019-05-13 08:18 PDT, Rob Buis
2019-06-10 01:27 PDT, Rob Buis
2019-06-11 10:45 PDT, Rob Buis
2019-06-15 09:39 PDT, Rob Buis