Created attachment 361629[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 361630[details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 361636[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
(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.
Created attachment 362109[details]
Archive of layout-test-results from ews103 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 362137[details]
Archive of layout-test-results from ews102 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
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 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 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?
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 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 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 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 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 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 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 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 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?
> 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.
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
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
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
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
(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.
> 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.
(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?
(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.
(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.
(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.
2019-02-10 07:14 PST, Rob Buis
2019-02-10 09:21 PST, EWS Watchlist
2019-02-10 09:38 PST, EWS Watchlist
2019-02-10 10:11 PST, Rob Buis
2019-02-10 12:20 PST, EWS Watchlist
2019-02-10 12:51 PST, Rob Buis
2019-02-13 05:24 PST, Rob Buis
2019-02-13 08:04 PST, Rob Buis
2019-02-13 10:53 PST, Rob Buis
2019-02-15 02:52 PST, Rob Buis
2019-02-15 04:03 PST, EWS Watchlist
2019-02-15 10:19 PST, Rob Buis
2019-02-15 11:31 PST, EWS Watchlist
2019-02-18 00:14 PST, Rob Buis
2019-02-19 08:07 PST, Rob Buis
2019-03-01 03:06 PST, Rob Buis
2019-03-01 08:06 PST, Rob Buis
2019-03-01 09:47 PST, Rob Buis
2019-03-01 12:17 PST, EWS Watchlist
2019-03-01 13:52 PST, Rob Buis
2019-03-03 08:26 PST, Rob Buis
2019-03-05 01:28 PST, Rob Buis
2019-03-05 03:45 PST, EWS Watchlist
2019-03-05 07:08 PST, Rob Buis
2019-03-05 09:30 PST, EWS Watchlist
2019-03-05 09:44 PST, Rob Buis
2019-03-05 11:10 PST, EWS Watchlist
2019-03-05 11:57 PST, EWS Watchlist
2019-03-05 12:01 PST, Rob Buis
2019-03-06 00:56 PST, Rob Buis