Implement referrerpolicy attribute for img elements

Implement referrerpolicy attribute for img elements

The 'referrerpolicy' attribute controls the referrer for the outgoing
request when an image is loaded.

BUG=490608
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200940

[estark, 2015-08-18 00:17:27]

estark@chromium.org changed reviewers:
+ mkwst@chromium.org

[estark, 2015-08-18 00:17:28]

Mike, do you think you might be able to take a look at this in Jochen's absence,
or recommend another reviewer? Thanks!

[Yoav Weiss, 2015-08-18 07:22:17]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-08-18 07:22:17]

Hi estark :)

I can review that.
All in all, looks good, but there are some changes to be made around getting the
value, both in HTMLImageElement and in HTMLPreloadScanner.

Also, can you add a test that makes sure this works for `<picture><source
srcset><img></picture>` as well?

https://codereview.chromium.org/1291613010/diff/20001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1291613010/diff/20001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:771: }
Processing the referrer every time referrerPolicy() is called means that you'd
have to translate it from string each time. It's better to process it once in
parseAttribute() and on relevant mutations (if there are any).

Also, missed a newline :)

https://codereview.chromium.org/1291613010/diff/20001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1291613010/diff/20001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:605: }
I don't think we should handle the referrer attribute here. It should probably
be handled as part of processImgAttribute for this change, since we want to
enable it only on img elements, right?

[estark, 2015-08-18 17:34:26]

Thanks, Yoav! Addressed your comments and added the <picture> test.

https://codereview.chromium.org/1291613010/diff/20001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1291613010/diff/20001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:771: }
On 2015/08/18 07:22:17, Yoav Weiss wrote:
> Processing the referrer every time referrerPolicy() is called means that you'd
> have to translate it from string each time. It's better to process it once in
> parseAttribute() and on relevant mutations (if there are any).
> 
> Also, missed a newline :)

Done.

https://codereview.chromium.org/1291613010/diff/20001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1291613010/diff/20001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:605: }
On 2015/08/18 07:22:17, Yoav Weiss wrote:
> I don't think we should handle the referrer attribute here. It should probably
> be handled as part of processImgAttribute for this change, since we want to
> enable it only on img elements, right?

Hmm, I originally put it here because the spec defines it for multiple elements,
not just img (e.g. <iframe> and <a>), but after looking more closely, it seems
the <img> is the only relevant element for which we send a preload request, so I
think you're right. Done. (Though that does require adding a m_referrerPolicy
member to StartTagScanner, IIUC.)

[Yoav Weiss, 2015-08-18 21:09:21]

All in all looks good :)
A few more comments though

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/referrer-policy-attribute-img-no-referrer-expected.txt
(right):

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/referrer-policy-attribute-img-no-referrer-expected.txt:1:
layer at (0,0) size 800x600
An expected.html result would be better here than a tree dump and a PNG.
You can replace both with a single html that includes a green image in the same
location as the test html.

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/security/referrer-policy-attribute-img-picture.html
(right):

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/referrer-policy-attribute-img-picture.html:7:
referrerpolicy="no-referrer" />
Can you add another such image here where the media matches and the
`green-if-no-referrer` image is on <source>? It should Just Work™.

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/security/referrer-policy-attribute-no-policy.html
(right):

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/referrer-policy-attribute-no-policy.html:14:
img.href = "http://localhost:8000/security/resources/compass.jpg";
img.src? (Although I'm not sure it's relevant for the test)

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:279: }
We could replace the if{}else{} here similarly to my comment in
HTMLPreloadScanner

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:174: // Referrer policy parsed from the
'referrerpolicy' attribute.
Nit: Not sure this comment adds much value, but your call if you want to keep it

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:250: }
I believe we can replace the last 6 lines with
`SecurityPolicy::referrerPolicyFromString(attributeValue, &m_referrerPolicy)`
(since referrerPolicyFromString doesn't have any side effects on the input
referrerPolicy pointer in case it returns false)

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:608: 
Nit: spurious new line

https://codereview.chromium.org/1291613010/diff/40001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1291613010/diff/40001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:321: if (referrerPolicy !=
ReferrerPolicyDefault) {
nit: braces are not required here, and since the rest of the file doesn't have
braces for single lines, IMO it's better to leave them out.

[estark, 2015-08-19 02:53:22]

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/referrer-policy-attribute-img-no-referrer-expected.txt
(right):

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/referrer-policy-attribute-img-no-referrer-expected.txt:1:
layer at (0,0) size 800x600
On 2015/08/18 21:09:21, Yoav Weiss wrote:
> An expected.html result would be better here than a tree dump and a PNG.
> You can replace both with a single html that includes a green image in the
same
> location as the test html.

Cool, I didn't know about that, thanks! Done.

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/security/referrer-policy-attribute-img-picture.html
(right):

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/referrer-policy-attribute-img-picture.html:7:
referrerpolicy="no-referrer" />
On 2015/08/18 21:09:21, Yoav Weiss wrote:
> Can you add another such image here where the media matches and the
> `green-if-no-referrer` image is on <source>? It should Just Work™.

Done.

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/security/referrer-policy-attribute-no-policy.html
(right):

https://codereview.chromium.org/1291613010/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/referrer-policy-attribute-no-policy.html:14:
img.href = "http://localhost:8000/security/resources/compass.jpg";
On 2015/08/18 21:09:21, Yoav Weiss wrote:
> img.src? (Although I'm not sure it's relevant for the test)

Done.

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:279: }
On 2015/08/18 21:09:21, Yoav Weiss wrote:
> We could replace the if{}else{} here similarly to my comment in
> HTMLPreloadScanner

