Implement <img> referrer attribute

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: fkiefer, Assigned: fkiefer)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla42
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(5 attachments, 10 obsolete attachments)

3.93 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
52.21 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
23.44 KB, patch
Details | Diff | Splinter Review
16.46 KB, patch
fkiefer
: review+
Details | Diff | Splinter Review
1.20 KB, patch
Details | Diff | Splinter Review
Implementing the referrer attribute for the image tag to specify a referrer policy for images.

http://w3c.github.io/webappsec/specs/referrer-policy/#referrer-policy-delivery-referrer-attribute
Blocks: 999754
Created attachment 8616799 [details] [diff] [review]
Teach parser about referrer (java)
Attachment #8616799 - Flags: review?(hsivonen)
Created attachment 8616800 [details] [diff] [review]
Teach parser about referrer (generated bits)
Attachment #8616800 - Flags: review?(hsivonen)
Created attachment 8616801 [details] [diff] [review]
referrer attribute for img tag (r=ckerschb)
Attachment #8616801 - Flags: review?(mozilla)
Comment on attachment 8616801 [details] [diff] [review]
referrer attribute for img tag (r=ckerschb)

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

Hi Franziskus, I took a first look at this. Looks pretty good to me. Please fix all my nits and please split the patch, so that the test is in it's own patch. Once cleaned up, you can flag me again and I will also take a look at the logic. Cool Stuff - Excited to see this happening!

::: dom/base/nsImageLoadingContent.cpp
@@ +960,5 @@
> +  mozilla::net::ReferrerPolicy imgReferrerPolicy;
> +  GetImageReferrerPolicy(imgReferrerPolicy);
> +  // replace referrerPolicy with the one define in the img tag
> +  if (imgReferrerPolicy != mozilla::net::RP_Unset)
> +    referrerPolicy = imgReferrerPolicy;

You don't need to query both policies, right?
Query the document wide one only if the image does not provide it's own.

mozilla::net::ReferrerPolicy imgReferrerPolicy;
rv = GetImageReferrerPolicy(imgReferrerPolicy);
NS_ENSURE_SUCCESS(rv, rv);
// if the image does not provide it's own referrer, fall back to the documents one
if (imgReferrerPolicy == mozilla::net::RP_Unset) {
  ...
}

Please make sure do add braces everywhere.

@@ +1044,5 @@
> +//     *aReferrerPolicy = mozilla::net::RP_Default;
> +//   }
> +
> +//   return NS_OK;
> +// }

If the code is unused, please remove.

@@ +1613,5 @@
> +//   // FIXME: make this something nully fall back to document wide referrer when not implemented
> +//   aReferrerPolicy = mozilla::net::RP_No_Referrer;
> +  
> +//   return NS_OK;
> +// }

Same here, please remove if not used.

::: dom/base/nsImageLoadingContent.h
@@ +24,5 @@
>  #include "nsAutoPtr.h"
>  #include "nsIContentPolicy.h"
>  #include "mozilla/dom/BindingDeclarations.h"
>  
> +#include "mozilla/net/ReferrerPolicy.h"

nit: please remove the empty line above.

@@ +204,5 @@
> +  {
> +    aReferrerPolicy = mozilla::net::RP_Unset;
> +
> +    return NS_OK; 
> +  };

I am not sure if we inline virtual functions, please move to the *.cpp; remove the empty line and the whitespace after NS_OK;

::: dom/html/HTMLImageElement.cpp
@@ +305,5 @@
>    SetWidth(aWidth, rv);
>    return rv.StealNSResult();
>  }
>  
> +

please remove that empty line.

::: dom/html/HTMLImageElement.h
@@ +202,5 @@
> +  GetImageReferrerPolicy(mozilla::net::ReferrerPolicy &aReferrerPolicy)
> +  {
> +    GetReferrerPolicy(aReferrerPolicy);
> +
> +    return NS_OK;  

Nit: please remove the empty line and also the whitespace after NS_OK;

::: dom/html/nsGenericHTMLElement.cpp
@@ +998,5 @@
>      }
>  
> +    if (aAttribute == nsGkAtoms::referrer) {
> +      ParseReferrerAttribute(aValue, aResult);
> +      return true;

return ParseReferrerAttribute(aValue, aResult);

@@ +1270,5 @@
>  }
>  
>  bool
> +nsGenericHTMLElement::ParseReferrerAttribute(const nsAString& aString,
> +                                          nsAttrValue& aResult)

