Implement referrer attribute for <a> and <area> tags.

Comment on attachment 8624893 [details] [diff] [review] referrer attribute for a and anchor tags, src Review of attachment 8624893 [details] [diff] [review]: ----------------------------------------------------------------- Alright, there are a few things we need to clean up before we can pass this on to some of the peers. Please address my concerns and flag me again. Next time should not take as long as the first iteration. ::: browser/base/content/browser.js @@ +5595,5 @@ > > + // get document wide referrer policy > + let referrerPolicyCode = doc.referrerPolicy; > + // get referrer attribute from clicked link and parse it > + // (only if enabled in preferences) I think it would be great to have a little more documentation here, maybe add a block mentioning that if per element referrer is enabled, then the element referrer overrules the document wide referrer. @@ +5602,5 @@ > + let referrerPolicyUtils = Cc["@mozilla.org/netwerk/referrer-policy-utils;1"]. > + createInstance(Ci.nsIReferrerPolicyUtils); > + let referrer = {}; > + if (referrerString && > + referrerPolicyUtils.parseAttributePolicyString(referrerString, referrer)){ I think you don't need to use the if-clause here once you have changed the signature in the *.idl. @@ +5608,5 @@ > + referrerPolicyCode = referrer.value; > + } > + } > + } > + ideally you would have something like this: // Allow per element referrer to overrule the document wide referrer if enabled. let linkReferrerPolicy = doc.referrerPolicy; if (Preferences.get("network.http.enablePerElementReferrer", false)) { let referrerPolicyUtils = Cc["@mozilla.org/netwerk/referrer-policy-utils;1"]. createInstance(Ci.nsIReferrerPolicyUtils); let elementReferrer = referrerPolicyUtils.parseAttributePolicyString(linkNode.getAttribute("referrer")); if (referrer !== Ci.nsIHttpChannel.REFERRER_POLICY_NOT_SET) { linkReferrerPolicy = elementReferrer; } } ::: browser/base/content/content.js @@ +117,5 @@ > + if (referrer.value !== Components.interfaces.nsIHttpChannel.REFERRER_POLICY_NOT_SET) { > + referrerPolicy = referrer.value; > + } > + } > + } Use the same as in browser.js and make sure you don't have any trailing whitespace anywhere. ::: docshell/base/nsDocShell.cpp @@ +13521,5 @@ > nsCOMPtr<nsIURI> referer = refererDoc->GetDocumentURI(); > uint32_t refererPolicy = refererDoc->GetReferrerPolicy(); > > + // get referrer attribute from clicked link and parse it > + // (only if enabled in preferences) you could use more or less the same documentation before any of those blocks where you overrule the document wide referrer. ::: docshell/base/nsDocShell.h @@ +33,5 @@ > #include "nsAutoPtr.h" > #include "nsThreadUtils.h" > #include "nsContentUtils.h" > #include "TimelineMarker.h" > +#include "mozilla/net/ReferrerPolicy.h" no need for the inclusion here, right? ::: dom/interfaces/html/nsIDOMHTMLAnchorElement.idl @@ +27,5 @@ > > attribute DOMString rel; > attribute DOMString hreflang; > attribute DOMString type; > + attribute DOMString referrer; you have to generate a new uuid whenever you add an attribute. ::: dom/interfaces/html/nsIDOMHTMLAreaElement.idl @@ +26,5 @@ > attribute DOMString target; > > attribute DOMString ping; > attribute DOMString download; > + attribute DOMString referrer; same here, you need a new uuid. ::: netwerk/base/ReferrerPolicy.cpp @@ +8,5 @@ > + > +ReferrerPolicyUtils::ReferrerPolicyUtils() > +{ > + // empty > +} remove the empty, but have a blank link underneath the closing paren. @@ +11,5 @@ > + // empty > +} > +ReferrerPolicyUtils::~ReferrerPolicyUtils() > +{ > + // empty same here. @@ +22,5 @@ > +{ > + *policyEnum = (uint32_t)mozilla::net::AttributeReferrerPolicyFromString(policyString); > + *_retval = true; > + > + return NS_OK; once you have changed your *.idl this code will also become way cleaner. ::: netwerk/base/ReferrerPolicy.h @@ +107,5 @@ > + if (IsValidAttributeReferrerPolicy(content)) { > + return ReferrerPolicyFromString(content); > + } else { > + return RP_No_Referrer; > + } you can remove the else here, just use return RP_No_Referrer; ::: netwerk/base/nsIReferrerPolicyUtils.idl @@ +25,5 @@ > + * @return > + * true > + */ > + bool parseAttributePolicyString(in AString aPolicyString, > + out unsigned long aPolicyEnum); I think it would be great to have: unsigned long parseAttributePolicyString(in AString aPolicyString); no need for a return that is always true :-)

