Implement referrer policy for WorkerGlobalScope #12473

aravind-pg · 2016-07-16T16:12:19Z

Adds a new Option<ReferrerPolicy> field to WorkerGlobalScope and sets it appropriately by parsing the header when setting up a ServiceWorkerGlobalScope or a DedicatedWorkerGlobalScope.

Also adds a new referrer policy accessor to GlobalRef and uses it in XMLHttpRequest's constructor.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11863
There are tests for these changes

This change is

highfive · 2016-07-16T16:12:23Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/bindings/global.rs, components/script/dom/workerglobalscope.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/script_thread.rs, components/script/dom/serviceworkerglobalscope.rs

aravind-pg · 2016-07-16T16:13:26Z

Only one WPT test was fixed by this PR, so I'm not sure I didn't do something wrong.

r? @jdm cc @rebstar6

jdm · 2016-07-17T15:10:59Z

I believe all of the worker test infrastructure in referrer-policy is disabled. I haven't looked into why that's the case yet.

aravind-pg · 2016-07-17T18:46:06Z

Ah I vaguely recall reading about this somewhere but can't seem to find it now. Could you point me to where exactly they're being disabled? I wasn't able to find anything in my look-around.

jdm · 2016-07-17T19:45:20Z

It's the worker-request thing in spec.src.json. Maybe it's excluded, or just not included.

aravind-pg · 2016-07-18T04:10:20Z

Updated to re-enable the worker-request tests, a bunch of which did pass :)

bors-servo · 2016-07-18T17:55:18Z

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

aravind-pg · 2016-07-21T06:33:38Z

Rebased.

aravind-pg · 2016-07-21T06:34:34Z

components/script/dom/xmlhttprequest.rs

-        } else {
-            (None, None)
-        };
+        let referrer_url = Some(global.get_url());


Note that now the field referrer_url is always Some, so I don't think it needs to stay an option any more?

Seems reasonable!

bors-servo · 2016-07-21T17:25:17Z

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

aravind-pg · 2016-07-23T17:24:19Z

Rebased.

jdm · 2016-07-23T18:44:40Z

I'll review this on Monday. Sorry for the wait!

aravind-pg · 2016-07-23T18:59:18Z

No problem at all.

jdm · 2016-07-25T22:45:30Z

The fact that we're failing tests like http-http/worker-request/insecure-protocol.keep-origin-redirect.http.html.ini make me suspect that either the tests aren't working right or our implementation isn't right. The changes in this PR look like what I expected to see, but we should track down the source of the test failure before moving ahead with merging this.

aravind-pg · 2016-07-26T04:58:29Z

Yeah I'm a bit surprised by it too. Will try to look into it more and come back if I have questions.

bors-servo · 2016-07-27T05:00:06Z

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

aravind-pg · 2016-08-01T04:23:29Z

@jdm Sorry for the delay here, I've been a bit busy with things and will continue to be for a few days more. If you have any suggestions (just high-level stuff off the top of your head) for where to start poking around to see why the tests are failing that would be super useful, but no problem at all if not, I'll just poke around myself (in a few days).

jdm · 2016-08-04T13:00:10Z

Aha!

we removed generic/subresources/worker.py from the forked tests
we need to reinstate it with a change like https://dxr.mozilla.org/servo/source/tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/subresource/document.py#4
That should give us much more meaningful results.


        script: Add a referrer policy field to WorkerGlobalScope


        script: Add referrer policy API to GlobalRef and use it in XMLHttpReq…

…uest::new_inherited


        test: Re-enable referrer policy worker tests


        test: Restore worker.py test runner to wpt/mozilla/.../referrer-policy


        test: Update wpt test expectations

aravind-pg · 2016-09-04T13:44:37Z

@jdm Really sorry for vanishing for so long, was a busy time for me.

So I reinstated worker.py as per your suggestion, but unfortunately it seems not to have made any difference. I'm really not sure what's going on here, but I'll try to dig some more.

jdm · 2016-09-04T16:29:11Z

Does worker.js.template exist? If not we need to reinstate it as well.

aravind-pg · 2016-09-04T19:06:22Z

Does look like it does: worker.js.template.

jdm · 2016-09-05T13:38:08Z

I took a look at the branch locally and figured out what's going on. b423496 isn't necessary any more since we're enabling the worker tests in the upstream tests, rather than our local fork. The reason why the tests are failing is that they are testing the reported referrer for the initial worker object that is created, while the changes in 33007c2 and 53cdc62 are in support of referrers for requests created from a worker script. Fixing the tests should only require modifiying the WorkerScriptLoadOrigin object in Worker::Constructor to use the document's referrer policy; however, we should add a new set of tests that initiates requests via XMLHttpRequest inside of a worker to ensure that the changes in this PR actually work.

bors-servo · 2016-09-06T18:35:03Z

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

