Bug 179053 - Consider supporting the `referrerpolicy` attribute.
Summary: Consider supporting the `referrerpolicy` attribute.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks: 185550
  Show dependency treegraph
 
Reported: 2017-10-31 07:08 PDT by Mike West
Modified: 2019-03-21 10:59 PDT (History)
17 users (show)

See Also:


Attachments
Patch (30.42 KB, patch)
2019-02-10 07:14 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.53 MB, application/zip)
2019-02-10 09:21 PST, Build Bot
no flags Details
Archive of layout-test-results from ews206 for win-future (12.87 MB, application/zip)
2019-02-10 09:38 PST, Build Bot
no flags Details
Patch (78.00 KB, patch)
2019-02-10 10:11 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.60 MB, application/zip)
2019-02-10 12:20 PST, Build Bot
no flags Details
Patch (79.59 KB, patch)
2019-02-10 12:51 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (81.26 KB, patch)
2019-02-13 05:24 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (85.25 KB, patch)
2019-02-13 08:04 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (85.22 KB, patch)
2019-02-13 10:53 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (85.20 KB, patch)
2019-02-15 02:52 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.57 MB, application/zip)
2019-02-15 04:03 PST, Build Bot
no flags Details
Patch (89.75 KB, patch)
2019-02-15 10:19 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.53 MB, application/zip)
2019-02-15 11:31 PST, Build Bot
no flags Details
Patch (90.80 KB, patch)
2019-02-18 00:14 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (90.94 KB, patch)
2019-02-19 08:07 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (89.88 KB, patch)
2019-03-01 03:06 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (117.63 KB, patch)
2019-03-01 08:06 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (118.27 KB, patch)
2019-03-01 09:47 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.92 MB, application/zip)
2019-03-01 12:17 PST, Build Bot
no flags Details
Patch (119.65 KB, patch)
2019-03-01 13:52 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (119.63 KB, patch)
2019-03-03 08:26 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (119.70 KB, patch)
2019-03-05 01:28 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.96 MB, application/zip)
2019-03-05 03:45 PST, Build Bot
no flags Details
Patch (119.70 KB, patch)
2019-03-05 07:08 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (10.25 MB, application/zip)
2019-03-05 09:30 PST, Build Bot
no flags Details
Patch (119.96 KB, patch)
2019-03-05 09:44 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.58 MB, application/zip)
2019-03-05 11:10 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.56 MB, application/zip)
2019-03-05 11:57 PST, Build Bot
no flags Details
Patch (119.96 KB, patch)
2019-03-05 12:01 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (143.98 KB, patch)
2019-03-06 00:56 PST, 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 Mike West 2017-10-31 07:08:08 PDT
It doesn't look like Safari TP supports the `referrerpolicy` attribute, which defines a referrer policy for a given element's request: https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery-referrer-attribute