Comment on attachment 8624896 [details] [diff] [review] referrer attribute for a and anchor tags, tests Review of attachment 8624896 [details] [diff] [review]: ----------------------------------------------------------------- Test coverage itself looks good, but we should remove all of the duplicate code and simplify those tests. Please address my concerns and re-flag me. ::: dom/base/test/anchor_area_referrer_testserver.sjs @@ +1,1 @@ > +var BASE_URL = 'example.com/tests/dom/base/test/anchor_area_referrer_testserver.sjs'; You could add a nice little description on top of the sjs file as well, so we know it belongs to what bug and what it does. @@ +78,5 @@ > + > + '</script>\n\ > + </body>\n\ > + </html>'; > +} Seems like createTestPage and createAreaTestPage share a lot of the same code, no? Can we refactor that please. (Nit: Please also eliminate trailing whitespace). @@ +139,5 @@ > + > + '</script>\n\ > + </body>\n\ > + </html>'; > +} Also createChangingTest as well as createChangingTest2 seem to introduce a lot of duplicate code. Please refactor. @@ +213,5 @@ > + state = {}; > + setSharedState(sharedKey, JSON.stringify(state)); > + response.write(""); > + return; > + } else if (action === 'test') { I like that you use early returns, but then no need for 'else if' throught this file. @@ +256,5 @@ > + } > + > + // forward link clock to redirect URL to finish test > + if (type === 'link') { > + var loc = 'https://example.com/tests/dom/base/test/file_anchor_area_referrer_redirect.html'; you could do something like referrer_testserver.sjs?redirect and then use the queryString to identify the redirect so you don't need the file file_anchor_area_referrer_redirect.html at all. @@ +265,5 @@ > + return; > + } else if (action === 'get-test-results') { > + // ?action=get-result > + response.setHeader('Cache-Control', 'no-cache', false); > + response.setHeader('Content-Type', 'text/plain', false); You should do response.setHeader only once; no need to do that within every if-else block. @@ +276,5 @@ > + var referrerPolicy = unescape(params[1].split('=')[1]); > + var name = unescape(params[2].split('=')[1]); > + var metaPolicy = params[3].split('=')[1]; > + > + response.write(createTest(metaPolicy, referrerPolicy, name)); the only thing that's really changing for every test if the response, right? If so, then please unescape all variables at the top and then just create the different tests within a switch statement. That should simplify this code drastically. ::: dom/base/test/file_anchor_area_referrer_redirect.html @@ +6,5 @@ > + </script> > +</head> > +<body> > +</body> > +</html> See suggestion in *_testserver.sjs and remove that file from. ::: dom/base/test/mochitest.ini @@ +237,5 @@ > referrerHelper.js > test_performance_user_timing.js > img_referrer_testserver.sjs > + anchor_area_referrer_testserver.sjs > + file_anchor_area_referrer_redirect.html Nit: no need for the file_ prefix, please update. I know we did that a while ago - no need anymore.

Comment on attachment 8627746 [details] [diff] [review] referrer attribute for a and anchor tags, src Review of attachment 8627746 [details] [diff] [review]: ----------------------------------------------------------------- Alrighty, getting there, one more time and we can hand it off to a peer for review. ::: browser/base/content/browser.js @@ +5587,5 @@ > catch (e) { } > } > > + // get document wide referrer policy > + let referrerPolicyCode = doc.referrerPolicy; you could use refPolEnum like you do in the js files - either way, please make it consistent throughout the patches. @@ +5590,5 @@ > + // get document wide referrer policy > + let referrerPolicyCode = doc.referrerPolicy; > + // get referrer attribute from clicked link and parse it > + // if per element referrer is enabled, the element referrer overrules > + // the document wide referrer Again, you could simplify the comment to: // Allow per element referrer to overrule the document wide referrer if enabled and have it before you write any code, kind of a block statement. @@ +5598,5 @@ > + let referrerAttribute = referrerPolicyUtils. > + parseAttributePolicyString(linkNode. > + getAttribute("referrer")); > + if (referrerAttribute !== > + Components.interfaces.nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE) { Replace Components.interfaces. with Ci. ::: browser/base/content/content.js @@ +116,5 @@ > + let referrerAttribute = referrerPolicyUtils. > + parseAttributePolicyString(event.target. > + getAttribute("referrer")); > + if (referrerAttribute !== > + Components.interfaces.nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE) { Again replace Components.interfaces with Ci. ::: netwerk/base/ReferrerPolicy.cpp @@ +14,5 @@ > +{ > +} > + > +NS_IMETHODIMP > +ReferrerPolicyUtils::ParseAttributePolicyString(const nsAString & policyString, const nsAString& aPolicyString @@ +15,5 @@ > +} > + > +NS_IMETHODIMP > +ReferrerPolicyUtils::ParseAttributePolicyString(const nsAString & policyString, > + uint32_t *policyEnum) uint32_t* outPolicyEnum @@ +18,5 @@ > +ReferrerPolicyUtils::ParseAttributePolicyString(const nsAString & policyString, > + uint32_t *policyEnum) > +{ > + *policyEnum = (uint32_t)mozilla::net::AttributeReferrerPolicyFromString(policyString); > + nit: remove empty line. ::: netwerk/base/ReferrerPolicy.h @@ +94,5 @@ > } > > + > +inline bool > +IsValidAttributeReferrerPolicy(const nsAString& content) const nsAString& aContent @@ +97,5 @@ > +inline bool > +IsValidAttributeReferrerPolicy(const nsAString& content) > +{ > + // Spec allows only these three policies at the moment > + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1178337 Nit: just write: see bug 1178337 no need for the full blown url @@ +104,5 @@ > + || content.LowerCaseEqualsLiteral("unsafe-url"); > +} > + > +inline ReferrerPolicy > +AttributeReferrerPolicyFromString(const nsAString& content) const nsAString& aContent @@ +114,5 @@ > + } > + // if the referrer attribute string is empty, return RP_Unset > + if (content.IsEmpty()) { > + return RP_Unset; > + } Flip it around, do if (content.IsEmpty()) { return RP_Uneset; } if (IsValidAttributeReferrerPolicy... that way you only have to do the empty check once.

