Bug 200000 - Verify Prefetch and credential behavior
Summary: Verify Prefetch and credential behavior
Status: REOPENED
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:
Blocks:
 
Reported: 2019-07-22 08:42 PDT by Rob Buis
Modified: 2019-10-18 09:04 PDT (History)
11 users (show)

See Also:


Attachments
Patch (6.13 KB, patch)
2019-07-22 08:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.97 KB, patch)
2019-08-02 08:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.19 MB, application/zip)
2019-08-02 09:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews212 for win-future (13.59 MB, application/zip)
2019-08-02 10:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.02 MB, application/zip)
2019-08-02 10:15 PDT, Build Bot
no flags Details
Patch (9.63 KB, patch)
2019-08-02 10:52 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.49 KB, patch)
2019-08-02 13:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.63 KB, patch)
2019-08-05 00:11 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.48 KB, patch)
2019-08-20 11:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.82 KB, patch)
2019-08-21 05:37 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.86 KB, patch)
2019-08-21 06:42 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
link-prefetch-main-resource-redirect-crash-log.txt (78.87 KB, text/plain)
2019-08-21 11:38 PDT, Russell Epstein
no flags Details
link-prefetch-main-resource-crash-log.txt (74.77 KB, text/plain)
2019-08-21 11:39 PDT, Russell Epstein
no flags Details
Patch (10.79 KB, patch)
2019-08-21 12:20 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.04 KB, patch)
2019-10-04 01:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.01 KB, patch)
2019-10-10 02:11 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.82 MB, application/zip)
2019-10-10 10:05 PDT, Build Bot
no flags Details
Patch (10.19 KB, patch)
2019-10-17 10:30 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.
Description Rob Buis 2019-07-22 08:42:09 PDT
Verify Prefetch and credential behavior.
Comment 1 Rob Buis 2019-07-22 08:45:24 PDT
Created attachment 374600 [details]
Patch
Comment 2 Rob Buis 2019-08-02 08:21:33 PDT
Created attachment 375411 [details]
Patch
Comment 3 Build Bot 2019-08-02 09:31:54 PDT
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
Comment 4 Build Bot 2019-08-02 09:31:56 PDT
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 5 Build Bot 2019-08-02 10:14:49 PDT
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
Comment 6 Build Bot 2019-08-02 10:14:53 PDT
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 7 Build Bot 2019-08-02 10:15:35 PDT
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
Comment 8 Build Bot 2019-08-02 10:15:37 PDT
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
Comment 9 Rob Buis 2019-08-02 10:52:21 PDT
Created attachment 375426 [details]
Patch
Comment 10 Rob Buis 2019-08-02 13:33:56 PDT
Created attachment 375448 [details]
Patch
Comment 11 youenn fablet 2019-08-02 15:55:41 PDT
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.
Comment 12 Rob Buis 2019-08-05 00:11:34 PDT
Created attachment 375523 [details]
Patch
Comment 13 Rob Buis 2019-08-05 10:44:49 PDT
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 14 youenn fablet 2019-08-20 09:44:46 PDT
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
Comment 15 Rob Buis 2019-08-20 11:28:32 PDT
Created attachment 376785 [details]
Patch
Comment 16 Rob Buis 2019-08-20 14:02:46 PDT
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 17 youenn fablet 2019-08-21 02:59:14 PDT
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'
Comment 18 Rob Buis 2019-08-21 05:37:22 PDT
Created attachment 376865 [details]
Patch
Comment 19 Rob Buis 2019-08-21 06:27:56 PDT
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 20 WebKit Commit Bot 2019-08-21 06:30:58 PDT
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
Comment 21 Rob Buis 2019-08-21 06:42:40 PDT
Created attachment 376873 [details]
Patch
Comment 22 WebKit Commit Bot 2019-08-21 08:15:37 PDT
Comment on attachment 376873 [details]
Patch

Clearing flags on attachment: 376873

Committed r248942: <https://trac.webkit.org/changeset/248942>
Comment 23 WebKit Commit Bot 2019-08-21 08:15:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2019-08-21 08:16:30 PDT
<rdar://problem/54556104>
Comment 25 Russell Epstein 2019-08-21 11:37:41 PDT
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
Comment 26 Russell Epstein 2019-08-21 11:38:58 PDT
Created attachment 376899 [details]
link-prefetch-main-resource-redirect-crash-log.txt
Comment 27 Russell Epstein 2019-08-21 11:39:39 PDT
Created attachment 376900 [details]
link-prefetch-main-resource-crash-log.txt
Comment 28 Russell Epstein 2019-08-21 11:48:20 PDT
Reverted r248942 for reason:

Causes multiple layout test crashes on MacOS Bots

Committed r248953: <https://trac.webkit.org/changeset/248953>
Comment 29 Rob Buis 2019-08-21 12:20:18 PDT
Created attachment 376902 [details]
Patch
Comment 30 WebKit Commit Bot 2019-08-21 13:27:00 PDT
Comment on attachment 376902 [details]
Patch

Clearing flags on attachment: 376902

Committed r248961: <https://trac.webkit.org/changeset/248961>
Comment 31 WebKit Commit Bot 2019-08-21 13:27:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Russell Epstein 2019-08-26 10:12:58 PDT
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>
Comment 33 Rob Buis 2019-08-26 10:21:08 PDT
(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.
Comment 34 Rob Buis 2019-08-26 12:43:47 PDT
(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
Comment 35 Russell Epstein 2019-08-26 14:29:57 PDT
(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 ""
Comment 36 Frédéric Wang (:fredw) 2019-08-29 03:34:14 PDT
(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.
Comment 37 Alexey Proskuryakov 2019-08-29 09:30:28 PDT
Yes, installing a macOS Catalina developer beta, building WebKit and running tests on it should all work.
Comment 38 Rob Buis 2019-09-05 13:57:47 PDT
(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.
Comment 39 Rob Buis 2019-10-04 01:33:14 PDT
Created attachment 380197 [details]
Patch
Comment 40 Rob Buis 2019-10-04 12:50:31 PDT
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.
Comment 41 Rob Buis 2019-10-10 02:11:48 PDT
Created attachment 380614 [details]
Patch
Comment 42 Build Bot 2019-10-10 10:05:00 PDT
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
Comment 43 Build Bot 2019-10-10 10:05:04 PDT
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
Comment 44 Rob Buis 2019-10-17 10:30:53 PDT
Created attachment 381200 [details]
Patch
Comment 45 Rob Buis 2019-10-18 03:30:48 PDT
(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?
Comment 46 Alexey Proskuryakov 2019-10-18 09:04:57 PDT
No plans to update to pre-release builds at this time.