See http://w3c-test.org/referrer-policy/no-referrer-when-downgrade/attr-referrer/cross-origin/http-http/img-tag/insecure-protocol.swap-origin-redirect.http.html for example,  which Chrome and Firefox both pass. There are _lots_ of these tests that would go green with support for the attribute. :)
Comment 1 Radar WebKit Bug Importer 2017-10-31 10:48:09 PDT
<rdar://problem/35275307>
Comment 2 Rob Buis 2019-02-10 07:14:45 PST Comment hidden (obsolete)
Comment 3 Build Bot 2019-02-10 09:21:51 PST Comment hidden (obsolete)
Comment 4 Build Bot 2019-02-10 09:21:53 PST Comment hidden (obsolete)
Comment 5 Build Bot 2019-02-10 09:38:40 PST Comment hidden (obsolete)
Comment 6 Build Bot 2019-02-10 09:38:51 PST Comment hidden (obsolete)
Comment 7 Rob Buis 2019-02-10 10:11:41 PST Comment hidden (obsolete)
Comment 8 Brent Fulgham 2019-02-10 10:50:39 PST
Before landing we need to make sure this doesn’t bypass any ITO protections.
Comment 9 Build Bot 2019-02-10 12:20:12 PST Comment hidden (obsolete)
Comment 10 Build Bot 2019-02-10 12:20:14 PST Comment hidden (obsolete)
Comment 11 Rob Buis 2019-02-10 12:51:58 PST Comment hidden (obsolete)
Comment 12 Rob Buis 2019-02-10 12:54:49 PST
(In reply to Brent Fulgham from comment #8)
> Before landing we need to make sure this doesn’t bypass any ITO protections.

Did you mean ITP? And if so were you thinking about NetworkDataTaskCocoa::restrictRequestReferrerToOriginIfNeeded? AFAIK that is unrelated, but I have not looked at that code in detail.
Comment 13 Ryosuke Niwa 2019-02-12 20:25:27 PST
Please hide this behind a runtime flag.
Comment 14 Rob Buis 2019-02-13 05:24:38 PST Comment hidden (obsolete)
Comment 15 Rob Buis 2019-02-13 08:04:41 PST Comment hidden (obsolete)
Comment 16 Rob Buis 2019-02-13 10:53:54 PST Comment hidden (obsolete)
Comment 17 Rob Buis 2019-02-13 13:46:51 PST
(In reply to Ryosuke Niwa from comment #13)
> Please hide this behind a runtime flag.

Done.
Comment 18 Rob Buis 2019-02-15 02:52:28 PST Comment hidden (obsolete)
Comment 19 Build Bot 2019-02-15 04:03:26 PST Comment hidden (obsolete)
Comment 20 Build Bot 2019-02-15 04:03:28 PST Comment hidden (obsolete)
Comment 21 Rob Buis 2019-02-15 10:19:36 PST Comment hidden (obsolete)
Comment 22 Build Bot 2019-02-15 11:31:17 PST Comment hidden (obsolete)
Comment 23 Build Bot 2019-02-15 11:31:19 PST Comment hidden (obsolete)
Comment 24 Rob Buis 2019-02-18 00:14:06 PST Comment hidden (obsolete)
Comment 25 Frédéric Wang (:fredw) 2019-02-18 08:16:43 PST
Comment on attachment 362265 [details]
Patch

This lgtm but probably someone who is more familiar with that should review. Maybe you want to send an email to webkit-dev to announce it and get more visibility.
Comment 26 Rob Buis 2019-02-19 08:07:50 PST
Created attachment 362386 [details]
Patch
Comment 27 Darin Adler 2019-02-28 16:41:15 PST
Comment on attachment 362386 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362386&action=review

> Source/WebCore/html/HTMLIFrameElement.cpp:103
> +        m_referrerPolicy = ReferrerPolicy::EmptyString;
> +        if (auto policy = parseReferrerPolicy(value, ReferrerPolicySource::HTTPHeader))
> +            m_referrerPolicy = *policy;

I think value_or would be a more natural way to write this than an if statement.

Also seems a little unintuitive that it is correct to pass HTTPHeader when this is coming from an attribute, not an HTTP header field.

> Source/WebCore/html/HTMLIFrameElement.cpp:125
> +    if (m_referrerPolicy != ReferrerPolicy::EmptyString)

I would have reversed this if statement.

> Source/WebCore/html/HTMLIFrameElement.cpp:126
> +        return attributeWithoutSynchronization(referrerpolicyAttr).convertToASCIILowercase();

This is a really peculiar getter style; I don’t think I’ve seen it before in DOM. More typically either the getter is entirely a reflection of the attribute (in which case we use [Reflect]) or the getter is one of a set of hard-coded specific strings, so this function would be a switch statement and not call attributeWithoutSynchronization at all. Maybe this is just an alternate implementation of the switch statement?

No stripping of leading or trailing spaces needed? Maybe if there are spaces then the entire referrer policy is ignored and we just have EmptyString. Do we have test cases covering that part of the behavior?

> Source/WebCore/html/HTMLIFrameElement.cpp:127
> +    return String();

I kinda like "{ }" better than "String()"; not sure if others agree.

> Source/WebCore/html/HTMLIFrameElement.h:45
> +    ReferrerPolicy referrerPolicy() const override { return m_referrerPolicy; }

I think our style would be to use final here rather than override (or both).
Comment 28 youenn fablet 2019-02-28 17:46:33 PST
Comment on attachment 362386 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362386&action=review

> Source/WebCore/ChangeLog:10
> +        if valid it is used for the subframe load.

Is it supposed to have an impact on only the load of the iframe main resource or also to all sub resources of the iframe?

> Source/WebCore/ChangeLog:13
> +        Test: web-platform-tests/html/dom/reflection-embedded.html

It seems the reflection-embedded.html test is mostly checking setter and getter of the attribute.
Do we have tests to ensure the right referrer policy is used for the iframe main resource load?

> LayoutTests/imported/w3c/web-platform-tests/html/dom/reflection-embedded.html:1
> +<!DOCTYPE html>

Unnecessary change?
Comment 29 Rob Buis 2019-03-01 03:06:25 PST
Created attachment 363320 [details]
Patch
Comment 30 Rob Buis 2019-03-01 08:06:40 PST
Created attachment 363331 [details]
Patch
Comment 31 Rob Buis 2019-03-01 09:47:33 PST
Created attachment 363338 [details]
Patch
Comment 32 Build Bot 2019-03-01 12:17:40 PST
Comment on attachment 363338 [details]
Patch

Attachment 363338 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11333352

New failing tests:
http/tests/referrer-policy-iframe/strict-origin/same-origin.html
http/tests/referrer-policy-iframe/strict-origin-when-cross-origin/cross-origin-http-http.html
http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http-http.html
http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http.https.html
http/tests/referrer-policy-iframe/strict-origin/cross-origin-http-http.html
http/tests/referrer-policy-iframe/same-origin/cross-origin-http-http.html
Comment 33 Build Bot 2019-03-01 12:17:52 PST
Created attachment 363353 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 34 youenn fablet 2019-03-01 12:27:10 PST
Comment on attachment 363338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363338&action=review

> LayoutTests/http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http-http.html:15
> +    finishJSTest();

These seems like great tests. I wish we could contribute them to WPT.

To testharnessify these tests, you might be able to do the following:
- Move them to http/wpt.
- Replace port 8000 by port 8800
- Include testharness.js/testharnessreport.js instead of js-test resources.
- Replace shouldBeEqualToString by assert_equals
- Replace finishJSTest by done.
- Maybe add a title tag to have a good -expected.txt

To upstream them to WPT, you will need to make 127.0.0.1/localhost agnostic.
You can use /common/get-host-info.sub.js for that purpose.
Comment 35 Rob Buis 2019-03-01 13:11:09 PST
Comment on attachment 362386 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362386&action=review

>> Source/WebCore/html/HTMLIFrameElement.cpp:103
>> +            m_referrerPolicy = *policy;
> 
> I think value_or would be a more natural way to write this than an if statement.
> 
> Also seems a little unintuitive that it is correct to pass HTTPHeader when this is coming from an attribute, not an HTTP header field.

Using value_or now. And besides unintuitive, it was wrong of me to use HTTPHeader mode! Unlike the HTTP header, the attribute does not use the list notation but a single referrer policy value.

>> Source/WebCore/html/HTMLIFrameElement.cpp:125
>> +    if (m_referrerPolicy != ReferrerPolicy::EmptyString)
> 
> I would have reversed this if statement.

Done.

>> Source/WebCore/html/HTMLIFrameElement.cpp:126
>> +        return attributeWithoutSynchronization(referrerpolicyAttr).convertToASCIILowercase();
> 
> This is a really peculiar getter style; I don’t think I’ve seen it before in DOM. More typically either the getter is entirely a reflection of the attribute (in which case we use [Reflect]) or the getter is one of a set of hard-coded specific strings, so this function would be a switch statement and not call attributeWithoutSynchronization at all. Maybe this is just an alternate implementation of the switch statement?
> 
> No stripping of leading or trailing spaces needed? Maybe if there are spaces then the entire referrer policy is ignored and we just have EmptyString. Do we have test cases covering that part of the behavior?

I think I have seen that style, but I reverted to the switch now.
Surprisingly I see no coverage for the leading or trailing spaces. reflection-embedded.htm does not test it. I verified it works by using the Inspector, but maybe I need to add such tests to reflection-embedded.htm...

>> Source/WebCore/html/HTMLIFrameElement.cpp:127
>> +    return String();
> 
> I kinda like "{ }" better than "String()"; not sure if others agree.

Done.

>> Source/WebCore/html/HTMLIFrameElement.h:45
>> +    ReferrerPolicy referrerPolicy() const override { return m_referrerPolicy; }
> 
> I think our style would be to use final here rather than override (or both).

Done.
Comment 36 Rob Buis 2019-03-01 13:25:41 PST
Comment on attachment 363338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363338&action=review

>> LayoutTests/http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http-http.html:15
>> +    finishJSTest();
> 
> These seems like great tests. I wish we could contribute them to WPT.
> 
> To testharnessify these tests, you might be able to do the following:
> - Move them to http/wpt.
> - Replace port 8000 by port 8800
> - Include testharness.js/testharnessreport.js instead of js-test resources.
> - Replace shouldBeEqualToString by assert_equals
> - Replace finishJSTest by done.
> - Maybe add a title tag to have a good -expected.txt
> 
> To upstream them to WPT, you will need to make 127.0.0.1/localhost agnostic.
> You can use /common/get-host-info.sub.js for that purpose.

These are based on http/tests/referrer-policy. I wonder if both hierarchies need testing for more possible values (no-referrer, unsafe etc.) though?

About upstreaming these, WPT has similar/more extensive tests like:
http://w3c-test.org/referrer-policy/no-referrer/attr-referrer/same-origin/http-http/iframe-tag/swap-origin-redirect/generic.http.html
When loading those tests directly we PASS, however when importing them lots will FAIL/timeout, with a lot of Blocking access warnings, I am guessing that www1.localhost:8000 is not accepted. I think I have seen similar warnings in other WPT tests, not sure if it is a setup issue or the tests can do some more to prevent it.
So, not sure what it the best approach here. Also the patch is getting big, time to split up tests and code?
Comment 37 Rob Buis 2019-03-01 13:52:01 PST
Created attachment 363369 [details]
Patch
Comment 38 Rob Buis 2019-03-01 13:53:26 PST
I think more needs to be done for this patch, especially regarding tests, but reviews are welcome. And thanks for the reviews so far!
Comment 39 Darin Adler 2019-03-01 20:18:10 PST
Comment on attachment 363369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363369&action=review

> Source/WebCore/html/HTMLIFrameElement.cpp:123
> +        return "no-referrer";

Should use the _s suffix so these get the ASCIILiteral optimization.

> Source/WebCore/html/HTMLIFrameElement.cpp:139
> +    default:
> +        return { };

We want a warning if we forget to include a case. The way to get that with some compilers is to omit "default" and put the return outside the switch statement. Please do that.

> Source/WebCore/loader/SubframeLoader.cpp:323
> +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())
> +        policy = ownerElement.referrerPolicy();

When Ryosuke said this should be a runtime-enabled feature, did he mean that respecting the policy should be runtime-enabled, but the DOM manifestation of the policy attribute could be included unconditionally? I’m not sure I know the state of the art for runtime-enabled DOM features.
Comment 40 Rob Buis 2019-03-03 08:26:06 PST
Created attachment 363456 [details]
Patch
Comment 41 Rob Buis 2019-03-03 11:44:48 PST
Comment on attachment 363369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363369&action=review

>> Source/WebCore/html/HTMLIFrameElement.cpp:123
>> +        return "no-referrer";
> 
> Should use the _s suffix so these get the ASCIILiteral optimization.

Done.

>> Source/WebCore/html/HTMLIFrameElement.cpp:139
>> +        return { };
> 
> We want a warning if we forget to include a case. The way to get that with some compilers is to omit "default" and put the return outside the switch statement. Please do that.

I knew I had forgotten something about the switch :) Done.