Done.

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:174: // Referrer policy parsed from the
'referrerpolicy' attribute.
On 2015/08/18 21:09:21, Yoav Weiss wrote:
> Nit: Not sure this comment adds much value, but your call if you want to keep
it

Done.

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:250: }
On 2015/08/18 21:09:21, Yoav Weiss wrote:
> I believe we can replace the last 6 lines with
> `SecurityPolicy::referrerPolicyFromString(attributeValue, &m_referrerPolicy)`
> (since referrerPolicyFromString doesn't have any side effects on the input
> referrerPolicy pointer in case it returns false)

Done.

https://codereview.chromium.org/1291613010/diff/40001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:608: 
On 2015/08/18 21:09:21, Yoav Weiss wrote:
> Nit: spurious new line

Done.

https://codereview.chromium.org/1291613010/diff/40001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1291613010/diff/40001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:321: if (referrerPolicy !=
ReferrerPolicyDefault) {
On 2015/08/18 21:09:21, Yoav Weiss wrote:
> nit: braces are not required here, and since the rest of the file doesn't have
> braces for single lines, IMO it's better to leave them out. 

Done.

[Yoav Weiss, 2015-08-19 06:23:04]

Last round of comments, hopefully.
I missed it earlier, but can you please also add a unit test to
HTMLPreloadScannerTest.cpp making sure that the referrerpolicy changes are taken
into effect and are working properly? (even when two attributes with different
values are present)

https://codereview.chromium.org/1291613010/diff/60001/LayoutTests/http/tests/...
File LayoutTests/http/tests/security/referrer-policy-attribute-img-picture.html
(right):

https://codereview.chromium.org/1291613010/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/referrer-policy-attribute-img-picture.html:4:
<picture>
Nit: indentation is a bit messed up

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.idl (right):

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.idl:42: 
Nit: Extra new line

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:244: } else if
(match(attributeName, referrerpolicyAttr) && !attributeValue.isNull()) {
Sorry for missing it earlier, but we need to protect against two referrerpolicy
attributes on the same tag. You can do that by adding an m_referrerpolicySet
member var and checking for it here.

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:245: m_referrerPolicy =
ReferrerPolicyDefault;
Is there a scenario where initialization at the constructor is not enough?

[estark, 2015-08-19 16:17:01]

https://codereview.chromium.org/1291613010/diff/60001/LayoutTests/http/tests/...
File LayoutTests/http/tests/security/referrer-policy-attribute-img-picture.html
(right):

https://codereview.chromium.org/1291613010/diff/60001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/referrer-policy-attribute-img-picture.html:4:
<picture>
On 2015/08/19 06:23:04, Yoav Weiss wrote:
> Nit: indentation is a bit messed up

Done.

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.idl (right):

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.idl:42: 
On 2015/08/19 06:23:04, Yoav Weiss wrote:
> Nit: Extra new line

Done.

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:244: } else if
(match(attributeName, referrerpolicyAttr) && !attributeValue.isNull()) {
On 2015/08/19 06:23:04, Yoav Weiss wrote:
> Sorry for missing it earlier, but we need to protect against two
referrerpolicy
> attributes on the same tag. You can do that by adding an m_referrerpolicySet
> member var and checking for it here.

Done.

https://codereview.chromium.org/1291613010/diff/60001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:245: m_referrerPolicy =
ReferrerPolicyDefault;
On 2015/08/19 06:23:04, Yoav Weiss wrote:
> Is there a scenario where initialization at the constructor is not enough?

No, removed.

[estark, 2015-08-19 16:17:21]

On 2015/08/19 06:23:04, Yoav Weiss wrote:
> Last round of comments, hopefully.
> I missed it earlier, but can you please also add a unit test to
> HTMLPreloadScannerTest.cpp making sure that the referrerpolicy changes are
taken
> into effect and are working properly? (even when two attributes with different
> values are present)

Done.

[Yoav Weiss, 2015-08-19 16:26:21]

On 2015/08/19 16:17:21, estark wrote:
> On 2015/08/19 06:23:04, Yoav Weiss wrote:
> > Last round of comments, hopefully.
> > I missed it earlier, but can you please also add a unit test to
> > HTMLPreloadScannerTest.cpp making sure that the referrerpolicy changes are
> taken
> > into effect and are working properly? (even when two attributes with
different
> > values are present)
> 
> Done.

LGTM! :)