Comment on attachment 8627747 [details] [diff] [review] referrer attribute for a and anchor tags, tests Review of attachment 8627747 [details] [diff] [review]: ----------------------------------------------------------------- Getting there - one more time and we are ready to hand this off to a peer reviewer as well. ::: dom/base/test/anchor_area_referrer_testserver.sjs @@ +22,5 @@ > + > +function buildMetaString(aMetaPolicy) { > + if (aMetaPolicy) { > + return '<meta name="referrer" content="' + aMetaPolicy + '">'; > + } else { no need for the else. @@ +99,5 @@ > + > + '</script>\n\ > + </body>\n\ > + </html>'; > +} Looks better, but please rename the function to something more descriptive, e.g. for the first one: * createTestPageUsingReffer and the second to * createTestPageUsingSetAttributeReferrer or something similar, using 1 and 2 is not that great. @@ +124,5 @@ > +} > + > +// Creates the following test cases for the specified referrer > +// policy: > +// <a> with changing referrer in JS I like those comments, way cleaner now. @@ +135,5 @@ > + > +// Creates the following test cases for the specified referrer > +// policy: > +// <area> with referrer > +function createAreaTest(aMetaPolicy, aReferrerPolicy, aName) { any reason to have that indirection? why not just call createAreaTestPage instead of createAreaTest on the callsite, you are just passing through the arguments, right? @@ +154,5 @@ > +// policy: > +// <area> with changing referrer in JS > +function createChangingAreaTest2(aMetaPolicy, aReferrerPolicy, aNewReferrerPolicy, aName) { > + var _areaString = buildAreaString(aMetaPolicy, aReferrerPolicy, aName); > + var _metaString = buildMetaString(aMetaPolicy); why not even even factor our _areaString and _metaString? You could do that at the callsite and then just call createChangingPage, no? @@ +233,5 @@ > + response.write(getSharedState(sharedKey)); > + return; > + } > + response.setHeader('Cache-Control', 'no-cache', false); > + response.setHeader('Content-Type', 'text/html; charset=utf-8', false); move that to the very top of handleRequest to avoid unexpected behavior. ::: dom/base/test/test_anchor_area_referrer.html @@ +36,5 @@ > + } > +}); > + > +/** > + * helper to perform an XHR. please expand the comment to what the XHR is used for.

Comment on attachment 8628407 [details] [diff] [review] referrer attribute for a and anchor tags, src Review of attachment 8628407 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, please fix my nits and then we can hand it off to a peer for review. ::: browser/base/content/browser.js @@ +5596,5 @@ > + let referrerAttribute = referrerPolicyUtils. > + parseAttributePolicyString(linkNode. > + getAttribute("referrer")); > + if (referrerAttribute !== > + Ci.nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE) { nit: if you rename the variable to referrerAttr then this line would fit into a 80 char line (hopefully :-) ). ::: browser/base/content/content.js @@ +118,5 @@ > + getAttribute("referrer")); > + if (referrerAttribute !== > + Ci.nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE) { > + referrerPolicyEnum = referrerAttribute; > + } same here, would be great to have that if statement within one line. ::: dom/interfaces/html/nsIDOMHTMLAnchorElement.idl @@ +27,5 @@ > > attribute DOMString rel; > attribute DOMString hreflang; > attribute DOMString type; > + attribute DOMString referrer; As agreed, please remove from the *.idl and only have referrer within the webidl. ::: dom/interfaces/html/nsIDOMHTMLAreaElement.idl @@ +31,1 @@ > same here, please remove. ::: netwerk/base/ReferrerPolicy.h @@ +101,5 @@ > + // See bug 1178337 > + return aContent.LowerCaseEqualsLiteral("no-referrer") > + || aContent.LowerCaseEqualsLiteral("origin") > + || aContent.LowerCaseEqualsLiteral("unsafe-url"); > +} We should have introduced const char* for all of the different policies and use the constants throughout the file instead of using the string literals everywhere. Bonus points if you wanna incorporate that and also update ReferrerPolicyFromString as well as IsValidReferrerPolicy. Up to you though!

Comment on attachment 8628408 [details] [diff] [review] referrer attribute for a and anchor tags, tests Review of attachment 8628408 [details] [diff] [review]: ----------------------------------------------------------------- Ok, fix my nits and I am fine with accepting those tests. ::: dom/base/test/anchor_area_referrer_testserver.sjs @@ +108,5 @@ > + > +// creating anchor test case using referrer attribute > +function createTestPage(aMetaPolicy, aReferrerPolicy, aName) { > + var _anchorString = buildAnchorString(aMetaPolicy, aReferrerPolicy, aName); > + return createTestPageUsingReffer(aMetaPolicy, _anchorString); 'createTestPageUsingReffer' is that a typo or intentional? @@ +142,5 @@ > + return createChangingPageAttribute(aMetaPolicy, _areaString, aNewReferrerPolicy); > +} > + > +function handleRequest(request, response) { > + var sharedKey = 'anchor_area_referrer_testserver.sjs'; Also as in the other tests, you could move the constant at the top of the file and define as 'const' instead of var. @@ +216,5 @@ > + response.setHeader('Cache-Control', 'no-cache', false); > + response.setHeader('Content-Type', 'text/plain', false); > + response.write(getSharedState(sharedKey)); > + return; > + } Nit: same as in the other testcase, you could move that if further up - easy cases first, more complicated cases last :-)

Comment on attachment 8630186 [details] [diff] [review] referrer attribute for a and anchor tags, browser tests Review of attachment 8630186 [details] [diff] [review]: ----------------------------------------------------------------- r=me, fix my nits and please only land if this is not causing more intermittent failures on Treeherder. ::: browser/base/content/test/referrer/head.js @@ +41,5 @@ > + // toScheme: "http://", > + // policy: "origin", > + // rel: "noreferrer", > + // result: "" // rel=noreferrer trumps referrer policy > + // }, Why is this commented out? If not used, then please remove. @@ +288,5 @@ > + META_REFERRER = false; > + referrerTestCaseLoaded(0).then(function() { > + aStartTestCase(0); > + }); > + }else { nit: missing space between { and else.

Comment on attachment 8631272 [details] [diff] [review] referrer attribute for a and anchor tags, tests I won't be able to get to this on a timely basis. Please pick another reviewer.

Comment on attachment 8631273 [details] [diff] [review] referrer attribute for a and anchor tags, src >+ if (aContent->HasAttr(kNameSpaceID_None, nsGkAtoms::referrer)) { >+ nsAutoString referrerPolicy; >+ aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::referrer, referrerPolicy); How about: nsAutoString referrerPolicy; if (aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::referrer, referrerPolicy)) { ... } It's a little weird to store the parsed enum version but then convert it to string here just so you can reparse it from a string again. Is there a reason this code is not working with the parsed attr value directly? The DOM and docshell code look good modulo that. >+++ b/layout/build/nsLayoutModule.cpp Adding a necko class to nsLayoutModule looks wrong to me. Please pick where this referrer policy utils thing lives. That said, it's a bit odd to use createInstance for it (since it has no per-instance state), and it's not obvious to me that we really want to create a new interface/contract/etc for it as opposed to adding it to one of the various utils things we already have... Do we expect to grow more methods on this interface? For the rest, I'm not a peer for the relevant code, either browser or necko, so you will want to find reviewers from those modules.