>> Source/WebCore/loader/SubframeLoader.cpp:323
>> +        policy = ownerElement.referrerPolicy();
> 
> When Ryosuke said this should be a runtime-enabled feature, did he mean that respecting the policy should be runtime-enabled, but the DOM manifestation of the policy attribute could be included unconditionally? I’m not sure I know the state of the art for runtime-enabled DOM features.

I am not sure either, I saw at least EncryptedMedia flag works like this. Happy to change it if needed.
Comment 42 Darin Adler 2019-03-03 12:04:33 PST
Comment on attachment 363456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363456&action=review

Looks fine to me; worried about that question about exposing the attribute when the feature is turned off.

> Source/WebCore/html/HTMLIFrameElement.cpp:141
> +    return { };

Could put an ASSERT_NOT_REACHED() here too. Very unimportant since it’s not really a practical possibility at all.
Comment 43 Rob Buis 2019-03-04 08:23:20 PST
Comment on attachment 362386 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362386&action=review

>> Source/WebCore/ChangeLog:10
>> +        if valid it is used for the subframe load.
> 
> Is it supposed to have an impact on only the load of the iframe main resource or also to all sub resources of the iframe?

As far as I know only the load of the iframe main resource.

>> Source/WebCore/ChangeLog:13
>> +        Test: web-platform-tests/html/dom/reflection-embedded.html
> 
> It seems the reflection-embedded.html test is mostly checking setter and getter of the attribute.
> Do we have tests to ensure the right referrer policy is used for the iframe main resource load?