aravind-pg · 2016-10-23T11:47:53Z

Very sorry for the extend to which this stagnated, @jdm. I think I'll just close this PR for now and perhaps open a new one when I find some time. Thanks a lot for your help, and I'll keep your last comment in mind when I reopen.

highfive assigned wafflespeanut Jul 16, 2016

highfive added the S-awaiting-review label Jul 16, 2016

highfive assigned jdm and unassigned wafflespeanut Jul 16, 2016

aravind-pg force-pushed the aravind-pg:referrer-pol-worker branch from bd7b820 to 17feb71 Jul 18, 2016

highfive added the S-needs-rebase label Jul 18, 2016

aravind-pg force-pushed the aravind-pg:referrer-pol-worker branch 2 times, most recently from 99942d0 to 8bed635 Jul 21, 2016

aravind-pg reviewed Jul 21, 2016
View changes

highfive added the S-needs-rebase label Jul 21, 2016

aravind-pg force-pushed the aravind-pg:referrer-pol-worker branch from 8bed635 to 0c2343e Jul 23, 2016

jdm added S-tests-failed and removed S-awaiting-review S-needs-rebase labels Jul 25, 2016

highfive added the S-needs-rebase label Jul 27, 2016

aravind-pg added 5 commits Jul 16, 2016

script: Add a referrer policy field to WorkerGlobalScope

33007c2

script: Add referrer policy API to GlobalRef and use it in XMLHttpReq…

53cdc62

…uest::new_inherited

test: Re-enable referrer policy worker tests

24160ba

test: Restore worker.py test runner to wpt/mozilla/.../referrer-policy

b423496

test: Update wpt test expectations

Loading status checks…

fe7cbad

aravind-pg force-pushed the aravind-pg:referrer-pol-worker branch from 0c2343e to fe7cbad Sep 4, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Sep 4, 2016

jdm added S-needs-tests and removed S-awaiting-review S-needs-rebase labels Sep 5, 2016

highfive added the S-needs-rebase label Sep 6, 2016

aravind-pg closed this Oct 23, 2016

jdm mentioned this pull request Oct 23, 2016

Implement referrer policy for WorkerGlobalScope #11863

Open

servo / servo

Implement referrer policy for WorkerGlobalScope #12473

Implement referrer policy for WorkerGlobalScope #12473

aravind-pg commented Jul 16, 2016 •

edited

highfive commented Jul 16, 2016

aravind-pg commented Jul 16, 2016

jdm commented Jul 17, 2016

aravind-pg commented Jul 17, 2016

jdm commented Jul 17, 2016

aravind-pg commented Jul 18, 2016 •

edited

bors-servo commented Jul 18, 2016

aravind-pg commented Jul 21, 2016

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jul 21, 2016

aravind-pg commented Jul 23, 2016

jdm commented Jul 23, 2016

aravind-pg commented Jul 23, 2016

jdm commented Jul 25, 2016

aravind-pg commented Jul 26, 2016

bors-servo commented Jul 27, 2016

aravind-pg commented Aug 1, 2016

jdm commented Aug 4, 2016 •

edited

aravind-pg commented Sep 4, 2016

jdm commented Sep 4, 2016

aravind-pg commented Sep 4, 2016

jdm commented Sep 5, 2016

bors-servo commented Sep 6, 2016

aravind-pg commented Oct 23, 2016

servo / servo

Join GitHub today

Implement referrer policy for WorkerGlobalScope #12473

Implement referrer policy for WorkerGlobalScope #12473

Conversation

aravind-pg commented Jul 16, 2016 • edited

highfive commented Jul 16, 2016

aravind-pg commented Jul 16, 2016

jdm commented Jul 17, 2016

aravind-pg commented Jul 17, 2016

jdm commented Jul 17, 2016

aravind-pg commented Jul 18, 2016 • edited

bors-servo commented Jul 18, 2016

aravind-pg commented Jul 21, 2016

This comment has been minimized.

aravind-pg Jul 21, 2016

This comment has been minimized.

jdm Jul 25, 2016

bors-servo commented Jul 21, 2016

aravind-pg commented Jul 23, 2016

jdm commented Jul 23, 2016

aravind-pg commented Jul 23, 2016

jdm commented Jul 25, 2016

aravind-pg commented Jul 26, 2016

bors-servo commented Jul 27, 2016

aravind-pg commented Aug 1, 2016

jdm commented Aug 4, 2016 • edited

aravind-pg commented Sep 4, 2016

jdm commented Sep 4, 2016

aravind-pg commented Sep 4, 2016

jdm commented Sep 5, 2016

bors-servo commented Sep 6, 2016

aravind-pg commented Oct 23, 2016

aravind-pg commented Jul 16, 2016 •

edited

aravind-pg commented Jul 18, 2016 •

edited

jdm commented Aug 4, 2016 •

edited