Implement meta referrer policy delivery (3) #11468

rebstar6 · 2016-05-27T13:27:56Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10311 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-05-27T13:27:59Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmlmetaelement.rs, components/script/dom/document.rs, components/net/http_loader.rs, components/script/dom/xmlhttprequest.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/htmliframeelement.rs, components/script/dom/webidls/Document.webidl

rebstar6 · 2016-05-27T13:28:40Z

@nox - new (old) issue!

nox · 2016-05-27T20:36:31Z

Please squash the two commits together.

-S-awaiting-review +S-needs-code-changes +S-needs-squash

Previously, rebstar6 (Rebecca) wrote…

@nox - new (old) issue!

Reviewed 8 of 8 files at r1, 48 of 48 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

components/msg/constellation_msg.rs, line 355 [r1] (raw file):

/// [Policies](https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-states)
/// for providing a referrer header for a request
#[derive(HeapSizeOf, Clone, Deserialize, Serialize, Debug, Copy)]

Nit: alphanumeric order.

components/script/dom/document.rs, line 2804 [r1] (raw file):

    fn Referrer(&self) -> DOMString {
        //TODO - unimplemented, for ref pol tests
        DOMString::new()

Why don't we implement that? Should we remove it altogether for now if we can't? Or does that mean new test hacks?

components/script/dom/htmlmetaelement.rs, line 100 [r1] (raw file):

        https://html.spec.whatwg.org/multipage/#meta-referrer
        https://w3c.github.io/webappsec-referrer-policy/#set-referrer-policy
        */

So the spec is pretty clear, conceptually, the meta referrer policy is the referrer policy of the first meta name="referrer" element which satisfies all the conditions.

You should write a method on HTMLHeadElement that returns the first referrer policy it finds among the element children, and call this whenever there is a mutation somewhere that could change the referrer policy.

To do the second part, you will need to patch attribute_mutated, bind_from_tree and unbind_from_tree in the implementation of VirtualMethods for HTMLMetaElement.

components/script/dom/xmlhttprequest.rs, line 158 [r1] (raw file):

    fn new_inherited(global: GlobalRef) -> XMLHttpRequest {
        //TODO - will this panic (outside of the scope of the ref policy tests)?
        let current_doc = global.as_window().Document();

let (referrer_policy, referrer_url) = if let GlobalRef::Window(window) = global {
    let document = window.Document();
    (document.url().clone(), document.get_referrer_policy())
} else {
    (None, None)
};

And please put a todo about doing this correctly for workers.

Comments from Reviewable

bors-servo · 2016-05-30T12:44:38Z

☔ The latest upstream changes (presumably #11413) made this pull request unmergeable. Please resolve the merge conflicts.

rebstar6 · 2016-05-31T15:18:24Z

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

components/msg/constellation_msg.rs, line 355 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Nit: alphanumeric order.

Done.

components/script/dom/document.rs, line 2804 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Why don't we implement that? Should we remove it altogether for now if we can't? Or does that mean new test hacks?

I believe it is required for test hacks (@jdm can confirm). Implementing requires passing info from http_loader through to document, which is also required to implement referrer policy deliver via header. Given that, I was planning to group those two into the next PR.

components/script/dom/htmlmetaelement.rs, line 100 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

So the spec is pretty clear, conceptually, the meta referrer policy is the referrer policy of the first meta name="referrer" element which satisfies all the conditions.

You should write a method on HTMLHeadElement that returns the first referrer policy it finds among the element children, and call this whenever there is a mutation somewhere that could change the referrer policy.

To do the second part, you will need to patch attribute_mutated, bind_from_tree and unbind_from_tree in the implementation of VirtualMethods for HTMLMetaElement.

Pushing other changes - this is still TODO

components/script/dom/xmlhttprequest.rs, line 158 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

let (referrer_policy, referrer_url) = if let GlobalRef::Window(window) = global {
    let document = window.Document();
    (document.url().clone(), document.get_referrer_policy())
} else {
    (None, None)
};

And please put a todo about doing this correctly for workers.

Done.

Comments from Reviewable

jdm · 2016-05-31T16:57:53Z

The mozilla-specific tests were modified to not require document.referrer any more, so I think it would make sense to remove the stub implementation.

nox · 2016-06-01T10:05:19Z

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

components/script/dom/document.rs, line 2804 [r1] (raw file):

Previously, rebstar6 (Rebecca) wrote…

I believe it is required for test hacks (@jdm can confirm). Implementing requires passing info from http_loader through to document, which is also required to implement referrer policy deliver via header. Given that, I was planning to group those two into the next PR.

@jdm says it can apparently be removed altogether for now, given we modified the tests.

Comments from Reviewable

highfive · 2016-06-01T15:42:12Z

New code was committed to pull request.

rebstar6 · 2016-06-01T15:43:28Z

Review status: 53 of 56 files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/htmlmetaelement.rs, line 100 [r1] (raw file):

Previously, rebstar6 (Rebecca) wrote…

Pushing other changes - this is still TODO

Ok see latest commit - moved logic into HTMLHeadElement with a call from HTMLMetaElement. Still need to add calls within attribute_mutated and unbind_from_tree, but that should be quick if this piece looks right.

Comments from Reviewable

nox · 2016-06-01T15:49:06Z

-S-awaiting-review +S-needs-code-changes

Ssssssh... Do you hear it? Do you hear the merge coming to us, slowly but surely? :)

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 2 of 2 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

components/script/dom/htmlheadelement.rs, line 43 [r5] (raw file):

    pub fn set_document_referrer(&self) {
        //TODO is this THE head element, or any? If any, should confirm it's the one
        //on second thought, may not matter if this is only called from the metaelements

I think you can instead call this from HTMLMetaElement if the parent of the meta element is a head, and check here whether that head is the head element.

components/script/dom/htmlheadelement.rs, line 50 [r5] (raw file):

                           .filter(|elem| elem.get_string_attribute(&atom!("name")) == "referrer")
                           .filter(|elem| elem.get_attribute(&ns!(), &atom!("content")).is_some());

Nit: unneeded blanks.

components/script/dom/htmlmetaelement.rs, line 100 [r1] (raw file):

Previously, rebstar6 (Rebecca) wrote…

Ok see latest commit - moved logic into HTMLHeadElement with a call from HTMLMetaElement. Still need to add calls within attribute_mutated and unbind_from_tree, but that should be quick if this piece looks right.

This is mostly what I had in mind. Great work!

Comments from Reviewable

highfive · 2016-06-01T18:47:11Z

New code was committed to pull request.

bors-servo · 2016-06-02T18:04:36Z

💔 Test failed - linux-rel

highfive · 2016-06-02T18:04:38Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-002.htm
  └   → /css-transforms-1_dev/html/transform-abspos-002.htm 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 78d197606924062e8dd2a773c977afcecf8940f8
Testing 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8 == 78d197606924062e8dd2a773c977afcecf8940f8

nox · 2016-06-02T18:26:58Z

I'm pretty sure this is intermittent.

jdm · 2016-06-02T18:31:39Z

Filed #11561.

rebstar6 · 2016-06-02T21:20:05Z

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/script/dom/htmlheadelement.rs, line 48 [r6] (raw file):

Previously, nox (Anthony Ramine) wrote…

I know it returns an Option, but we have .r() on Option. :)

it's still an option though...i'll compare it to Some(self)?

components/script/dom/htmlmetaelement.rs, line 108 [r6] (raw file):

Previously, nox (Anthony Ramine) wrote…

It's more confusing to have the tests in two places.

Which 2 places? These checks should still be there, or set_document_referrer is going to get called for every meta element of any type...which seems super inefficient (?)

Comments from Reviewable

highfive · 2016-06-02T21:23:14Z

New code was committed to pull request.

nox · 2016-06-02T23:19:41Z

Remove the TODO comment that you forgot and r=me. Great work!

-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

components/script/dom/htmlheadelement.rs, line 48 [r6] (raw file):

Previously, rebstar6 (Rebecca) wrote…

it's still an option though...i'll compare it to Some(self)?

Yes exactly.

components/script/dom/htmlmetaelement.rs, line 108 [r6] (raw file):

Previously, rebstar6 (Rebecca) wrote…

Which 2 places? These checks should still be there, or set_document_referrer is going to get called for every meta element of any type...which seems super inefficient (?)

Never mind, I was talking about the two `if let` to get the parent `head` element, but we can't even avoid them given we have to call the `set_document_referrer` method on it. :)

Comments from Reviewable


        Add meta-referrer support for documents

highfive · 2016-06-03T01:48:50Z

New code was committed to pull request.

rebstar6 · 2016-06-03T01:49:48Z

Should be set. I believe my 'todos' are in order as well. Woot woot!

KiChjang · 2016-06-03T02:51:06Z

@bors-servo r=nox

bors-servo · 2016-06-03T02:51:09Z

📌 Commit 687d0cd has been approved by nox

bors-servo · 2016-06-03T02:51:11Z

⌛ Testing commit 687d0cd with merge 530b5a6...


        Auto merge of #11468 - rebstar6:refPol4, r=nox

Implement meta referrer policy delivery (3)  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10311 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11468)

bors-servo · 2016-06-03T03:40:50Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned glennw May 27, 2016

rebstar6 mentioned this pull request May 27, 2016

Implement meta referrer policy delivery (2) #11422

Closed

4 of 5 tasks complete

jdm assigned nox and unassigned glennw May 27, 2016

rebstar6 force-pushed the rebstar6:refPol4 branch from 9e5f9da to ed73932 May 31, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels May 31, 2016

nox added S-needs-code-changes and removed S-awaiting-review S-needs-squash S-needs-rebase labels Jun 1, 2016

rebstar6 force-pushed the rebstar6:refPol4 branch from ed73932 to 1f01c88 Jun 1, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 1, 2016

nox added S-needs-code-changes and removed S-awaiting-review labels Jun 1, 2016

rebstar6 force-pushed the rebstar6:refPol4 branch from 1f01c88 to e47f8a8 Jun 1, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 1, 2016

highfive added the S-tests-failed label Jun 2, 2016

nox removed the S-tests-failed label Jun 2, 2016

rebstar6 force-pushed the rebstar6:refPol4 branch from 4dd410f to e7f3307 Jun 2, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 2, 2016

nox added S-needs-code-changes and removed S-awaiting-review labels Jun 2, 2016

Add meta-referrer support for documents

Loading status checks…

687d0cd

rebstar6 force-pushed the rebstar6:refPol4 branch from e7f3307 to 687d0cd Jun 3, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 3, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 3, 2016

bors-servo merged commit 687d0cd into servo:master Jun 3, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 3, 2016

bors-servo mentioned this pull request Jun 3, 2016

Make fetch happen #11556

Merged

2 of 4 tasks complete

servo / servo

Implement meta referrer policy delivery (3) #11468

Implement meta referrer policy delivery (3) #11468

rebstar6 commented May 27, 2016 •

edited by larsbergstrom

highfive commented May 27, 2016

rebstar6 commented May 27, 2016

nox commented May 27, 2016

bors-servo commented May 30, 2016

rebstar6 commented May 31, 2016

jdm commented May 31, 2016

nox commented Jun 1, 2016

highfive commented Jun 1, 2016

rebstar6 commented Jun 1, 2016

nox commented Jun 1, 2016

highfive commented Jun 1, 2016

bors-servo commented Jun 2, 2016

highfive commented Jun 2, 2016

nox commented Jun 2, 2016

jdm commented Jun 2, 2016

rebstar6 commented Jun 2, 2016

highfive commented Jun 2, 2016

nox commented Jun 2, 2016

highfive commented Jun 3, 2016

rebstar6 commented Jun 3, 2016

KiChjang commented Jun 3, 2016

bors-servo commented Jun 3, 2016

bors-servo commented Jun 3, 2016

bors-servo commented Jun 3, 2016

servo / servo

Join GitHub today

Implement meta referrer policy delivery (3) #11468

Implement meta referrer policy delivery (3) #11468

Conversation

rebstar6 commented May 27, 2016 • edited by larsbergstrom

highfive commented May 27, 2016

rebstar6 commented May 27, 2016

nox commented May 27, 2016

bors-servo commented May 30, 2016

rebstar6 commented May 31, 2016

jdm commented May 31, 2016

nox commented Jun 1, 2016

highfive commented Jun 1, 2016

rebstar6 commented Jun 1, 2016

nox commented Jun 1, 2016

highfive commented Jun 1, 2016

bors-servo commented Jun 2, 2016

highfive commented Jun 2, 2016

nox commented Jun 2, 2016

jdm commented Jun 2, 2016

rebstar6 commented Jun 2, 2016

highfive commented Jun 2, 2016

nox commented Jun 2, 2016

highfive commented Jun 3, 2016

rebstar6 commented Jun 3, 2016

KiChjang commented Jun 3, 2016

bors-servo commented Jun 3, 2016

bors-servo commented Jun 3, 2016

bors-servo commented Jun 3, 2016

rebstar6 commented May 27, 2016 •

edited by larsbergstrom