I copied existing http/tests/referrer-policy tests for iframe referrerpolicy attribute, but I think we also may want to test the remaining values:
https://bugs.webkit.org/show_bug.cgi?id=195269.
As mentioned there are WPT tests as well but due to localhost/127.0.0.1 issues we can't run them.

>> LayoutTests/imported/w3c/web-platform-tests/html/dom/reflection-embedded.html:1
>> +<!DOCTYPE html>
> 
> Unnecessary change?

Fixed.
Comment 44 Rob Buis 2019-03-04 08:29:28 PST
Comment on attachment 363338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363338&action=review

>>> LayoutTests/http/tests/referrer-policy-iframe/origin-when-cross-origin/cross-origin-http-http.html:15
>>> +    finishJSTest();
>> 
>> These seems like great tests. I wish we could contribute them to WPT.
>> 
>> To testharnessify these tests, you might be able to do the following:
>> - Move them to http/wpt.
>> - Replace port 8000 by port 8800
>> - Include testharness.js/testharnessreport.js instead of js-test resources.
>> - Replace shouldBeEqualToString by assert_equals
>> - Replace finishJSTest by done.
>> - Maybe add a title tag to have a good -expected.txt
>> 
>> To upstream them to WPT, you will need to make 127.0.0.1/localhost agnostic.
>> You can use /common/get-host-info.sub.js for that purpose.
> 
> These are based on http/tests/referrer-policy. I wonder if both hierarchies need testing for more possible values (no-referrer, unsafe etc.) though?
> 
> About upstreaming these, WPT has similar/more extensive tests like:
> http://w3c-test.org/referrer-policy/no-referrer/attr-referrer/same-origin/http-http/iframe-tag/swap-origin-redirect/generic.http.html
> When loading those tests directly we PASS, however when importing them lots will FAIL/timeout, with a lot of Blocking access warnings, I am guessing that www1.localhost:8000 is not accepted. I think I have seen similar warnings in other WPT tests, not sure if it is a setup issue or the tests can do some more to prevent it.
> So, not sure what it the best approach here. Also the patch is getting big, time to split up tests and code?

