1264165 - Implement <link> referrerpolicy attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Thomas Nguyen (:tnguyen)]

Attachment: Implement link referrerpolicy attribute

Comment 2

[Thomas Nguyen (:tnguyen)]

Attachment: Update and add test

Comment 3

[Thomas Nguyen (:tnguyen)]

Hi Franziskus,
Could you please take a look and let me know if I am in incorrect way of missing anything?
Thanks

Comment 4

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8755815 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8755815 [details] [diff] [review]:
-----------------------------------------------------------------

I think most of this looks pretty good already. See comments inline.
Before giving this into review we should probably clarify the reload case.

::: dom/base/nsStyleLinkElement.cpp
@@ +434,5 @@
>        MOZ_LOG(SRILogHelper::GetSriLog(), mozilla::LogLevel::Debug,
>                ("nsStyleLinkElement::DoUpdateStyleSheet, integrity=%s",
>                 NS_ConvertUTF16toUTF8(integrity).get()));
>      }
> +    // if referrer attributes are enabled in preferences, load link's referrer

the link's referrer ...

@@ +436,5 @@
>                 NS_ConvertUTF16toUTF8(integrity).get()));
>      }
> +    // if referrer attributes are enabled in preferences, load link's referrer
> +    // attribute. If the link does not provide a referrer attribute, ignore this
> +    // and use document's referrer policy

the document's referrer

@@ +439,5 @@
> +    // attribute. If the link does not provide a referrer attribute, ignore this
> +    // and use document's referrer policy
> +
> +    net::ReferrerPolicy referrerPolicy = doc->GetReferrerPolicy();
> +    net::ReferrerPolicy linkReferrerPolicy = net::RP_Unset;

you don't really need linkReferrerPolicy here. simply use referrerPolicy and check that for RP_Unset.

::: dom/webidl/HTMLLinkElement.webidl
@@ +29,5 @@
>             attribute DOMString hreflang;
>    [SetterThrows, Pure]
>             attribute DOMString type;
> +  [SetterThrows, Pure, Pref="network.http.enablePerElementReferrer"]
> +    attribute DOMString referrerPolicy;

indentation looks a bit off

::: uriloader/prefetch/nsPrefetchService.cpp
@@ +139,5 @@
>      // configure HTTP specific stuff
>      nsCOMPtr<nsIHttpChannel> httpChannel =
>          do_QueryInterface(mChannel);
>      if (httpChannel) {
> +        httpChannel->SetReferrerWithPolicy(mReferrerURI, mReferrerPolicy);

so this didn't handle referrer policies before at all?

@@ +608,5 @@
> +    nsCOMPtr<nsINode> node(do_QueryInterface(aSource, &rv));
> +    if (NS_FAILED(rv)) return rv;
> +
> +    net::ReferrerPolicy referrerPolicy = node->OwnerDoc()->GetReferrerPolicy();
> +    net::ReferrerPolicy linkReferrerPolicy = net::RP_Unset;

same here

@@ +631,5 @@
>                       "document\n"));
>                  mCurrentNodes[i]->mSources.AppendElement(source);
> +                // The node is being prefetched, re-fetch with new referrer
> +                // policy
> +                if (linkReferrerPolicy != mCurrentNodes[i]->GetReferrerPolicy()) {

hm, not sure if want to do this. I think we shouldn't as it doesn't offer anything. But this should be checked with the spec (together with the img case).

@@ +883,5 @@
>      return NS_OK;
>  }
>  
>  // vim: ts=4 sw=4 expandtab
> +

?

Comment 5

[Thomas Nguyen (:tnguyen)]