Gijs can you review the browser and javascript bits and Steve the necko?

(In reply to Boris Zbarsky [:bz] from comment #20) > nsAutoString referrerPolicy; > if (aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::referrer, > referrerPolicy)) { > ... > } > done that. > It's a little weird to store the parsed enum version but then convert it to > string here just so you can reparse it from a string again. Is there a > reason this code is not working with the parsed attr value directly? we store the referrer policy string and only parse to the enum when used. > this referrer policy utils thing lives. That said, it's a bit odd to use > createInstance for it (since it has no per-instance state), and it's not > obvious to me that we really want to create a new interface/contract/etc for > it as opposed to adding it to one of the various utils things we already > have... Do we expect to grow more methods on this interface? moved this into nsINetUtil.idl

Comment on attachment 8631829 [details] [diff] [review] referrer attribute for a and anchor tags, src Review of attachment 8631829 [details] [diff] [review]: ----------------------------------------------------------------- Just a few nits. r=me for the necko code with those nits fixed. Thanks! ::: netwerk/base/ReferrerPolicy.h @@ +31,5 @@ > RP_Unset = nsIHttpChannel::REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE > }; > > +/* spec tokens: never no-referrer */ > +const char RPS_Never[] = "never"; kRPS_Never or RPS_NEVER ('k' prefix or ALL CAPS for constants). ::: netwerk/base/nsIOService.cpp @@ +1693,5 @@ > return NS_OK; > } > > +NS_IMETHODIMP > +nsIOService::ParseAttributePolicyString(const nsAString& policyString, Please add a one line comment above this function.. /* nsINetUtil.parseAttributePolicyString */ .. to indicate that it's an IDL method. @@ +1696,5 @@ > +NS_IMETHODIMP > +nsIOService::ParseAttributePolicyString(const nsAString& policyString, > + uint32_t *outPolicyEnum) > +{ > + *outPolicyEnum = (uint32_t)mozilla::net::AttributeReferrerPolicyFromString(policyString); Null check please.

> we store the referrer policy string and only parse to the enum when used. Is that true? That's not what I see in the nsGenericHTMLElement::ParseReferrerAttribute code that was checked in for bug 1166910.

Comment on attachment 8631829 [details] [diff] [review] referrer attribute for a and anchor tags, src Review of attachment 8631829 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +5590,5 @@ > > + // first get document wide referrer policy, then > + // get referrer attribute from clicked link and parse it and > + // allow per element referrer to overrule the document wide referrer if enabled > + let referrerPolicyEnum = doc.referrerPolicy; Nit: I don't really understand why you added "Enum" here. I think just calling it referrerPolicy is clear enough, that's also the name of the param property we eventually pass... @@ +5591,5 @@ > + // first get document wide referrer policy, then > + // get referrer attribute from clicked link and parse it and > + // allow per element referrer to overrule the document wide referrer if enabled > + let referrerPolicyEnum = doc.referrerPolicy; > + if (Preferences.get("network.http.enablePerElementReferrer", false)) { Don't use Preferences.jsm, use: Services.prefs.getBoolPref("network.http.enablePerElementReferrer") instead. Ditto for content.js, where it'll save you the import. @@ +5593,5 @@ > + // allow per element referrer to overrule the document wide referrer if enabled > + let referrerPolicyEnum = doc.referrerPolicy; > + if (Preferences.get("network.http.enablePerElementReferrer", false)) { > + let referrerPolicyUtils = Cc["@mozilla.org/netwerk/referrer-policy-utils;1"]. > + createInstance(Ci.nsIReferrerPolicyUtils); This code doesn't make sense to me. Several issues: - createInstance. This looks like essentially a "static" type method that we're using, so I am confused why we'd need a new thing every time we call this. - I can't find this interface in the tree, but I see that this patch implements this method in nsIOService.cpp, and defines the interface on nsINetUtil. So either the method is duplicated (in which case: why?) or this code should be using other contract and interface IDs (in which case: did you test this, and how did it pass?). Either way, I'm confused. :-) It seems like you could just use Services.io, as long as you ensure that that works and it doesn't need further QI'ing in order for that to work (and if it does, that should probably be fixed inside Services.jsm). @@ +5595,5 @@ > + if (Preferences.get("network.http.enablePerElementReferrer", false)) { > + let referrerPolicyUtils = Cc["@mozilla.org/netwerk/referrer-policy-utils;1"]. > + createInstance(Ci.nsIReferrerPolicyUtils); > + let referrerAttr = referrerPolicyUtils.parseAttributePolicyString(linkNode. > + getAttribute("referrer")); The other code here nullchecks linkNode. This code probably should too. You're naming this variable "referrerAttr", but it isn't an attribute, it's the policy resulting from the attribute, so the name seems misleading. @@ +5596,5 @@ > + let referrerPolicyUtils = Cc["@mozilla.org/netwerk/referrer-policy-utils;1"]. > + createInstance(Ci.nsIReferrerPolicyUtils); > + let referrerAttr = referrerPolicyUtils.parseAttributePolicyString(linkNode. > + getAttribute("referrer")); > + if (referrerAttr !== Ci.nsIHttpChannel.REFERRER_POLICY_DEFAULT) { Not a criticism per se, but just a note that you don't strictly need the strict equals here - the constant is guaranteed to be a number, as is the result of parseAttributePolicyString, so != will not need to convert anything and will return the same result. I don't mind much, but most of the code in browser/ uses ==/!= rather than the strict variant. The variable name makes this a little more confusing because attribute values are of course typically strings... ::: browser/base/content/content.js @@ +101,5 @@ > let docLocation = doc.location.href; > let charSet = doc.characterSet; > let baseURI = doc.baseURI; > let referrer = doc.referrer; > + let referrerPolicyEnum = doc.referrerPolicy; Likewise, I would prefer not changing the name of this variable. @@ +108,5 @@ > .outerWindowID; > > + // get referrer attribute from clicked link and parse it > + // if per element referrer is enabled, the element referrer overrules > + // the document wide referrer The code in this file has the same issues as the code in browser.js

Comment on attachment 8631831 [details] [diff] [review] referrer attribute for a and area tags, tests Review of attachment 8631831 [details] [diff] [review]: ----------------------------------------------------------------- I'm very glad that we're adding tests for this, but as it is they are quite hard to read and therefore quite hard to review. Could you: - use `template strings with ${variables}`. They can be multiline, save you concatenation work, and don't otherwise conflict with quotes. - abstract the referrer property vs. setAttribute thing into a query argument like "referrerSettingMethod" or whatever, instead of "test" vs. "test2", which will also allow you to de-duplicate all those functions generating various types of tests. - avoid some of the repetition by using loops. It seems you're testing a number of combinations of a number of variables where each has 2-3 options, so nested loops over the different items would simplify understanding that and checking we've covered all our bases, and that the results are logical. - avoid some of the indenting by factoring out functions arguments into named inner functions: instead of: doXHR('/tests/dom/base/test/anchor_area_referrer_testserver.sjs?action=get-test-results', function(xhr) { <snip 10+ lines> } just: let onload = xhr => { <snip 10+ lines> }; doXHR(..., onload, onerror); ::: dom/base/test/anchor_area_referrer_testserver.sjs @@ +84,5 @@ > + </html>'; > +} > + > +// test page using new referrer attribute set using .referrer = > +function createChangingPageAttribute(aMetaPolicy, aElementString, aNewReferrerPolicy) { This name is confusing. @@ +143,5 @@ > + return createChangingPageAttribute(aMetaPolicy, _areaString, aNewReferrerPolicy); > +} > + > +function handleRequest(request, response) { > + var params = request.queryString.split('&'); Nit: params = new URLSearchParams(request.queryString); will get you a key/value store, instead of all this order-dependent parsing... ::: dom/base/test/test_anchor_area_referrer.html @@ +99,5 @@ > + var sjs = "/tests/dom/base/test/anchor_area_referrer_testserver.sjs?action=generate-anchor-policy-test"; > + > + // setting anchor unsafe-url and meta origin - unsafe-url shall prevail > + yield resetState(); > + var name = 'unsaf-url-with-origin-in-meta'; Nit: typo. @@ +100,5 @@ > + > + // setting anchor unsafe-url and meta origin - unsafe-url shall prevail > + yield resetState(); > + var name = 'unsaf-url-with-origin-in-meta'; > + yield iframe.src = sjs + "&referrerPolicy=" + escape('unsafe-url') + "&name=" + name + "&policy=" + escape('origin'); Nit: Using URLSearchParams would save you from all of this. You don't technically need to escape these literals, and you're not escaping the variables like "name" (though I guess they're on the previous line and also hardcoded...) Further nit: please call the referrerPolicy and policy arguments something that distinguishes them. I'm assuming you want something like "elementPolicy" vs. "documentPolicy" or something - but right now I actually don't know which is which, and it's hard to read that in the server code as well, because you're reading this out by order rather than by variable name... @@ +105,5 @@ > + yield checkIndividualResults("unsafe-url (anchor) with origin in meta", ["full"], [name+'unsafe-url']); > + > + // setting anchor unsafe-url and meta origin - origin shall prevail > + yield resetState(); > + var name = 'origin-with-unsafe-url-in-meta'; You're redeclaring this a lot.