nit: please align arguments

::: dom/html/nsGenericHTMLElement.h
@@ +240,5 @@
> +    GetEnumAttr(nsGkAtoms::referrer, nullptr, aPolicyString);
> +    if (!aPolicyString.IsEmpty())
> +      aReferrerPolicy = mozilla::net::ReferrerPolicyFromString(aPolicyString);
> +    else
> +      aReferrerPolicy = mozilla::net::RP_Unset;

please use early returns, e.g.
if (aPolicy.IsEmpty()) {
   aReferrerPolicy = mozilla::net::RP_Unset;
   return NS_OK;
}
aReferrerPolicy = mozilla::net::ReferrerPolicyFromString(aPolicyString);
return NS_OK;

@@ +242,5 @@
> +      aReferrerPolicy = mozilla::net::ReferrerPolicyFromString(aPolicyString);
> +    else
> +      aReferrerPolicy = mozilla::net::RP_Unset;
> +
> +    return NS_OK;  

nit: trailing whitespace

@@ +725,5 @@
>                                      const nsAString& aString,
>                                      nsAttrValue& aResult);
> +
> +  static bool ParseReferrerAttribute(const nsAString& aString,
> +                                    nsAttrValue& aResult);

nit: please align arguments.

::: dom/interfaces/html/nsIDOMHTMLImageElement.idl
@@ +23,5 @@
>             attribute DOMString        src;
>             attribute DOMString        srcset;
>             attribute DOMString        sizes;
>             attribute DOMString        useMap;
> +           attribute DOMString        referrer;

you have to ref the uuid if you add an attribute, you can use:
./mach uuid

::: image/imgLoader.cpp
@@ +669,5 @@
>                       int32_t corsmode, nsIPrincipal* loadingPrincipal,
>                       nsISupports* aCX, ReferrerPolicy referrerPolicy)
>  {
>    // If the entry's Referrer Policy doesn't match, we can't use this request.
> +  // XXX: this will return false if an attribute uses a different policy than the document 

what does that mean? how do we fix this?
Attachment #8616801 - Flags: review?(mozilla)
Attachment #8616799 - Flags: review?(hsivonen) → review+
Attachment #8616800 - Flags: review?(hsivonen) → review+
Created attachment 8620696 [details] [diff] [review]
referrer attribute for img tag, src

fixed all points and some more.

Referrer attribute is now behind preference network.http.useReferrerAttributes (false by default).
Attachment #8616801 - Attachment is obsolete: true
Attachment #8620696 - Flags: review?(mozilla)
Created attachment 8620697 [details] [diff] [review]
referrer attribute for img tag, tests
Attachment #8620697 - Flags: review?(mozilla)
Comment on attachment 8620696 [details] [diff] [review]
referrer attribute for img tag, src

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

Looks good; getting there. Would like to see it again with my comments addressed.

::: dom/base/nsImageLoadingContent.cpp
@@ +73,5 @@
>    uri->GetSpec(spec);
>    printf("spec='%s'\n", spec.get());
>  }
>  #endif /* DEBUG_chb */
>  

please revert that change!

@@ +97,5 @@
>  {
>    if (!nsContentUtils::GetImgLoaderForChannel(nullptr, nullptr)) {
>      mLoadingEnabled = false;
>    }
> +

same here, please revert that change.

@@ +957,5 @@
>    }
>  
> +  nsresult rv;
> +  // get img referrer policy if there is one
> +  mozilla::net::ReferrerPolicy imgReferrerPolicy;

don't call this imgReferrerPolicy, might also be the policy of the document.

@@ +958,5 @@
>  
> +  nsresult rv;
> +  // get img referrer policy if there is one
> +  mozilla::net::ReferrerPolicy imgReferrerPolicy;
> +  rv = GetImageReferrerPolicy(imgReferrerPolicy);

you don't have to define nsresult rv above, just use it here and initialize it right away.

@@ +966,5 @@
> +  bool referrerAttributeEnabled = false;
> +  referrerAttributeEnabled = Preferences::GetBool("network.http.useReferrerAttributes", referrerAttributeEnabled);
> +  if (imgReferrerPolicy == mozilla::net::RP_Unset || !referrerAttributeEnabled) {
> +    imgReferrerPolicy = aDocument->GetReferrerPolicy();
> +  }