> ::: uriloader/prefetch/nsPrefetchService.cpp
> @@ +139,5 @@
> >      // configure HTTP specific stuff
> >      nsCOMPtr<nsIHttpChannel> httpChannel =
> >          do_QueryInterface(mChannel);
> >      if (httpChannel) {
> > +        httpChannel->SetReferrerWithPolicy(mReferrerURI, mReferrerPolicy);
> 
> so this didn't handle referrer policies before at all?
It used default referrer policy before, and I think using doc's referrer policy should be more reasonable. For example, if we have a referrer policy set in meta tag, link element should use that to fetch resource

Comment 6

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8755816 [details] [diff] [review]
Update and add test

Would like to add more tests

Comment 7

[Thomas Nguyen (:tnguyen)]

Attachment: Update and add test

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8755816 [details] [diff] [review]
Update and add test

Review of attachment 8755816 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't actually check the tests yet because I think you can do this with significantly less code. That would also make it easier to read. You can for example have a look at the other referrer attribute test cases (e.g. test_anchor_area_referrer.html,  referrer_testserver.sjs, and referrerHelper.js).

Comment 9

[Thomas Nguyen (:tnguyen)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #4)
> Comment on attachment 8755815 [details] [diff] [review]
> Implement link referrerpolicy attribute
> 
> Review of attachment 8755815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think most of this looks pretty good already. See comments inline.
> Before giving this into review we should probably clarify the reload case.

I think it makes sense to do nothing if the request is sent out because it's too late.
I filed an issue in github and and I think we all agree that reloading the resource does not help.
I updated and noted some comments in the patch
https://github.com/w3c/resource-hints/issues/61#issuecomment-222177151

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: Implement link referrerpolicy attribute

Comment 11

[Thomas Nguyen (:tnguyen)]

Attachment: Update and add test

Comment 12

[Thomas Nguyen (:tnguyen)]

Attachment: Update and add test

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8757882 [details] [diff] [review]
Implement link referrerpolicy attribute

jdm volunteered to review this (I have a bit high review load)

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 8757882 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8757882 [details] [diff] [review]:
-----------------------------------------------------------------

These changes look like they do the job, but I think we can avoid some duplication. I'd like to see the next version of this patch.

::: dom/base/nsStyleLinkElement.cpp
@@ +434,5 @@
>        MOZ_LOG(SRILogHelper::GetSriLog(), mozilla::LogLevel::Debug,
>                ("nsStyleLinkElement::DoUpdateStyleSheet, integrity=%s",
>                 NS_ConvertUTF16toUTF8(integrity).get()));
>      }
> +    // if referrer attributes are enabled in preferences, load the link's referrer

nit: add a newline above this.

@@ +451,5 @@
>      NS_ENSURE_TRUE(clonedURI, NS_ERROR_OUT_OF_MEMORY);
>      rv = doc->CSSLoader()->
>        LoadStyleLink(thisContent, clonedURI, title, media, isAlternate,
> +                    GetCORSMode(), referrerPolicy == net::RP_Unset ?
> +                    doc->GetReferrerPolicy() : referrerPolicy , integrity,

Let's move this into a separate conditional check instead of including it inline in the arguments here.

::: uriloader/prefetch/nsPrefetchService.cpp
@@ +611,5 @@
> +    net::ReferrerPolicy referrerPolicy = net::RP_Unset;
> +    if (node->IsHTMLElement(nsGkAtoms::link)) {
> +      referrerPolicy =
> +        static_cast<dom::HTMLLinkElement*>(node.get())->GetLinkReferrerPolicy();
> +    }

It seems like overkill to do this check here, rather than as part of OpenChannel where we already have code to deal with the CORS attributes. Getting the referrer policy there would mean that we wouldn't need any special logic for dealing with nodes that are in the queue but haven't been fetched yet, either.

::: uriloader/prefetch/nsPrefetchService.h
@@ +119,5 @@
>      RefPtr<nsPrefetchService>   mService;
>      nsCOMPtr<nsIChannel>        mChannel;
>      nsCOMPtr<nsIChannel>        mRedirectChannel;
>      int64_t                     mBytesRead;
> +    net::ReferrerPolicy         mReferrerPolicy;

The changes in this file shouldn't be necessary if we modify OpenChannel instead.

Comment 15

[Thomas Nguyen (:tnguyen)]

(In reply to Josh Matthews [:jdm] from comment #14)
> 
> ::: uriloader/prefetch/nsPrefetchService.cpp
> @@ +611,5 @@
> > +    net::ReferrerPolicy referrerPolicy = net::RP_Unset;
> > +    if (node->IsHTMLElement(nsGkAtoms::link)) {
> > +      referrerPolicy =
> > +        static_cast<dom::HTMLLinkElement*>(node.get())->GetLinkReferrerPolicy();
> > +    }
> 
> It seems like overkill to do this check here, rather than as part of
> OpenChannel where we already have code to deal with the CORS attributes.
> Getting the referrer policy there would mean that we wouldn't need any
> special logic for dealing with nodes that are in the queue but haven't been
> fetched yet, either.

Nice. Thanks you for your review. This will be useful if we handle reloading (we decide not to reload), so should be moved to OpenChannel. I updated the patch.

Comment 16

[Thomas Nguyen (:tnguyen)]

Attachment: Implement link referrerpolicy attribute

Comment 17

[Thomas Nguyen (:tnguyen)]

Attachment: Implement link referrerpolicy attribute

Comment 18

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8757885 [details] [diff] [review]
Update and add test

Review of attachment 8757885 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. But we should keep an eye on bug 1268962 and if this gives us errors.

::: dom/base/test/referrer_testserver.sjs
@@ +178,5 @@
> +
> +  var onEvent = '';
> +  // XXX Bug 1268962 - Fire load/error events on <link rel="prefetch">
> +  // Currently, we dont actually know when prefetching finishes.
> +  // Fire event after a delay 2 secs

hm, this sounds like a possible intermittent error

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 8758093 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8758093 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsStyleLinkElement.cpp
@@ +442,5 @@
> +
> +    net::ReferrerPolicy referrerPolicy = net::RP_Unset;
> +    if (thisContent->IsHTMLElement(nsGkAtoms::link)) {
> +      referrerPolicy =
> +        static_cast<dom::HTMLLinkElement*>(thisContent.get())->GetLinkReferrerPolicy();

Looking more closely at this, can't we call nsStyleLinkElement's GetLinkReferrerPolicy directly without casting to a particular concrete type? If that's true, we shouldn't need the `if` check either.

::: uriloader/prefetch/nsPrefetchService.cpp
@@ +113,4 @@
>      if (source->IsHTMLElement(nsGkAtoms::link)) {
>        corsMode = static_cast<dom::HTMLLinkElement*>(source.get())->GetCORSMode();
> +      referrerPolicy =
> +        static_cast<dom::HTMLLinkElement*>(source.get())->GetLinkReferrerPolicy();

Let's make a local variable instead of casting for both of these method calls.

Comment 20

[Thomas Nguyen (:tnguyen)]

Thanks a lot Franziskus. Josh

Comment 21

[Thomas Nguyen (:tnguyen)]

Attachment: Implement link referrerpolicy attribute

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 8758517 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8758517 [details] [diff] [review]:
-----------------------------------------------------------------

Delightful! Thanks for making the changes I requested.

Comment 23

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=982b2ea903f4

Comment 24

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b97404473773

Comment 25

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9724712000

Comment 26

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29139a40cf46

Comment 27

[Thomas Nguyen (:tnguyen)]

Hmm, there's weird failure on try
https://treeherder.mozilla.org/logviewer.html#?job_id=21859042&repo=try#L1922
referrerPolicy is not treated as attribute on Android 4.3

Comment 28

[Thomas Nguyen (:tnguyen)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #18)
> Comment on attachment 8757885 [details] [diff] [review]
> Update and add test
> 
> Review of attachment 8757885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm. But we should keep an eye on bug 1268962 and if this gives us errors.
> 
> ::: dom/base/test/referrer_testserver.sjs
> @@ +178,5 @@
> > +
> > +  var onEvent = '';
> > +  // XXX Bug 1268962 - Fire load/error events on <link rel="prefetch">
> > +  // Currently, we dont actually know when prefetching finishes.
> > +  // Fire event after a delay 2 secs
> 
> hm, this sounds like a possible intermittent error

Hmm, I ran try and it has intermittent error exactly for prefetching case. But I am not sure how and when to to send event back.

Comment 29

[Thomas Nguyen (:tnguyen)]

Attachment: Update and add test

Use "prefetch_load_completed" event observer for current implementation

Comment 30

[Thomas Nguyen (:tnguyen)]

test timeout on non-e10s opt test machine

Comment 31

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b762572fe86

Comment 32

[Thomas Nguyen (:tnguyen)]

Attachment: Update and add test

Comment 33

[Thomas Nguyen (:tnguyen)]

Attachment: Update and add test

Comment 34

[Thomas Nguyen (:tnguyen)]

Attachment: Update and add test

Comment 35

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b561a353479&selectedJob=22718927

Comment 36

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8764200 [details] [diff] [review]
Update and add test

Could you please review the patch?
The patch only changes as below compared to the last one you reviewed:
- Separate prefetch and stylesheet test
- Using prefetch-load-completed listener (could remove after bug 1268962 is fixed)
- Skip test failed (Bug 1281415, could enable after the bug is fixed)
Thanks

Comment 37

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8764200 [details] [diff] [review]
Update and add test

Review of attachment 8764200 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. But why exactly do we need two different test cases test_link_prefetch.html and test_link_stylesheet.html?

Comment 38

[Thomas Nguyen (:tnguyen)]

Thanks for looking at. 

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #37)
> 
> lgtm. But why exactly do we need two different test cases
> test_link_prefetch.html and test_link_stylesheet.html?
Just separate the prefetch test cases, we could track bug 1268962 and bug 1281415 (these are only related to prefetch) and update later.

Comment 39

[Thomas Nguyen (:tnguyen)]

Try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b561a353479&selectedJob=22718927

Comment 40

[Carsten Book [:Tomcat]]

need dom peer review 
got from hg:
WebIDL file dom/webidl/HTMLLinkElement.webidl altered without DOM peer review

thanks!

Comment 41

[Thomas Nguyen (:tnguyen)]

Thanks you.
(In reply to Carsten Book [:Tomcat] from comment #40)
> need dom peer review 
> got from hg:
> WebIDL file dom/webidl/HTMLLinkElement.webidl altered without DOM peer review
> 
> thanks!

Hi Kyle,
Could you please take a look at that?
Thanks

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8758517 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8758517 [details] [diff] [review]:
-----------------------------------------------------------------

The WebIDL changes look fine.

Comment 43

[Thomas Nguyen (:tnguyen)]

Attachment: Implement link referrerpolicy attribute (dom-reviewed)

Nice! Thanks Kyle.

Updated commit's summary

Comment 44

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8764800 [details] [diff] [review]
Implement link referrerpolicy attribute  (dom-reviewed)

Carry r+

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e75889b87f3a
Implement link referrerPolicy attribute. r=josh, r=khuey
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9f5b5b4286
Update and add test cases. r=franziskus

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e75889b87f3a
https://hg.mozilla.org/mozilla-central/rev/4e9f5b5b4286

Comment 47

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/50#HTML
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link
https://developer.mozilla.org/en-US/docs/Web/API/HTMLLinkElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLLinkElement/referrerPolicy