should address everything. Using nsIReferrerPolicyUtils was me forgetting to change this to nsINetUtil in the last patch, sorry for the confusion.

simplified tests

I'd still like to understand why elements parse the string to an enum and then consumers ask for it to be serialized before reparsing it into a (different? same?) enum...

Does this need an intent to implement?

Comment on attachment 8632196 [details] [diff] [review] referrer attribute for a and area tags, src Review of attachment 8632196 [details] [diff] [review]: ----------------------------------------------------------------- Almost r+ (on the JS bits, to be clear), but see below for notes, and I would like to see bz's question answered, too. ::: browser/base/content/browser.js @@ +5603,5 @@ > + let referrerPolicy = doc.referrerPolicy; > + if (Services.prefs.getBoolPref("network.http.enablePerElementReferrer") && > + linkNode) { > + let netUtils = Cc["@mozilla.org/network/util;1"]. > + getService(Components.interfaces.nsINetUtil); Still wondering why we can't use Services.io here. Did you check? ::: browser/base/content/content.js @@ +108,5 @@ > + // get referrer attribute from clicked link and parse it > + // if per element referrer is enabled, the element referrer overrules > + // the document wide referrer > + if (Services.prefs.getBoolPref("network.http.enablePerElementReferrer") && > + event && event.target) { event or event.target can't be null here (see also the unchecked use of it later on. :-) )

