Verify Prefetch and credential behavior.
Created attachment 374600 [details] Patch
Created attachment 375411 [details] Patch
Comment on attachment 375411 [details] Patch Attachment 375411 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12852365 New failing tests: http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html
Created attachment 375413 [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 on attachment 375411 [details] Patch Attachment 375411 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12852468 New failing tests: http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html
Created attachment 375419 [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 on attachment 375411 [details] Patch Attachment 375411 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12852413 New failing tests: http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html
Created attachment 375420 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 375426 [details] Patch
Created attachment 375448 [details] Patch
Comment on attachment 375448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375448&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:221 > + if (!entry->response.httpHeaderField(WebCore::HTTPHeaderName::Vary).contains("Cookie")) { I would move that check in the prefetch cache which is responsible of validating that the navigation request is matching with the prefetch request. Probably as part of PrefetchCache::take and it would be given the request and not just the request.url(). If we decide to not prefetch any vary: cookie header, we should cancel these loads as soon as we get such response. A case to consider is the case where we are starting the navigation load but we do not have yet the response.
Created attachment 375523 [details] Patch
Comment on attachment 375448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375448&action=review >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:221 >> + if (!entry->response.httpHeaderField(WebCore::HTTPHeaderName::Vary).contains("Cookie")) { > > I would move that check in the prefetch cache which is responsible of validating that the navigation request is matching with the prefetch request. > Probably as part of PrefetchCache::take and it would be given the request and not just the request.url(). > > If we decide to not prefetch any vary: cookie header, we should cancel these loads as soon as we get such response. > > A case to consider is the case where we are starting the navigation load but we do not have yet the response. I moved the check, however I turned it in an ASSERT now, since I like the idea of canceling the prefetch and vary: cookie header. The case you mention sounds related to https://bugs.webkit.org/show_bug.cgi?id=199162, which should probably go in first.
Comment on attachment 375523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375523&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:502 > + if (response.httpHeaderField(WebCore::HTTPHeaderName::Vary).contains("Cookie")) { WebCore:: not needed. > Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:60 > +std::unique_ptr<PrefetchCache::Entry> PrefetchCache::take(const WebCore::ResourceRequest& request) Since we do not check request headers or cookies, we can keep passing a URL. > LayoutTests/http/wpt/prefetch/resources/navigate-cross-origin-vary-cookie.html:15 > + await fetch(get_host_info().HTTP_REMOTE_ORIGIN + "/WebKit/prefetch/resources/main-resource-cross-origin-set-cookie.py", { "credentials": "include" }); Why do we need this one? Is it just to wait for the link prefetch to finish? Why do we need another py script for it? Can we remove main-resource-cross-origin-set-cookie.py? > LayoutTests/platform/mac-wk1/TestExpectations:741 > +webkit.org/b/200000 http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html [ Skip ] Since prefetch is only WK2, I would just add one comment saying prefetch not supported in WK1 and would not add a link to webkit.org/b/200000
Created attachment 376785 [details] Patch
Comment on attachment 375523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375523&action=review >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:502 >> + if (response.httpHeaderField(WebCore::HTTPHeaderName::Vary).contains("Cookie")) { > > WebCore:: not needed. Done. >> Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp:60 >> +std::unique_ptr<PrefetchCache::Entry> PrefetchCache::take(const WebCore::ResourceRequest& request) > > Since we do not check request headers or cookies, we can keep passing a URL. Fixed. >> LayoutTests/http/wpt/prefetch/resources/navigate-cross-origin-vary-cookie.html:15 >> + await fetch(get_host_info().HTTP_REMOTE_ORIGIN + "/WebKit/prefetch/resources/main-resource-cross-origin-set-cookie.py", { "credentials": "include" }); > > Why do we need this one? Is it just to wait for the link prefetch to finish? > Why do we need another py script for it? Can we remove main-resource-cross-origin-set-cookie.py? It is needed to set the cookie. I don't think document.cookie can work in this case. Let me know if you see some other way than using main-resource-cross-origin-set-cookie.py. >> LayoutTests/platform/mac-wk1/TestExpectations:741 >> +webkit.org/b/200000 http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html [ Skip ] > > Since prefetch is only WK2, I would just add one comment saying prefetch not supported in WK1 and would not add a link to webkit.org/b/200000 Done.
Comment on attachment 376785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376785&action=review > LayoutTests/http/wpt/prefetch/resources/main-resource-cross-origin-set-cookie.py:11 > +</script> I was initially confused about this script element and the fact the resource is loaded with fetch API. Maybe it should be removed or the response could just be something like 'PASS' or 'Setting Cookie'. > LayoutTests/http/wpt/prefetch/resources/main-resource-cross-origin-vary-cookie.py:9 > + window.opener.postMessage(result, '*'); indentation > LayoutTests/platform/mac-wk1/TestExpectations:742 > +http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html [ Skip ] It seems you could skip the whole http/wpt/prefetch folder here. And put http/tests/cache/link-prefetch-main-resource.html and http/tests/cache/link-prefetch-main-resource-iframe.html skip lines below the '# prefetch not supported in WK1'
Created attachment 376865 [details] Patch
Comment on attachment 376785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376785&action=review >> LayoutTests/http/wpt/prefetch/resources/main-resource-cross-origin-set-cookie.py:11 >> +</script> > > I was initially confused about this script element and the fact the resource is loaded with fetch API. > Maybe it should be removed or the response could just be something like 'PASS' or 'Setting Cookie'. Right, sorry, that was a left over from an earlier test, removed now. >> LayoutTests/http/wpt/prefetch/resources/main-resource-cross-origin-vary-cookie.py:9 >> + window.opener.postMessage(result, '*'); > > indentation Done. >> LayoutTests/platform/mac-wk1/TestExpectations:742 >> +http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html [ Skip ] > > It seems you could skip the whole http/wpt/prefetch folder here. > And put http/tests/cache/link-prefetch-main-resource.html and http/tests/cache/link-prefetch-main-resource-iframe.html skip lines below the '# prefetch not supported in WK1' Done.
Comment on attachment 376865 [details] Patch Rejecting attachment 376865 [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', 'validate-changelog', '--check-oops', '--non-interactive', 376865, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/12951471
Created attachment 376873 [details] Patch
Comment on attachment 376873 [details] Patch Clearing flags on attachment: 376873 Committed r248942: <https://trac.webkit.org/changeset/248942>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54556104>
This patch has caused Layout Tests to crash and/or fail. Crashing Tests (logs attached): http/tests/cache/link-prefetch-main-resource.html http/wpt/prefetch/link-prefetch-main-resource-redirect.html Results History: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=http%2Fwpt%2Fprefetch%2Flink-prefetch-main-resource-redirect.html%20http%2Ftests%2Fcache%2Flink-prefetch-main-resource.html
Created attachment 376899 [details] link-prefetch-main-resource-redirect-crash-log.txt
Created attachment 376900 [details] link-prefetch-main-resource-crash-log.txt
Reverted r248942 for reason: Causes multiple layout test crashes on MacOS Bots Committed r248953: <https://trac.webkit.org/changeset/248953>
Created attachment 376902 [details] Patch
Comment on attachment 376902 [details] Patch Clearing flags on attachment: 376902 Committed r248961: <https://trac.webkit.org/changeset/248961>
Reverted r248961 for reason: Same patch was re-landed after being rolled out. Patch is causing Catalina/iOS 13 test failures. Rolling out. Committed r249101: <https://trac.webkit.org/changeset/249101>
(In reply to Russell Epstein from comment #32) > Reverted r248961 for reason: > > Same patch was re-landed after being rolled out. It was not the same patch, the ASSERT was different. Maybe it still has a logic error, will check.
(In reply to Russell Epstein from comment #32) > Reverted r248961 for reason: > > Same patch was re-landed after being rolled out. Patch is causing > Catalina/iOS 13 test failures. Rolling out. > > Committed r249101: <https://trac.webkit.org/changeset/249101> What are the test failures you mention above? The two original tests that crashes were fixed if I interpret this correctly: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=http%2Fwpt%2Fprefetch%2Flink-prefetch-main-resource-redirect.html%20http%2Ftests%2Fcache%2Flink-prefetch-main-resource.html
(In reply to Rob Buis from comment #34) > (In reply to Russell Epstein from comment #32) > > Reverted r248961 for reason: > > > > Same patch was re-landed after being rolled out. Patch is causing > > Catalina/iOS 13 test failures. Rolling out. > > > > Committed r249101: <https://trac.webkit.org/changeset/249101> > > What are the test failures you mention above? The two original tests that > crashes were fixed if I interpret this correctly: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#tests=http%2Fwpt%2Fprefetch%2Flink-prefetch-main-resource-redirect. > html%20http%2Ftests%2Fcache%2Flink-prefetch-main-resource.html http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html iOS 13 and Catalina Diff: @@ -1,3 +1,3 @@ -PASS Verify that navigating to a prefetched main resource that sets Vary: Cookie does send cookies. +FAIL Verify that navigating to a prefetched main resource that sets Vary: Cookie does send cookies. assert_equals: expected "foo=bar" but got ""
(In reply to Russell Epstein from comment #35) > -PASS Verify that navigating to a prefetched main resource that sets Vary: > Cookie does send cookies. > +FAIL Verify that navigating to a prefetched main resource that sets Vary: > Cookie does send cookies. assert_equals: expected "foo=bar" but got "" Hi, I think it's difficult to detect this kind of issue without a public buildbot and even harder to debug this without access to the Catalina / iOS13 configuration. Is there any such bot? Is it possible to build that configuration for external contributors (with an Apple developer account)? See also bug 199705 comment 8.
Yes, installing a macOS Catalina developer beta, building WebKit and running tests on it should all work.
(In reply to Alexey Proskuryakov from comment #37) > Yes, installing a macOS Catalina developer beta, building WebKit and running > tests on it should all work. Thanks, I had some problems with file privileges but now I can reproduce. I have to do some more verifying but pretty sure the test runs into https://bugs.webkit.org/show_bug.cgi?id=200857.
Created attachment 380197 [details] Patch
Since this runs into https://bugs.webkit.org/show_bug.cgi?id=200857 on OS X Catalina, I think the best workaround for now is to change the test to check that the prefetch is canceled instead of checking the actual cookie, which I implemented in my latest patch.
Created attachment 380614 [details] Patch
Comment on attachment 380614 [details] Patch Attachment 380614 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13114421 New failing tests: fast/images/reset-image-animation.html http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html
Created attachment 380655 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 381200 [details] Patch
(In reply to Rob Buis from comment #40) > Since this runs into https://bugs.webkit.org/show_bug.cgi?id=200857 on OS X > Catalina, I think the best workaround for now is to change the test to check > that the prefetch is canceled instead of checking the actual cookie, which I > implemented in my latest patch. New status, on Catalina OS X the old testcase passes (buildbot and locally 10.15.1 Beta (19B68f)) with latest updates. On iOS buildbot it fails, seems like it is using 13.1, whereas (https://bugs.webkit.org/show_bug.cgi?id=200857) confirmed it is fixed on iOS 13.2 Developer beta 2. Are there plans to update iOS-13-Simulator-WK2-Tests-EWS soon?
No plans to update to pre-release builds at this time.
All of our simulator bots are on 10.15.3 now. Triggering EWS retry.
(In reply to Alexey Proskuryakov from comment #47) > All of our simulator bots are on 10.15.3 now. Triggering EWS retry. Thanks for the notifivation There seems to be an EWS problem. Regardless, I'll rebase the patch.
(In reply to Rob Buis from comment #48) > Thanks for the notifivation There seems to be an EWS problem. Regardless, I'll rebase the patch. Yeah, the patch needs a rebase. I retried the ios-sim build, and it failed to apply the patch in https://ews-build.webkit.org/#/builders/23/builds/11185 ios-wk2 failed to retry since it's been too long since the patch was built and we store the built archive only for 2 weeks.
Created attachment 391416 [details] Patch
Comment on attachment 391416 [details] Patch LGTM View in context: https://bugs.webkit.org/attachment.cgi?id=391416&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:551 > + if (response.httpHeaderField(HTTPHeaderName::Vary).contains("Cookie")) { Might be good to add a RELEASE_LOG here as well.
Created attachment 391526 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 391526 [details]: imported/w3c/web-platform-tests/IndexedDB/interleaved-cursors-large.html bug 201849 The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 391526 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 391526 [details] Patch Rejecting attachment 391526 [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-02', 'apply-attachment', '--no-update', '--non-interactive', 391526, '--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=391526&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=200000&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Processing patch 391526 from bug 200000. Fetching: https://bugs.webkit.org/attachment.cgi?id=391526 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 11 diffs from patch file(s). patching file Source/WebKit/ChangeLog patching file Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp patching file Source/WebKit/NetworkProcess/cache/PrefetchCache.cpp patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie-expected.txt patching file LayoutTests/http/wpt/prefetch/link-prefetch-cross-origin-vary-cookie.html patching file LayoutTests/http/wpt/prefetch/resources/main-resource-cross-origin-set-cookie.py patching file LayoutTests/http/wpt/prefetch/resources/main-resource-cross-origin-vary-cookie.py patching file LayoutTests/http/wpt/prefetch/resources/navigate-cross-origin-vary-cookie.html patching file LayoutTests/platform/mac-wk1/TestExpectations Hunk #1 FAILED at 950. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk1/TestExpectations.rej patching file LayoutTests/platform/win/TestExpectations 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/13327823
Created attachment 391539 [details] Patch
Comment on attachment 391539 [details] Patch Clearing flags on attachment: 391539 Committed r257211: <https://trac.webkit.org/changeset/257211>