so, here is how I would do it, since this also lives behind a pref. From a semantic point of view we have a document referrer policy, right? 
(1) init a variable with the document wide referrer
(2) check the bool pref
(3) if enabled, then query the img referrer and overrule the document wide one within that if-statement.

@@ +1604,5 @@
>    MOZ_COUNT_DTOR(ImageObserver);
>    NS_CONTENT_DELETE_LIST_MEMBER(ImageObserver, this, mNext);
>  }
> +
> +nsresult 

nit, trailing whitespace.

@@ +1607,5 @@
> +
> +nsresult 
> +nsImageLoadingContent::GetImageReferrerPolicy(mozilla::net::ReferrerPolicy& aReferrerPolicy)
> +{
> +  aReferrerPolicy = mozilla::net::RP_Unset;

We should add a comment on top and define who falls back to call this base class and who overrides it. As I see it, at the moment only HTMLImageElement overrides this function, right? But Firefox supports loading SVGs within an image tag, right? So probably also SVGImageElement should override it and not fall back to the base class.

There are also this other classes that inherit nsImageLoadingContent:
/dom/base/nsGenConImageContent.cpp
/dom/base/nsObjectLoadingContent.h
/dom/html/HTMLInputElement.h
/dom/svg/SVGFEImageElement.h

Not sure what an nsGenConImage is, but we should make sure. Definitely need a testcase for loading an SVG using an img tag.

::: dom/html/HTMLImageElement.cpp
@@ +238,5 @@
> +NS_IMETHODIMP
> +HTMLImageElement::GetReferrer(nsAString& aReferrer)
> +{
> +  GetEnumAttr(nsGkAtoms::referrer, nullptr, aReferrer);
> +

nit, remove empty line.

::: dom/html/HTMLImageElement.h
@@ +194,5 @@
> +  NS_IMETHODIMP
> +  GetImageReferrerPolicy(mozilla::net::ReferrerPolicy &aReferrerPolicy)
> +  {
> +    GetReferrerPolicy(aReferrerPolicy);
> +    return NS_OK;

return GetReferrerPolicy

::: image/imgLoader.cpp
@@ +671,5 @@
>  {
>    // If the entry's Referrer Policy doesn't match, we can't use this request.
> +  // XXX: this will return false if an image has different referrer attributes,
> +  // i.e. we currently don't use the cached image but reload the image with
> +  // the new referrer policy

comment ok, but we still need to determine if we want to do that. Dan Veditz should help us make a decision.

::: modules/libpref/init/all.js
@@ +1260,5 @@
>  // Controls whether we send HTTPS referres to other HTTPS sites.
>  // By default this is enabled for compatibility (see bug 141641)
>  pref("network.http.sendSecureXSiteReferrer", true);
>  
> +// Controls whether referrer attributes in a, img, area, and iframe are honoured

nit, please write the tage like this
<a>, <area>, <img> or <iframe>
otherwise the <a> might get lost
Attachment #8620696 - Flags: review?(mozilla)
Comment on attachment 8620697 [details] [diff] [review]
referrer attribute for img tag, tests

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

Looks good, I like that the *.sjs is generating all the html instead of loading external files. Let me see it one more time before we can hand it off to some other reviewer.

::: dom/base/test/bug1166910.sjs
@@ +6,5 @@
> +         'policy=' + policy + '&' +
> +         'name=' + name;
> +}
> +
> +function createTestPage(head, imgPolicy, name) {

please also prefix with a, otherwise the head unerneath looks like you forgot to add the <> around it.

@@ +9,5 @@
> +
> +function createTestPage(head, imgPolicy, name) {
> +  var _createTestUrl = createTestUrl.bind(null, imgPolicy, 'test', name);
> +
> +  return '<!DOCTYPE HTML>\n\

instead of using \n you could use the same as you do above, closing the string literal using " and using +
but that's up to you.

@@ +33,5 @@
> +//   <img> with referrer
> +function createTest(policy, imgPolicy, name) {
> +  var headString = '<head>';
> +  if (policy)
> +    headString += '<meta name="referrer" content="' + policy + '">';

please brace your ifs

@@ +103,5 @@
> +    var state = getSharedState(sharedKey);
> +    state = {};
> +    setSharedState(sharedKey, JSON.stringify(state));
> +    response.write("");
> +  } else if (action === 'test') {

can you rewrite all this if-else statements and use early returns instead?

@@ +192,5 @@
> +    var otherPolicy = unescape(params[2].split('=')[1]);
> +    var name = unescape(params[3].split('=')[1]);
> +
> +    response.write(createReadOnlyTest(imgPolicy, otherPolicy, name));
> +  }

maybe then also send some error message back in case no if (early return is taken).

::: dom/base/test/mochitest.ini
@@ +55,5 @@
>    bug696301-script-2.js
>    bug704320.sjs
>    bug704320_counter.sjs
>    bug819051.sjs
> +  bug1166910.sjs

Nit, please rename to img_referrer_testserver.sjs or something like that. I think in general we would like to get away from using bugnumbers as file names.

@@ +680,5 @@
>  [test_bug1163743.html]
>  support-files = referrerHelper.js
>  [test_bug1165501.html]
>  support-files = referrerHelper.js
> +[test_bug1166910.html]

same nit here, please rename.

::: dom/base/test/test_bug1166910.html
@@ +2,5 @@
> +<html>
> +<!--
> +Spot test to see if newer meta-referrer policy is used
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1165501
> +-->

Maybe add a small description what the test is doing.
Loading 3 images using 3 different policies, etc, so people get an idea of what we are testing here.

@@ +9,5 @@
> +  <meta charset="utf-8">
> +  <title>Test policies for Bug 1165501</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +

does all that need to go into the head? or is otherwise the spec-parser not triggered? If so, please also add a small comment so in case people are going to change/extend the test, they know about that.

@@ +43,5 @@
> +      onSuccess(xhr);
> +    } else {
> +      onFail(xhr);
> +    }
> +  };

I think you can use onload and onerror instead of quering the status codes.
Attachment #8620697 - Flags: review?(mozilla)
Created attachment 8621654 [details] [diff] [review]
referrer attribute for img tag, src
Attachment #8620696 - Attachment is obsolete: true
Attachment #8621654 - Flags: review?(mozilla)
Created attachment 8621655 [details] [diff] [review]
referrer attribute for img tag, tests

added regression tests that meta is still working if referrer attributes are enabled
Attachment #8620697 - Attachment is obsolete: true
Attachment #8621655 - Flags: review?(mozilla)
Created attachment 8621714 [details] [diff] [review]
referrer attribute for img tag, src

fixed windows compilation bug

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07c896c81326
Attachment #8621654 - Attachment is obsolete: true
Attachment #8621654 - Flags: review?(mozilla)
Attachment #8621714 - Flags: review?(mozilla)
Comment on attachment 8621714 [details] [diff] [review]
referrer attribute for img tag, src

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

Great - really excited to see that happening.

@Sid: I don't know whether you would like to review those patches as well? if you are fine with my r+, we could hand it off to a dom peer for review. Any suggestions?

::: modules/libpref/init/all.js
@@ +1268,5 @@
>  pref("network.http.sendSecureXSiteReferrer", true);
>  
> +// Controls whether referrer attributes in <a>, <img>, <area>, and <iframe> are honoured
> +pref("network.http.useReferrerAttributes", false);
> +

Sorry, I haven't pointed that out in the previous review, maybe you can change that to:
> network.http.enablePerElementReferrer
Attachment #8621714 - Flags: review?(sstamm)
Attachment #8621714 - Flags: review?(mozilla)
Attachment #8621714 - Flags: review+
Comment on attachment 8621655 [details] [diff] [review]
referrer attribute for img tag, tests

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

::: dom/base/test/test_img_referrer.html
@@ +34,5 @@
> +    }
> +    else if (event.data.contains("childLoadComplete")) {
> +      mTestResult = event.data.split(",")[1];
> +      advance();
> +    }

nit: sometimes you use 4 spaces and sometimes only 2 spaces for indentation.
Attachment #8621655 - Flags: review?(sstamm)
Attachment #8621655 - Flags: review?(mozilla)
Attachment #8621655 - Flags: review+
Comment on attachment 8621714 [details] [diff] [review]
referrer attribute for img tag, src

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

> @Sid: I don't know whether you would like to review those patches as well?

I think your review is sufficient from the DOM::Security perspective, but I took a quick look at the changes in ReferrerPolicy.h and nsIHttpChannel.idl and will give some comments.

> if you are fine with my r+, we could hand it off to a dom peer for review. 
> Any suggestions?

Works for me.  You probably want someone from necko to approve the interface changes in netwerk.

::: netwerk/base/ReferrerPolicy.h
@@ +27,5 @@
>    /* spec tokens: always unsafe-url */
> +  RP_Unsafe_URL                  = nsIHttpChannel::REFERRER_POLICY_UNSAFE_URL,
> +
> +  /* referrer policy is not set */
> +  RP_Unset                       = nsIHttpChannel::REFERRER_POLICY_NOT_SET

RP_Unset gives me pause since it's not absolutely clear how this is different than RP_Default.  Isn't that the same?  If it is, we should remove either RP_Default or RP_Unset and make them effectively be the same thing.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +71,5 @@
>      const unsigned long REFERRER_POLICY_ORIGIN_WHEN_XORIGIN        = 3;
>      /*   always sends the referrer, even on downgrade.                  */
>      const unsigned long REFERRER_POLICY_UNSAFE_URL                 = 4;
> +    /*   no referrer attribute set                                      */
> +    const unsigned long REFERRER_POLICY_NOT_SET                    = 5;

I am uneasy about using "NOT_SET" because the rest of the tokens are valid policies and this one is not.  (see my comment in ReferrerPolicy.h)
Attachment #8621714 - Flags: review?(sstamm)
Comment on attachment 8621655 [details] [diff] [review]
referrer attribute for img tag, tests

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

I think r=ckerschb is sufficient for tests of this feature.
Attachment #8621655 - Flags: review?(sstamm)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14)
> ::: netwerk/base/ReferrerPolicy.h
> @@ +27,5 @@
> >    /* spec tokens: always unsafe-url */
> > +  RP_Unsafe_URL                  = nsIHttpChannel::REFERRER_POLICY_UNSAFE_URL,
> > +
> > +  /* referrer policy is not set */
> > +  RP_Unset                       = nsIHttpChannel::REFERRER_POLICY_NOT_SET
> 
> RP_Unset gives me pause since it's not absolutely clear how this is
> different than RP_Default.  Isn't that the same?  If it is, we should remove
> either RP_Default or RP_Unset and make them effectively be the same thing.
> 

RP_Unset is not an actually value but is returned when a tag has no referrer attribute. Without changing additional code we always have to hand over a ReferrerPolicy Enum. That's why I added it to the enum. We could actually reuse RP_Default because that's not a valid referrer attribute, but I'd prefer this new value to easier distinguish between these two cases in the code.

> ::: netwerk/protocol/http/nsIHttpChannel.idl
> @@ +71,5 @@
> >      const unsigned long REFERRER_POLICY_ORIGIN_WHEN_XORIGIN        = 3;
> >      /*   always sends the referrer, even on downgrade.                  */
> >      const unsigned long REFERRER_POLICY_UNSAFE_URL                 = 4;
> > +    /*   no referrer attribute set                                      */
> > +    const unsigned long REFERRER_POLICY_NOT_SET                    = 5;
> 
> I am uneasy about using "NOT_SET" because the rest of the tokens are valid
> policies and this one is not.  (see my comment in ReferrerPolicy.h)

Ok, we could move it out of there or rename it, but if we want to keep the RP_Unset enum value, we should define it somewhere in the idl. Something that should solve your points is reusing REFERRER_POLICY_DEFAULT for RP_Unset.
Blocks: 1174913
Flags: needinfo?(sstamm)
Blocks: 1175736
> Something that should solve your points is reusing 
> REFERRER_POLICY_DEFAULT for RP_Unset.

Yeah, this might be best.
Flags: needinfo?(sstamm)
Created attachment 8624481 [details] [diff] [review]
referrer attribute for img tag, src
Attachment #8621714 - Attachment is obsolete: true
Attachment #8624481 - Flags: review?(hsivonen)
Created attachment 8624485 [details] [diff] [review]
referrer attribute for img tag, tests
Attachment #8621655 - Attachment is obsolete: true
Attachment #8624485 - Flags: review?(hsivonen)
Comment on attachment 8624481 [details] [diff] [review]
referrer attribute for img tag, src

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

r+ if the two comments are addressed.

::: dom/base/nsImageLoadingContent.h
@@ +198,5 @@
>     * default implementation returns CORS_NONE unconditionally.
>     */
>    virtual mozilla::CORSMode GetCORSMode();
>  
> +  virtual nsresult GetImageReferrerPolicy(mozilla::net::ReferrerPolicy& aReferrerPolicy);

The nsresult return type seems useless, since it always returns NS_OK. Please make this take zero arguments and return mozilla::net::ReferrerPolicy instead.

::: parser/html/nsHtml5SpeculativeLoad.h
@@ +180,5 @@
>    private:
>      eHtml5SpeculativeLoad mOpCode;
>      nsString mUrl;
>      nsString mMetaReferrerPolicy;
> +    nsString mImageReferrerPolicy;

In order to avoid uselessly bloating the size of speculative load objects, please don't add new members here when you don't need a larger number of members than are already there. Instead, please rename mMetaReferrerPolicy to mReferrerPolicy and reuse that.
Attachment #8624481 - Flags: review?(hsivonen) → review+
Comment on attachment 8624485 [details] [diff] [review]
referrer attribute for img tag, tests

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

r+ with the below nits fixed.

::: dom/base/test/test_img_referrer.html
@@ +7,5 @@
> +* loading a single image multiple times with different policies (generate-img-policy-test3)
> +* testing setAttribute and .referrer (generate-setAttribute-test)
> +* regression tests that meta referrer is still working even if attribute referrers are enabled
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1166910
> +-->

Please move the comment after <meta charset>. Having comments before <meta charset> risks the <meta> prescan failing. (I didn't count the bytes here, but it's a good idea to never have any comments before <meta charset>.)

@@ +41,5 @@
> +/**
> + * helper to perform an XHR.
> + */
> +function doXHR(aUrl, onSuccess, onFail) {
> +  var xhr = new XMLHttpRequest();

Please set xhr.responseType = "json"; here.

@@ +59,5 @@
> +function checkIndividualResults(aTestname, aExpectedImg, aName) {
> +  doXHR('/tests/dom/base/test/img_referrer_testserver.sjs?action=get-test-results',
> +        function(xhr) {
> +          var results = JSON.parse(xhr.responseText);
> +          info(xhr.responseText);

Please use xhr.response here (thanks to responseType = "json") and omit the JSON.parse call.
Attachment #8624485 - Flags: review?(hsivonen) → review+
Created attachment 8627709 [details] [diff] [review]
referrer attribute for img tag, src

r+ carries over
Attachment #8624481 - Attachment is obsolete: true
Attachment #8627709 - Flags: review+
Created attachment 8627710 [details] [diff] [review]
referrer attribute for img tag, tests

r+ carries over
Attachment #8624485 - Attachment is obsolete: true
Attachment #8627710 - Flags: review+
Keywords: checkin-needed

Comment 24

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f3827ffcf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/981a1dbe042b
Keywords: checkin-needed
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=11248314&repo=mozilla-inbound
Flags: needinfo?(franziskuskiefer)

Comment 26

3 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b492dc1934f
https://hg.mozilla.org/integration/mozilla-inbound/rev/74795c76fadc
It seems to me that https://bug1166910.bugzilla.mozilla.org/attachment.cgi?id=8616800 was not checked in, which defines 'ATTR_REFERRER'.
Flags: needinfo?(franziskuskiefer) → needinfo?(cbook)
Attachment #8616799 - Attachment description: Teach parser about referrer (generated bits) → Teach parser about referrer (java)
a try run (looks all good): https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd5d9e37941d
the following patches need checkin (in this order)
* Teach parser about referrer (generated bits)
* referrer attribute for img tag, src 
* referrer attribute for img tag, tests
Keywords: checkin-needed
webidl changes need DOM peer review
Flags: needinfo?(cbook) → needinfo?(franziskuskiefer)
Keywords: checkin-needed
Henri is DOM peer, do you want another one?
Flags: needinfo?(franziskuskiefer) → needinfo?(ryanvm)
Please don't change nsIDOMHTMLImageElement at all.  Just add this to the webidl interface and implement only the webidl version.
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #30)
> Henri is DOM peer, do you want another one?

Odd, he's listed on the hg hook:
https://hg.mozilla.org/hgcustom/version-control-tools/file/62dfcd2ca7bf/hghooks/mozhghooks/prevent_webidl_changes.py

But it rejected my push :\. Maybe my spelling sucked. Anyway, sounds like bz wants you to change the patch now anyway.
Flags: needinfo?(ryanvm)
Keywords: dev-doc-needed
Created attachment 8628548 [details] [diff] [review]
referrer attribute for img tag, src
Attachment #8627709 - Attachment is obsolete: true
Attachment #8628548 - Flags: review?(bzbarsky)
Created attachment 8628549 [details] [diff] [review]
referrer attribute for img tag, tests

(clean up) r+ carries over
Attachment #8627710 - Attachment is obsolete: true
Attachment #8628549 - Flags: review+
Comment on attachment 8628548 [details] [diff] [review]
referrer attribute for img tag, src

r=me on just the webidl and HTMLImageElement.h bits.

I'm not sure why the referrer attr parsing is being done generically for all HTML elements, and it wasn't that way in the earlier reviewed patches on this bug, but I'm going to assume someone has reviewed that part and is OK with it.  If not, let me know...
Attachment #8628548 - Flags: review?(bzbarsky) → review+
The referrer attribute is used in other HTML elements as well (see bug 1174913 and bug 1175736) and therefore done generically for all elements.
Keywords: checkin-needed

Comment 37

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a48456e8eaa5
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d7963ccfb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fae1257ad4e
Keywords: checkin-needed
The second commit here was missing an 'override' annotation on GetImageReferrerPolicy() (declared in nsImageLoadingContent and overridden in HTMLImageElement). The missing 'override' causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I'm pushing a followup (posted here via pulsebot shortly) to add that keyword, with blanket r+ that ehsan
granted me for fixes of this sort over in bug 1126447 comment 2.

Comment 39

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8a4fd19f56
https://hg.mozilla.org/mozilla-central/rev/a48456e8eaa5
https://hg.mozilla.org/mozilla-central/rev/78d7963ccfb0
https://hg.mozilla.org/mozilla-central/rev/3fae1257ad4e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
https://hg.mozilla.org/mozilla-central/rev/8c8a4fd19f56
Was there an intent to implement/ship sent for this?  If not, why not?  (Yes, I should have asked this before granting review on the IDL...)
Flags: needinfo?(franziskuskiefer)
This is still disabled behind a pref. We currently prepare the intent to implement/ship.
Flags: needinfo?(franziskuskiefer)
> This is still disabled behind a pref.

It is?  The IDL changes were not disabled behind a pref!
Flags: needinfo?(franziskuskiefer)
The pref only disables applying of the referrer policy, not parsing (in order to avoid unnecessary code). If you strongly prefer disabling the parsing as well, we could add that (same goes for bug 1174913 and bug 1175736 implementing referrer policies for iframe, a, and anchor).
Flags: needinfo?(franziskuskiefer) → needinfo?(bzbarsky)
The parsing is not a big deal, since it's not script-visible, right?

But having the IDL property present without actually applying the policy seems pretty broken to me.  In particular, it breaks the ability to do detection of whether referrer policy is supported, no?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(franziskuskiefer)
Created attachment 8630765 [details] [diff] [review]
put attribute behind pref in webidl, r=bz

right, we should put the attribute behind the pref as well.
Flags: needinfo?(franziskuskiefer)
Attachment #8630765 - Flags: review?(bzbarsky)
Comment on attachment 8630765 [details] [diff] [review]
put attribute behind pref in webidl, r=bz

r=me
Attachment #8630765 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed

Comment 49

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/674efa62ef17
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/674efa62ef17
:fkiefer, I am in the progress of documenting this. I don't find a mention HTMLImageElement.referrer (as well as about HTML{Anchor,Area,IFrame}Element.referrer in the spec [1]. Is this defined elsewhere or should we file a spec bug?

In the latter case, how?


[1] http://w3c.github.io/webappsec/specs/referrer-policy/
Flags: needinfo?(franziskuskiefer)
I'll rename the relevant pages, when it will be renamed (dev-doc-needed is set on the relevant bug)

Documentation updated:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img
https://developer.mozilla.org/en-US/Firefox/Releases/42#HTML

New page created:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/referrer
Keywords: dev-doc-needed → dev-doc-complete
The spec is not there yet. [1] mentions it and should be extended when the attribute is renamed (and then probably should also be added to the html spec, but I guess that should wait til the referrer spec is more stable).

[1] https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery-referrer-attribute
Flags: needinfo?(franziskuskiefer)
You need to log in before you can comment on or make changes to this bug.