For the record, I think http/tests/referrer-policy are very easy to read, much easier than wpt/referrer-policy ones, but the latter are more exhaustive. I think we can go with the (expanded) http/tests/referrer-policy tests for now.

When I looked again at importing wpt/referrer-policy tests, I noticed some more could be made to pass by using get-host-info.sub.js, but I think not all of them could be made to pass. I assume there are more wpt tests that have the same 127.0.0.1/localhost confusion. Is there a bug for this problem?
Comment 45 youenn fablet 2019-03-04 22:29:42 PST


> When I looked again at importing wpt/referrer-policy tests, I noticed some
> more could be made to pass by using get-host-info.sub.js, but I think not
> all of them could be made to pass. I assume there are more wpt tests that
> have the same 127.0.0.1/localhost confusion. Is there a bug for this problem?

WPT upstream would like WebKit to support things like www1.localhost but so far we have not been able to find a simple and proper solution that would be easy for both developers and bots.

I am surprised we could not use everywhere get-host-info.sub.js for these tests, though it is true it might be less convenient since you will need to implement in JS the templating done by the WPT server.

> Source/WebCore/loader/SubframeLoader.cpp:322
> +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())

You could remove that if check since it should be EmptyString when not exposing the attribute.
You could add an ASSERT I guess.

> Looks fine to me; worried about that question about exposing the attribute
> when the feature is turned off.