One last nit: comments don't have to be split to a new line at 80 chars in
Blink, so it might be better to split them differently if you feel they're
getting too long for a single line.

[estark, 2015-08-19 17:00:29]

Thanks, Yoav!

Mike, do you want to take a look before I land this?

On 2015/08/19 16:26:21, Yoav Weiss wrote:
> On 2015/08/19 16:17:21, estark wrote:
> > On 2015/08/19 06:23:04, Yoav Weiss wrote:
> > > Last round of comments, hopefully.
> > > I missed it earlier, but can you please also add a unit test to
> > > HTMLPreloadScannerTest.cpp making sure that the referrerpolicy changes are
> > taken
> > > into effect and are working properly? (even when two attributes with
> different
> > > values are present)
> > 
> > Done.
> 
> LGTM! :)
> 
> One last nit: comments don't have to be split to a new line at 80 chars in
> Blink, so it might be better to split them differently if you feel they're
> getting too long for a single line.

I unwrapped the ones I added in this CL since they aren't very long. How do you
usually decided when to wrap them? Just split if it starts to get ridiculously
long?

[Yoav Weiss, 2015-08-19 20:19:38]

On 2015/08/19 17:00:29, estark wrote:

> > One last nit: comments don't have to be split to a new line at 80 chars in
> > Blink, so it might be better to split them differently if you feel they're
> > getting too long for a single line.
> 
> I unwrapped the ones I added in this CL since they aren't very long. How do
you
> usually decided when to wrap them? Just split if it starts to get ridiculously
> long?

When they get too long, I just break them on the next comma or so.

[estark, 2015-08-20 17:33:02]

Mike, I'm going to take your silence as a lack of objections. :) Going to go
ahead and CQ this, but please feel free to uncheck if you have any concerns.

[estark, 2015-08-20 17:33:13]

The CQ bit was checked by estark@chromium.org

[estark, 2015-08-20 17:33:13]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1291613010/#ps100001
(title: "unwrap comments")

[commit-bot: I haz the power, 2015-08-20 17:33:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291613010/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291613010/100001

[commit-bot: I haz the power, 2015-08-20 18:44:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-20 18:44:54]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2015-08-20 18:51:49]

The CQ bit was checked by estark@chromium.org

[estark, 2015-08-20 18:51:49]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1291613010/#ps120001
(title: "update webexposed tests")

[commit-bot: I haz the power, 2015-08-20 18:52:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291613010/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291613010/120001

[commit-bot: I haz the power, 2015-08-20 19:59:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-20 19:59:58]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yoav Weiss, 2015-08-20 20:09:17]

On 2015/08/20 19:59:58, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Hmm, global-interface-listing.html seems to be consistently crashing here :(
I'm not sure how that could be related, but if that's not happening on the tree
without the patch, it's worth looking into that.

[Yoav Weiss, 2015-08-20 20:11:52]

On 2015/08/20 20:09:17, Yoav Weiss wrote:
> On 2015/08/20 19:59:58, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Hmm, global-interface-listing.html seems to be consistently crashing here :(
> I'm not sure how that could be related, but if that's not happening on the
tree
> without the patch, it's worth looking into that.

If it is related to the patch, HTMLImageElement.idl is probably the immediate
suspect

[estark, 2015-08-20 22:00:51]

The CQ bit was checked by estark@chromium.org

[estark, 2015-08-20 22:00:51]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1291613010/#ps160001
(title: "actually fix webexposed test this time")

[commit-bot: I haz the power, 2015-08-20 22:01:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291613010/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291613010/160001

[commit-bot: I haz the power, 2015-08-21 00:23:10]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200940

[Yoav Weiss, 2015-08-21 10:57:39]

On 2015/08/21 00:23:10, commit-bot: I haz the power wrote:
> Committed patchset #9 (id:160001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=200940

A huge oversight on my part, but this change should have been added behind a
runtime flag :/
(As there's an intent to implement, but no intent to ship).

estark - Can you please add a runtime flag that prevents this from running by
default?

[estark, 2015-08-21 17:16:57]

On 2015/08/21 10:57:39, Yoav Weiss wrote:
> On 2015/08/21 00:23:10, commit-bot: I haz the power wrote:
> > Committed patchset #9 (id:160001) as
> > https://src.chromium.org/viewvc/blink?view=rev&revision=200940
> 
> A huge oversight on my part, but this change should have been added behind a
> runtime flag :/
> (As there's an intent to implement, but no intent to ship).
> 
> estark - Can you please add a runtime flag that prevents this from running by
> default?

Oh no, my bad! I already had the runtime flag for anchor elements and used it in
my original patch for this CL but accidentally dropped it somewhere along the
way. Please see https://codereview.chromium.org/1302403005/