Closed Bug 1264165 Opened 3 years ago Closed 3 years ago

Implement <link> referrerpolicy attribute

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- affected
firefox50 --- fixed

People

(Reporter: franziskus, Assigned: tnguyen)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-active)

Attachments

(2 files, 12 obsolete files)

22.29 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
7.53 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
No description provided.
Whiteboard: btpp-active
Status: NEW → ASSIGNED
Attached patch Update and add test (obsolete) — Splinter Review
Attachment #8755815 - Flags: feedback?(franziskuskiefer)
Attachment #8755816 - Flags: feedback?(franziskuskiefer)
Hi Franziskus,
Could you please take a look and let me know if I am in incorrect way of missing anything?
Thanks
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
> +

?
Attachment #8755815 - Flags: feedback?(franziskuskiefer) → feedback+
> ::: 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 on attachment 8755816 [details] [diff] [review]
Update and add test

Would like to add more tests
Attachment #8755816 - Flags: feedback?(franziskuskiefer)
Attached patch Update and add test (obsolete) — Splinter Review
Attachment #8755816 - Attachment is obsolete: true
Attachment #8755816 - Flags: feedback?(franziskuskiefer)
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).
Attachment #8755816 - Flags: feedback?(franziskuskiefer)
(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
Attachment #8755815 - Attachment is obsolete: true
Attachment #8757882 - Flags: review?(bugs)
Attachment #8757882 - Flags: feedback+
Attached patch Update and add test (obsolete) — Splinter Review
Attachment #8756226 - Attachment is obsolete: true
Attached patch Update and add test (obsolete) — Splinter Review
Attachment #8757883 - Attachment is obsolete: true
Attachment #8757885 - Flags: review?(franziskuskiefer)
Comment on attachment 8757882 [details] [diff] [review]
Implement link referrerpolicy attribute

jdm volunteered to review this (I have a bit high review load)
Attachment #8757882 - Flags: review?(bugs) → review?(josh)
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.
Attachment #8757882 - Flags: review?(josh)
(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.
Attachment #8757882 - Attachment is obsolete: true
Attachment #8758091 - Attachment is obsolete: true
Attachment #8758093 - Flags: review?(josh)
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
Attachment #8757885 - Flags: review?(franziskuskiefer) → review+
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.
Attachment #8758093 - Flags: review?(josh)
Thanks a lot Franziskus. Josh
Attachment #8758093 - Attachment is obsolete: true
Attachment #8758517 - Flags: review?(josh)
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.
Attachment #8758517 - Flags: review?(josh) → review+
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
(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.
Attached patch Update and add test (obsolete) — Splinter Review
Use "prefetch_load_completed" event observer for current implementation
Attachment #8757885 - Attachment is obsolete: true
Attachment #8760293 - Flags: review?(franziskuskiefer)
Attachment #8760293 - Flags: review?(franziskuskiefer)
test timeout on non-e10s opt test machine
Attachment #8763834 - Attachment is obsolete: true
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
Attachment #8764200 - Flags: review?(franziskuskiefer)
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?
Attachment #8764200 - Flags: review?(franziskuskiefer) → review+
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.
Keywords: checkin-needed
need dom peer review 
got from hg:
WebIDL file dom/webidl/HTMLLinkElement.webidl altered without DOM peer review

thanks!
Flags: needinfo?(tnguyen)
Keywords: checkin-needed
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
Flags: needinfo?(tnguyen)
Attachment #8758517 - Flags: review?(khuey)
Comment on attachment 8758517 [details] [diff] [review]
Implement link referrerpolicy attribute

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

The WebIDL changes look fine.
Attachment #8758517 - Flags: review?(khuey) → review+
Nice! Thanks Kyle.

Updated commit's summary
Comment on attachment 8764800 [details] [diff] [review]
Implement link referrerpolicy attribute  (dom-reviewed)

Carry r+
Attachment #8764800 - Attachment description: Update and add test (dom-reviewed) → Implement link referrerpolicy attribute (dom-reviewed)
Attachment #8764800 - Flags: review+
Attachment #8758517 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e75889b87f3a
https://hg.mozilla.org/mozilla-central/rev/4e9f5b5b4286
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Summary: Implement <link> referrer attribute → Implement <link> referrerpolicy attribute
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.