This patch makes it so the attribute is not exposed when the feature is turned off. This allows feature detection and ensures that there is no change of functionality in case it is off.
Comment 46 Rob Buis 2019-03-05 01:28:57 PST
Created attachment 363620 [details]
Patch
Comment 47 Build Bot 2019-03-05 03:45:52 PST
Comment on attachment 363620 [details]
Patch

Attachment 363620 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11375818

New failing tests:
fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html
Comment 48 Build Bot 2019-03-05 03:45:55 PST
Created attachment 363634 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 49 Rob Buis 2019-03-05 07:08:34 PST
Created attachment 363640 [details]
Patch
Comment 50 Build Bot 2019-03-05 09:30:32 PST
Comment on attachment 363640 [details]
Patch

Attachment 363640 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11378941

New failing tests:
fast/events/beforeunload-alert-user-interaction2.html
fast/scrolling/ios/mixing-user-and-programmatic-scroll-006.html
Comment 51 Build Bot 2019-03-05 09:30:34 PST
Created attachment 363644 [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.13.6
Comment 52 Rob Buis 2019-03-05 09:44:28 PST
Created attachment 363647 [details]
Patch
Comment 53 Build Bot 2019-03-05 11:10:11 PST
Comment on attachment 363647 [details]
Patch

Attachment 363647 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11380629

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html
Comment 54 Build Bot 2019-03-05 11:10:14 PST
Created attachment 363657 [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
Comment 55 Build Bot 2019-03-05 11:57:17 PST
Comment on attachment 363647 [details]
Patch

Attachment 363647 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11380816

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html
Comment 56 Build Bot 2019-03-05 11:57:20 PST
Created attachment 363665 [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.13.6
Comment 57 Rob Buis 2019-03-05 12:01:16 PST
Created attachment 363667 [details]
Patch
Comment 58 Rob Buis 2019-03-05 13:09:26 PST
(In reply to youenn fablet from comment #45)
> 
> 
> > When I looked again at importing wpt/referrer-policy tests, I noticed some
> > more could be made to pass by using get-host-info.sub.js, but I think not
> > all of them could be made to pass. I assume there are more wpt tests that
> > have the same 127.0.0.1/localhost confusion. Is there a bug for this problem?
> 
> WPT upstream would like WebKit to support things like www1.localhost but so
> far we have not been able to find a simple and proper solution that would be
> easy for both developers and bots.
> 
> I am surprised we could not use everywhere get-host-info.sub.js for these
> tests, though it is true it might be less convenient since you will need to
> implement in JS the templating done by the WPT server.

I tried using get-host-info.sub.js before and the no-redirect tests were fine, but not the redirect ones.
I did not investigate further but was suspecting get_swapped_origin_netloc:
https://github.com/web-platform-tests/wpt/blob/master/referrer-policy/generic/subresource/subresource.py

> > Source/WebCore/loader/SubframeLoader.cpp:322
> > +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())
> 
> You could remove that if check since it should be EmptyString when not
> exposing the attribute.
> You could add an ASSERT I guess.

I am not sure if that was your suggestion, but I moved the flag check check to IFrameElement::referrerPolicy, which gives a bit cleaner code.
Comment 59 youenn fablet 2019-03-05 13:22:46 PST
> I tried using get-host-info.sub.js before and the no-redirect tests were
> fine, but not the redirect ones.
> I did not investigate further but was suspecting get_swapped_origin_netloc:
> https://github.com/web-platform-tests/wpt/blob/master/referrer-policy/
> generic/subresource/subresource.py

Indeed, this would need to be updated.
Maybe any request triggering subresource.py should have as query parameter the actual cross-origin domain to use.

This makes subresource.py more generic and the JS test is solely in control of which cross-origin domain to use, through get-host-info.sub.js.

> > > Source/WebCore/loader/SubframeLoader.cpp:322
> > > +    if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled())
> > 
> > You could remove that if check since it should be EmptyString when not
> > exposing the attribute.
> > You could add an ASSERT I guess.
> 
> I am not sure if that was your suggestion, but I moved the flag check check
> to IFrameElement::referrerPolicy, which gives a bit cleaner code.

Sounds good.
Comment 60 Rob Buis 2019-03-05 14:10:20 PST
(In reply to youenn fablet from comment #59)
> > I tried using get-host-info.sub.js before and the no-redirect tests were
> > fine, but not the redirect ones.
> > I did not investigate further but was suspecting get_swapped_origin_netloc:
> > https://github.com/web-platform-tests/wpt/blob/master/referrer-policy/
> > generic/subresource/subresource.py
> 
> Indeed, this would need to be updated.
> Maybe any request triggering subresource.py should have as query parameter
> the actual cross-origin domain to use.
> 
> This makes subresource.py more generic and the JS test is solely in control
> of which cross-origin domain to use, through get-host-info.sub.js.

Ok, should I try that as part of this bug or should I just land what we have?
Comment 61 youenn fablet 2019-03-05 14:38:00 PST
(In reply to Rob Buis from comment #60)
> Ok, should I try that as part of this bug or should I just land what we have?

I think we should land as is.
I will file a GitHub WPT issue on www.localhost to see how we should fix it upstream.
Based on that, we should work, probably upstream, to update these referrer policy tests so that they can be run in WebKit CI.
Comment 62 Rob Buis 2019-03-06 00:56:14 PST
Created attachment 363735 [details]
Patch
Comment 63 Rob Buis 2019-03-06 03:00:43 PST
(In reply to youenn fablet from comment #61)
> (In reply to Rob Buis from comment #60)
> > Ok, should I try that as part of this bug or should I just land what we have?
> 
> I think we should land as is.
> I will file a GitHub WPT issue on www.localhost to see how we should fix it
> upstream.
> Based on that, we should work, probably upstream, to update these referrer
> policy tests so that they can be run in WebKit CI.

Thanks, sounds good. I will land now that I added the remaining referrer-policy-iframe tests.
Comment 64 WebKit Commit Bot 2019-03-06 03:28:44 PST
Comment on attachment 363735 [details]
Patch

Clearing flags on attachment: 363735

Committed r242534: <https://trac.webkit.org/changeset/242534>
Comment 65 WebKit Commit Bot 2019-03-06 03:28:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 66 youenn fablet 2019-03-21 10:59:59 PDT
(In reply to Rob Buis from comment #63)
> (In reply to youenn fablet from comment #61)
> > (In reply to Rob Buis from comment #60)
> > > Ok, should I try that as part of this bug or should I just land what we have?
> > 
> > I think we should land as is.
> > I will file a GitHub WPT issue on www.localhost to see how we should fix it
> > upstream.
> > Based on that, we should work, probably upstream, to update these referrer
> > policy tests so that they can be run in WebKit CI.
> 
> Thanks, sounds good. I will land now that I added the remaining
> referrer-policy-iframe tests.

Filed https://github.com/web-platform-tests/wpt/issues/15692.
The idea would be to use hosts[alt] wherever simple cross origin loads are required and use domains[www] for the case where hierarchical URLS do matter.