Comment on attachment 8632315 [details] [diff] [review] referrer attribute for a and area tags, tests Review of attachment 8632315 [details] [diff] [review]: ----------------------------------------------------------------- Better, but there is a lot that could be better about this JS... ::: dom/base/test/anchor_area_referrer_testserver.sjs @@ +50,5 @@ > + var metaString = buildMetaString(aMetaPolicy); > + if (aChangingMethod === 'setAttribute') { > + var changeString = `document.getElementById("link").setAttribute("referrer", "${aNewAttributePolicy}")`; > + } else if (aChangingMethod === 'attribute') { > + var changeString = `document.getElementById("link").referrer = "${aNewAttributePolicy}"`; This is a property, not an attribute. Please disambiguate this (ie aChangingMethod should be 'property'), so that it's clear what the difference between 'setAttribute' and the other thing is. Nit: you're redeclaring changeString here. @@ +79,5 @@ > + > + if (action === 'resetState') { > + var state = getSharedState(SHARED_KEY); > + state = {}; > + setSharedState(SHARED_KEY, JSON.stringify(state)); Instead of these three lines, setSharedState(SHARED_KEY, "{}") would be sufficient. @@ +105,5 @@ > + if (result === '') { > + result = {}; > + } else { > + result = JSON.parse(result); > + } Could replace this entire block with: result = result ? JSON.parse(result) : {}; @@ +108,5 @@ > + result = JSON.parse(result); > + } > + > + if (!result["tests"]) { > + result["tests"] = {}; We never use any other keys in result, so the extra level of nesting seems superfluous. @@ +111,5 @@ > + if (!result["tests"]) { > + result["tests"] = {}; > + } > + > + if (name === "setAttribute") { I don't think this is ever true, see my comment in the test file. @@ +117,5 @@ > + } else { // get request's referer and store it > + var referrerLevel = "none"; > + var test = {} > + if (request.hasHeader('Referer')) { > + let referrer = request.getHeader('Referer'); Nit: double quotes. (everywhere except for strings that have them inside requiring single quotes to go around them, really) Also: you're mixing var and let. Front-end code sticks to either of the two, for clarity's sake. It'd probably be good to do the same here. @@ +120,5 @@ > + if (request.hasHeader('Referer')) { > + let referrer = request.getHeader('Referer'); > + if (referrer.indexOf("anchor_area_referrer_testserver") > 0) { > + referrerLevel = "full"; > + } else if (referrer == "http://mochi.test:8888") { This is going to break if we ever change whether we add/remove the trailing slash. Maybe it should be more robust? Also, this will mean the referrerLevel will be "none" if the referrer does not match either criteria. That seems wrong? Seems like it should only be "none" if we got no referrer. @@ +123,5 @@ > + referrerLevel = "full"; > + } else if (referrer == "http://mochi.test:8888") { > + referrerLevel = "origin"; > + } > + test.referrer = request.getHeader('Referer'); Something funny is going on with the indentation here. Also, why not reuse the "referrer" variable from earlier? @@ +135,5 @@ > + > + setSharedState(SHARED_KEY, JSON.stringify(result)); > + } > + > + // forward link clock to redirect URL to finish test link clock? @@ +137,5 @@ > + } > + > + // forward link clock to redirect URL to finish test > + if (type === 'link') { > + var loc = 'http://'+BASE_URL+'?action=redirect'; Nit: spaces around operators, double quotes please. (my apologies for the fact that we don't have a formatting thing for JS that could do this kind of thing for you, or flag it before review) @@ +160,5 @@ > + if (params.has(META_POLICY)) { > + var metaPolicy = params.get(META_POLICY); > + } else { > + var metaPolicy = ''; > + } All three of these blocks redeclare variables. For all of them: var foo = params.get(FOO) || ''; would work. @@ +189,5 @@ > + response.write(createTestPageUsingRefferer(metaPolicy, _areaString, newAttributePolicy, 'setAttribute')); > + return; > + } > + if (action === 'generate-area-changing-policy-test-attribute') { > + var _areaString = buildAreaString(metaPolicy, attributePolicy, name); There's a lot of repetition here. It seems like the: 1) type of element (area vs. anchor) 2) type of setter (nothing, attribute, property) could be parameters, which would simplify this code further. ::: dom/base/test/test_anchor_area_referrer.html @@ +27,5 @@ > +const ACTION = 'action'; > +const TESTS = 'tests'; > + > +const testCases = [ > + {ACTION: ["generate-anchor-policy-test", "generate-area-policy-test"], All of these are the same everywhere except the "anchor" and "area" difference, which is another smell that that could just be a param. @@ +73,5 @@ > + {ATTRIBUTE_POLICY: 'no-referrer', > + NEW_ATTRIBUTE_POLICY: 'unsafe-url', > + NAME: 'unsaf-url-no-referrer-with-origin-in-meta', > + META_POLICY: 'origin', > + DESC: "unsafe-url (anchor, orginally no-referrer) with origin in meta", Is there a particular reason we don't test more permutations for the property and setAttribute variants, while we test so many for the basic version? @@ +78,5 @@ > + RESULT: 'full'}]} > +]; > + > +SimpleTest.waitForExplicitFinish(); > +var advance = function() { tests.next(); }; The typical way we use a generator with async things is using promises (that get resolved when an async task is done) and Task.jsm. What you've done works, but it disconnects the thing that calls back into the main "yield loop" from the thing that is yielding (e.g. you do an iframe load, and the load handler is somewhere in the HTML and just blanket calls .next() even for iframe loads). That seems fragile. I feel bad asking you to rewrite this test still more, but please consider it next time. @@ +80,5 @@ > + > +SimpleTest.waitForExplicitFinish(); > +var advance = function() { tests.next(); }; > + > +var mTestResult; Nit: "m" means member, and this is a global, so you want "g" as a prefix. @@ +120,5 @@ > + */ > +function checkIndividualResults(aTestname, aExpectedReferrer, aName) { > + let onload = xhr => { > + var results = xhr.response; > + info(JSON.stringify(xhr.response)); Is xhr.responseText null because of the responseType or something? Could just use that if not... @@ +121,5 @@ > +function checkIndividualResults(aTestname, aExpectedReferrer, aName) { > + let onload = xhr => { > + var results = xhr.response; > + info(JSON.stringify(xhr.response)); > + if (aName === 'setAttribute') { This is never true, AFAICT, because aName comes from tests[i].NAME, which comes from the array at the top of this file, where there is no property NAME that has the value 'setAttribute'. @@ +180,5 @@ > + }; > + }; > + }; > + > + // complete. Be sure to yield so we don't call this twice. Why does yielding stop us from calling this twice?

(In reply to :Gijs Kruitbosch from comment #32) > Comment on attachment 8632196 [details] [diff] [review] > referrer attribute for a and area tags, src > > Review of attachment 8632196 [details] [diff] [review]: > ----------------------------------------------------------------- > > Almost r+ (on the JS bits, to be clear), but see below for notes, and I > would like to see bz's question answered, too. > is done in a separate patch > ::: browser/base/content/browser.js > @@ +5603,5 @@ > > + let referrerPolicy = doc.referrerPolicy; > > + if (Services.prefs.getBoolPref("network.http.enablePerElementReferrer") && > > + linkNode) { > > + let netUtils = Cc["@mozilla.org/network/util;1"]. > > + getService(Components.interfaces.nsINetUtil); > > Still wondering why we can't use Services.io here. Did you check? > ok, moved it in there

(In reply to :Gijs Kruitbosch from comment #33) > Comment on attachment 8632315 [details] [diff] [review] > referrer attribute for a and area tags, tests > > Review of attachment 8632315 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +108,5 @@ > > + result = JSON.parse(result); > > + } > > + > > + if (!result["tests"]) { > > + result["tests"] = {}; > > We never use any other keys in result, so the extra level of nesting seems > superfluous. > all of this (including setAttribute) has been leftover from an older version of this test -> removed all of it. > @@ +120,5 @@ > > + if (request.hasHeader('Referer')) { > > + let referrer = request.getHeader('Referer'); > > + if (referrer.indexOf("anchor_area_referrer_testserver") > 0) { > > + referrerLevel = "full"; > > + } else if (referrer == "http://mochi.test:8888") { > > This is going to break if we ever change whether we add/remove the trailing > slash. Maybe it should be more robust? > > Also, this will mean the referrerLevel will be "none" if the referrer does > not match either criteria. That seems wrong? Seems like it should only be > "none" if we got no referrer. > added another case, note however that this new case should never happen as this would mean the referrer is origin but different from the actual one. > @@ +73,5 @@ > > + {ATTRIBUTE_POLICY: 'no-referrer', > > + NEW_ATTRIBUTE_POLICY: 'unsafe-url', > > + NAME: 'unsaf-url-no-referrer-with-origin-in-meta', > > + META_POLICY: 'origin', > > + DESC: "unsafe-url (anchor, orginally no-referrer) with origin in meta", > > Is there a particular reason we don't test more permutations for the > property and setAttribute variants, while we test so many for the basic > version? > If you think we should test more, I'm happy to add more permutations. However, this new code only sets a different referrer policy coming from the referrer attribute (that's what we're testing). The entire handling of the referrer policy is the same as before, i.e. tested there (bug 704320). > @@ +78,5 @@ > > + RESULT: 'full'}]} > > +]; > > + > > +SimpleTest.waitForExplicitFinish(); > > +var advance = function() { tests.next(); }; > > The typical way we use a generator with async things is using promises (that > get resolved when an async task is done) and Task.jsm. > > What you've done works, but it disconnects the thing that calls back into > the main "yield loop" from the thing that is yielding (e.g. you do an iframe > load, and the load handler is somewhere in the HTML and just blanket calls > .next() even for iframe loads). That seems fragile. I feel bad asking you to > rewrite this test still more, but please consider it next time. > well, I can't find any other test in dom/base doing this... maybe because these are plain mochitests, don't I need some sort of chrome for that (i.e. browser tests)? Since we should test this for all platforms I think we have to use plain mochitest, right? > @@ +120,5 @@ > > + */ > > +function checkIndividualResults(aTestname, aExpectedReferrer, aName) { > > + let onload = xhr => { > > + var results = xhr.response; > > + info(JSON.stringify(xhr.response)); > > Is xhr.responseText null because of the responseType or something? Could > just use that if not... > yep, responseText is null if contentType is json.

(In reply to Boris Zbarsky [:bz] from comment #30) > I'd still like to understand why elements parse the string to an enum and > then consumers ask for it to be serialized before reparsing it into a > (different? same?) enum... modified parsing of the attribute according to your suggestion (also simplifies some of the code). Code for bug 1175736 will be adapted accordingly.

r+ carries over

(In reply to :Ms2ger from comment #31) > Does this need an intent to implement? will be sent out soon

Please don't use Task.jsm in tests for anything exposed to content; that makes it even more painful to share the tests than it already is.

Comment on attachment 8633776 [details] [diff] [review] referrer attribute for a and area tags, src Review of attachment 8633776 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the JS changes, with the change below. ::: browser/base/content/content.js @@ +367,5 @@ > + // the document wide referrer > + let referrerPolicy = ownerDoc.referrerPolicy; > + if (Services.prefs.getBoolPref("network.http.enablePerElementReferrer")) { > + let referrerAttrValue = Services.netUtils.parseAttributePolicyString(event.target. > + getAttribute("referrer")); Reading the context here, I think you want node.getAttribute, and you'll want to nullcheck node along with the pref. I also noticed that we set noReferrer if the link has rel="noreferrer". How does that play with this attribute? Should we add a test for it? :-)

Comment on attachment 8633781 [details] [diff] [review] move referrer parsing to Element (r=bz) >+++ b/docshell/base/nsDocShell.cpp >+ uint32_t refPolEnum = ((Element*)aContent)->GetReferrerPolicy(); At least use aContent->AsElement(). But also, unless something is already asserting IsElement() somewhere in here, might be good to check before jumping. >+ if (refPolEnum != mozilla::net::RP_Unset) { This whole file is "using namespace mozilla", so no need for that part. >+++ b/dom/base/Element.h >+ if (referrerValue) { >+ nsAutoString referrerPolicy; >+ referrerValue->ToString(referrerPolicy); >+ return mozilla::net::ReferrerPolicyFromString(referrerPolicy); So, this is the part I just don't understand. ;) For an HTML element, the referrer attribute is parsed as an enumerated attribute (per nsGenericHTMLElement::ParseReferrerAttribute), right? Why? Its set of recognized enum values seems to be a subset of the ones ReferrerPolicyFromString recognizes. Why isn't it all of them? If we're not going to use the stored enum here anyway, why are we wasting time parsing it as an enum instead of just storing as a string? Again, no need for the "mozilla::" bit. And please don't make this inline, if that involves pulling Preferences.h into Element.h. There's no reason to inline this function. >+++ b/dom/base/nsImageLoadingContent.cpp No need for "mozilla::" in this file either. >+ nsresult rv = nsContentUtils::LoadImage(aNewURI, aDocument, Please fix the indent on the following lines. > +++ b/dom/html/HTMLImageElement.h No need for "mozilla::". Also, why are you adding the cast in GetImageReferrerPolicy, exactly? >+ RP_Default = nsIHttpChannel::REFERRER_POLICY_DEFAULT, This seems like an odd change to make in this bug.... Should it have a separate bug or something? Is this a substantive change or just a typo fix?

r+ carries over

(In reply to :Gijs Kruitbosch from comment #40) > Comment on attachment 8633776 [details] [diff] [review] > referrer attribute for a and area tags, src > > Review of attachment 8633776 [details] [diff] [review]: > ----------------------------------------------------------------- > > I also noticed that we set noReferrer if the link has rel="noreferrer". How > does that play with this attribute? Should we add a test for it? :-) rebased and added test cases (rel=noreferrer and invalid values). Note however that rel overrides the policy and is handled later, i.e. is already tested for bug 704320.

(In reply to Boris Zbarsky [:bz] from comment #41) > Comment on attachment 8633781 [details] [diff] [review] > move referrer parsing to Element (r=bz) > > >+++ b/dom/base/Element.h > >+ if (referrerValue) { > >+ nsAutoString referrerPolicy; > >+ referrerValue->ToString(referrerPolicy); > >+ return mozilla::net::ReferrerPolicyFromString(referrerPolicy); > > So, this is the part I just don't understand. ;) > should be ok now, as discussed (I hope). Invalid policy values in attributes are currently ignored (in contrast to invalid policies specified in meta, which result in no-referrer). This might have to be changed according to spec when defined there. > > +++ b/dom/html/HTMLImageElement.h > > No need for "mozilla::". Also, why are you adding the cast in > GetImageReferrerPolicy, exactly? > nsContentUtils::LoadImage expects the policy value to be net::ReferrerPolicy > >+ RP_Default = nsIHttpChannel::REFERRER_POLICY_DEFAULT, > > This seems like an odd change to make in this bug.... Should it have a > separate bug or something? Is this a substantive change or just a typo fix? that's just a typo, doesn't change anything as both values (REFERRER_POLICY_DEFAULT and REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE) are both 0. Just spotted this and thought it's not worth opening another bug for this but can take it out if you prefer.

sorry that was the wrong patch, updated

> nsContentUtils::LoadImage expects the policy value to be net::ReferrerPolicy Sure. But Element::GetReferrerPolicy returns a net::ReferrerPolicy, no? > doesn't change anything as both values (REFERRER_POLICY_DEFAULT and > REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE) are both 0.

I _meant_ to say, that the enum value change is fine if they're both the same value. No need for another bug indeed.

Comment on attachment 8634396 [details] [diff] [review] referrer attribute for a and area tags, tests Review of attachment 8634396 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK bar some nits. ::: dom/base/test/anchor_area_referrer_testserver.sjs @@ +177,5 @@ > + response.write(_getAreaPage('property')); > + return; > + } > + > + response.write("I don't know action "+action); Still missing spaces around operator here. ::: dom/base/test/test_anchor_area_referrer.html @@ +31,5 @@ > +const testCases = [ > + {ACTION: ["generate-anchor-policy-test", "generate-area-policy-test"], > + TESTS: [ > + {ATTRIBUTE_POLICY: 'unsafe-url', > + NAME: 'unsaf-url-with-origin-in-meta', Nit: typo, same a few times below. @@ +221,5 @@ > + yield resetState(); > + var searchParams = new URLSearchParams(); > + searchParams.append(ACTION, actionString); > + searchParams.append(NAME, tests[i].NAME); > + if (tests[i].ATTRIBUTE_POLICY){ Nit: space before {, also below. Could also do: for (let k of ["ATTRIBUTE_POLICY", "NEW_ATTRIBUTE_POLICY", "META_POLICY", "REL"]) { if (tests[i][k]) { searchParams.append(window[k], tests[i][k]); } } but I'm not too fussed either way.

Comment on attachment 8634436 [details] [diff] [review] remove unnecessary attribute parsing (r=bz) >+ if (IsElementAnchor(aContent) && aContent->IsElement()) { OK, so _I_ can't read and _you_ need to push back on silly review comments. ;) If IsElementAnchor() returned true, aContent->IsElement() better be true. >+ uint32_t refPolEnum = aContent->AsElement()->GetReferrerPolicy(); s/uint32_t/net::ReferrerPolicy/ here. >+#include "mozilla/Preferences.h" Like I said in my previous review, please put this in Element.cpp, not Element.h. Please do make sure this happens. r=me with that fixed.

r+ carries over

r+ carries over

r+ carries over

checkin needed for https://bugzilla.mozilla.org/attachment.cgi?id=8635009 https://bugzilla.mozilla.org/attachment.cgi?id=8635010 https://bugzilla.mozilla.org/attachment.cgi?id=8635011

Link to a recent Try push? :)

r+ carries over (rebase)

r+ carries over (rebase)

r+ carries over (rebase and splitting tests due to b2g failures)

latest try run (fixed b2g test failures) https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ccd5ac9002f checkin needed for https://bugzilla.mozilla.org/attachment.cgi?id=8636174 https://bugzilla.mozilla.org/attachment.cgi?id=8636175 https://bugzilla.mozilla.org/attachment.cgi?id=8636176

https://hg.mozilla.org/integration/mozilla-inbound/rev/c309c6f04d2e https://hg.mozilla.org/integration/mozilla-inbound/rev/c55086289922 https://hg.mozilla.org/integration/mozilla-inbound/rev/339a34e9a135

https://hg.mozilla.org/mozilla-central/rev/c309c6f04d2e https://hg.mozilla.org/mozilla-central/rev/c55086289922 https://hg.mozilla.org/mozilla-central/rev/339a34e9a135

Docs created: https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement/referrer https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement/referrer and updated: https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a https://developer.mozilla.org/en-US/docs/Web/HTML/Element/area https://developer.mozilla.org/en-US/Firefox/Releases/42#HTML