Allow dynamic changes of referrer policies

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: franziskus, Assigned: franziskus)

Tracking

(Blocks 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

After internal discussions and with the working group I think we should allow to dynamically change the referrer policy.

This should work for referrer attributes in anchor and area tags when bug 1174913 is landed.

The meta referrer however needs to be fixed, as changes here are ignored.

Chrome has numbers on pages that change referrer policies on the fly to a different value [1].


[1] https://www.chromestatus.com/metrics/feature/popularity#ResetReferrerPolicy
Comment on attachment 8627293 [details] [diff] [review]
allow changing meta referrer policy, src

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

This looks like it will work, but do you want to report erroneous policies to the error console or something?  You can use IsValidReferrerPolicy for the check if you want.

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/ReferrerPolicy.h#64

You probably need a DOM peer to review this patch.
Attachment #8627293 - Flags: review?(mozbugs) → feedback+
Comment on attachment 8627294 [details] [diff] [review]
allow changing meta referrer policy, tests

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

Sorry it took me so long to get to this!

Template strings might make the sjs stuff a little simpler... https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings

Test looks good.  Just some minor comments!

::: dom/base/test/referrer_change_server.sjs
@@ +19,5 @@
> +               // LOAD EVENT (of the test)
> +               // fires when the page is loaded, then click link
> +               // first change meta referrer, then click link
> +               'window.addEventListener("load", function() {\n\
> +                 document.getElementsByName("referrer")[0].content = "'+aReferrerPolicy+'";\n\

What should happen if instead of modifying the meta tag, we add a second one?

@@ +28,5 @@
> +           </body>\n\
> +         </html>';
> +}
> +
> +function createTest2(aMetaPolicy, aReferrerPolicy, aName) {

To make this a little easier to follow and debug, please give this function a name that describes how it's different from the other one (and maybe rename createTest as well). Perhaps createDotContentModifierTest and createSetAttributeTest or similar?  You'll want to modify the parameters to the sjs to reflect this as well (generate-policy-test and generate-policy-test2).

::: dom/base/test/test_change_policy.html
@@ +7,5 @@
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +
> +<!--
> +This checks if the right policies are applied from a given string when the policy is changed after the document has been loaded.
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1101288

In this comment block, please describe the structure of the test.  What frames are created and how are they related?
Attachment #8627294 - Flags: review?(mozbugs) → review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3)
> Comment on attachment 8627293 [details] [diff] [review]
> allow changing meta referrer policy, src
> 
> Review of attachment 8627293 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like it will work, but do you want to report erroneous policies
> to the error console or something?  You can use IsValidReferrerPolicy for
> the check if you want.

If we do this, we should do it everywhere. Filed bug 1185719 for this.
Attachment #8627293 - Flags: review?(bobbyholley)
Comment on attachment 8627293 [details] [diff] [review]
allow changing meta referrer policy, src

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

We should presumably be sharing this code with the code in HTMLMetaElement::BindToTree, right?

Note that I'm about to go on PTO, so you may want another reviewer.
Attachment #8627293 - Flags: review?(bobbyholley) → review-
moved out setting meta referrer policy to HTMLMetaElement::SetMetaReferrer
Attachment #8627293 - Attachment is obsolete: true
Attachment #8638026 - Flags: review?(amarchesini)
Comment on attachment 8638026 [details] [diff] [review]
allow changing meta referrer policy, src

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

::: dom/html/HTMLMetaElement.cpp
@@ +50,5 @@
>  
> +nsresult
> +HTMLMetaElement::SetMetaReferrer(nsIDocument* aDocument)
> +{
> +  nsresult rv = NS_OK;

remove this.

@@ +52,5 @@
> +HTMLMetaElement::SetMetaReferrer(nsIDocument* aDocument)
> +{
> +  nsresult rv = NS_OK;
> +  if (aDocument &&
> +      AttrValueIs(kNameSpaceID_None, nsGkAtoms::name, nsGkAtoms::referrer, eIgnoreCase)) {

What about:

if (!aDocument ||
    !AttrValueIs(kNameSpaceID_None, nsGkAtoms::name, nsGkAtoms::referrer, eIgnoreCase)) {
  return NS_OK;
}

@@ +54,5 @@
> +  nsresult rv = NS_OK;
> +  if (aDocument &&
> +      AttrValueIs(kNameSpaceID_None, nsGkAtoms::name, nsGkAtoms::referrer, eIgnoreCase)) {
> +    nsAutoString content;
> +    rv = GetContent(content);

nsresult rv = GetContent(content);
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +62,5 @@
> +        content = nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespace>(content);
> +        aDocument->SetHeaderData(nsGkAtoms::referrer, content);
> +    }
> +  }
> +  return rv;

return NS_OK;

@@ +76,5 @@
>        CreateAndDispatchEvent(document, NS_LITERAL_STRING("DOMMetaChanged"));
>      }
> +    // Update referrer policy when it got changed from JS
> +    nsresult rv = SetMetaReferrer(document);
> +    NS_ENSURE_SUCCESS(rv, rv);

I still don't know if we are pro or con this macro, but write this:

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +102,5 @@
>    }
> +  // Referrer Policy spec requires a <meta name="referrer" tag to be in the
> +  // <head> element.
> +  rv = SetMetaReferrer(aDocument);
> +  NS_ENSURE_SUCCESS(rv, rv);

same here.
Attachment #8638026 - Flags: review?(amarchesini) → review+
r+ carries over
Attachment #8638026 - Attachment is obsolete: true
Attachment #8638721 - Flags: review+
Franziskus, is there anything missing or can we land those patches within that bug?
Flags: needinfo?(franziskuskiefer)
I thought of rewriting the tests to make them a bit nicer (haven't addressed sid's comments yet either). The code is fine though.
Flags: needinfo?(franziskuskiefer)
As discussed with Steve, we should get those patches landed to avoid bitrot. I rebased the patch, this one is ready to go. Carrying over r+ from baku and sstamm.
Attachment #8659986 - Flags: review+
Posted patch bug1174915-test.patch (obsolete) — Splinter Review
Again, as discussed with Steve we should get those patches landed to avoid bitrot. This test could need a little cleanup in fact, but not critical at the moment. Carrying over r+ from sstamm.
Attachment #8627294 - Attachment is obsolete: true
Attachment #8638721 - Attachment is obsolete: true
Attachment #8659987 - Flags: review+
Here is the try-push of the rebased patches:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c281b38e7ce3

B2G ICS Emulator looks troublesome - I suppose we need to inspect those tests again before we can land these patches.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> Here is the try-push of the rebased patches:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c281b38e7ce3
> 
> B2G ICS Emulator looks troublesome - I suppose we need to inspect those
> tests again before we can land these patches.

Ah, it's because B2G EMU does not have SSL support. Disabling the tests on B2G.
Attachment #8659987 - Attachment is obsolete: true
Attachment #8660140 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c02f4d8cbb05
https://hg.mozilla.org/mozilla-central/rev/4f506110d1af
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.