1166910 - Implement <img> referrer attribute

Status: RESOLVED FIXED

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

Description

[Franziskus Kiefer [:franziskus]]

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

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: Teach parser about referrer (java)

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: Teach parser about referrer (generated bits)

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag (r=ckerschb)

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, src

fixed all points and some more.

Referrer attribute is now behind preference network.http.useReferrerAttributes (false by default).

Comment 6

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, tests

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 9

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, src

Comment 10

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, tests

added regression tests that meta is still working if referrer attributes are enabled

Comment 11

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, src

fixed windows compilation bug

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

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

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)

Comment 15

[Sid Stamm [:geekboy or :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.

Comment 16

[Franziskus Kiefer [:franziskus]]

(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.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

> Something that should solve your points is reusing 
> REFERRER_POLICY_DEFAULT for RP_Unset.

Yeah, this might be best.

Comment 18

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, src

Comment 19

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, tests

Comment 20

[Henri Sivonen (: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.

Comment 21

[Henri Sivonen (:hsivonen)]

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.

Comment 22

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, src

r+ carries over

Comment 23

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, tests

r+ carries over

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f3827ffcf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/981a1dbe042b

Comment 25

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=11248314&repo=mozilla-inbound

Comment 26

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b492dc1934f
https://hg.mozilla.org/integration/mozilla-inbound/rev/74795c76fadc

Comment 27

[Franziskus Kiefer [:franziskus]]

It seems to me that https://bug1166910.bugzilla.mozilla.org/attachment.cgi?id=8616800 was not checked in, which defines 'ATTR_REFERRER'.

Comment 28

[Franziskus Kiefer [:franziskus]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

webidl changes need DOM peer review

Comment 30

[Franziskus Kiefer [:franziskus]]

Henri is DOM peer, do you want another one?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Please don't change nsIDOMHTMLImageElement at all.  Just add this to the webidl interface and implement only the webidl version.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 33

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, src

Comment 34

[Franziskus Kiefer [:franziskus]]

Attachment: referrer attribute for img tag, tests

(clean up) r+ carries over

Comment 35

[Boris Zbarsky [:bzbarsky]]

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...

Comment 36

[Franziskus Kiefer [:franziskus]]

The referrer attribute is used in other HTML elements as well (see bug 1174913 and bug 1175736) and therefore done generically for all elements.

Comment 37

[Pulsebot]

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

Comment 38

[Daniel Holbert [:dholbert]]

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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8a4fd19f56

Comment 40

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

https://hg.mozilla.org/mozilla-central/rev/a48456e8eaa5
https://hg.mozilla.org/mozilla-central/rev/78d7963ccfb0
https://hg.mozilla.org/mozilla-central/rev/3fae1257ad4e

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c8a4fd19f56

Comment 42

[Boris Zbarsky [:bzbarsky]]

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...)

Comment 43

[Franziskus Kiefer [:franziskus]]

This is still disabled behind a pref. We currently prepare the intent to implement/ship.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> This is still disabled behind a pref.

It is?  The IDL changes were not disabled behind a pref!

Comment 45

[Franziskus Kiefer [:franziskus]]

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).

Comment 46

[Boris Zbarsky [: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?

Comment 47

[Franziskus Kiefer [:franziskus]]

Attachment: put attribute behind pref in webidl, r=bz

right, we should put the attribute behind the pref as well.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8630765 [details] [diff] [review]
put attribute behind pref in webidl, r=bz

r=me

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/674efa62ef17

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/674efa62ef17

Comment 51

[Jean-Yves Perrier [:teoli]]

: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/

Comment 52

[Jean-Yves Perrier [:teoli]]

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

Comment 53

[Franziskus Kiefer [:franziskus]]

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