Bug 199499

Created attachment 373466 [details] Patch

Created attachment 373481 [details] Patch

Created attachment 373507 [details] Patch

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.

> 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.

Created attachment 373827 [details] Patch

Created attachment 373831 [details] Patch

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

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

Created attachment 373835 [details] Patch

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....

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.

Created attachment 374368 [details] Patch

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.

Created attachment 374376 [details] Patch

Created attachment 374381 [details] Patch

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

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

Created attachment 374397 [details] Patch

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?

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

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

Created attachment 374596 [details] Patch

(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).

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.

Created attachment 374769 [details] Patch

Created attachment 374773 [details] Patch

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?

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"?

Created attachment 374887 [details] Patch

Created attachment 374888 [details] Patch

Comment on attachment 374888 [details] Patch Clearing flags on attachment: 374888 Committed r247821: <https://trac.webkit.org/changeset/247821>

All reviewed patches have been landed. Closing bug.

<rdar://problem/53543632>

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

Reverted r247821 for reason: Caused two crashing Layout Tests Committed r247825: <https://trac.webkit.org/changeset/247825>

Created attachment 374898 [details] Patch

Comment on attachment 374898 [details] Patch Clearing flags on attachment: 374898 Committed r247860: <https://trac.webkit.org/changeset/247860>

All reviewed patches have been landed. Closing bug.