1174915 - Allow dynamic changes of referrer policies

Status: RESOLVED FIXED

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

Description

[Franziskus Kiefer [:franziskus]]

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 1

[Franziskus Kiefer [:franziskus]]

Attachment: allow changing meta referrer policy, src

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: allow changing meta referrer policy, tests

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

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?

Comment 5

[Franziskus Kiefer [:franziskus]]

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

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Franziskus Kiefer [:franziskus]]

Attachment: allow changing meta referrer policy, src

moved out setting meta referrer policy to HTMLMetaElement::SetMetaReferrer

Comment 8

[Andrea Marchesini [:baku]]

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.

Comment 9

[Franziskus Kiefer [:franziskus]]

Attachment: allow changing meta referrer policy, src

r+ carries over

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Franziskus, is there anything missing or can we land those patches within that bug?

Comment 11

[Franziskus Kiefer [:franziskus]]

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.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug1174915.patch

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.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug1174915-test.patch

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.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug1174915-test.patch

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c02f4d8cbb05
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f506110d1af

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c02f4d8cbb05
https://hg.mozilla.org/mozilla-central/rev/4f506110d1af