Verify Prefetch and credential behavior

• Component: Page Loading
• Status: RESOLVED FIXED

[Rob Buis]

Verify Prefetch and credential behavior.

[Rob Buis]

Created attachment 374600 [details]
Patch

[Rob Buis]

Created attachment 375411 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 375426 [details]
Patch

[Rob Buis]

Created attachment 375448 [details]
Patch

[youenn fablet]

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.

[Rob Buis]

Created attachment 375523 [details]
Patch

[Rob Buis]

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.

[youenn fablet]

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

[Rob Buis]

Created attachment 376785 [details]
Patch

[Rob Buis]

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.

[youenn fablet]

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'

[Rob Buis]

Created attachment 376865 [details]
Patch

[Rob Buis]

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.

[WebKit Commit Bot]

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

[Rob Buis]

Created attachment 376873 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376873 [details]
Patch

Clearing flags on attachment: 376873

Committed r248942: <https://trac.webkit.org/changeset/248942>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54556104>

[Russell Epstein]

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

[Russell Epstein]

Created attachment 376899 [details]
link-prefetch-main-resource-redirect-crash-log.txt

[Russell Epstein]

Created attachment 376900 [details]
link-prefetch-main-resource-crash-log.txt

[Russell Epstein]

Reverted r248942 for reason:

Causes multiple layout test crashes on MacOS Bots

Committed r248953: <https://trac.webkit.org/changeset/248953>

[Rob Buis]

Created attachment 376902 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376902 [details]
Patch

Clearing flags on attachment: 376902

Committed r248961: <https://trac.webkit.org/changeset/248961>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Russell Epstein]

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>

[Rob Buis]

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

[Rob Buis]

(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

[Russell Epstein]

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

[Frédéric Wang (:fredw)]

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

[Alexey Proskuryakov]

Yes, installing a macOS Catalina developer beta, building WebKit and running tests on it should all work.

[Rob Buis]

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

[Rob Buis]

Created attachment 380197 [details]
Patch

[Rob Buis]

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.

[Rob Buis]

Created attachment 380614 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 381200 [details]
Patch

[Rob Buis]

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

[Alexey Proskuryakov]

No plans to update to pre-release builds at this time.

[Alexey Proskuryakov]

All of our simulator bots are on 10.15.3 now. Triggering EWS retry.

[Rob Buis]

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

[Aakash Jain]

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

[Rob Buis]

Created attachment 391416 [details]
Patch

[youenn fablet]

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.

[Rob Buis]

Created attachment 391526 [details]
Patch

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 391526 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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 [e-mail]...
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

[Rob Buis]

Created attachment 391539 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391539 [details]
Patch

Clearing flags on attachment: 391539

Committed r257211: <https://trac.webkit.org/changeset/257211>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.