Allow browsing contexts to resolve to cross-origin windows #15358

asajeffrey · 2017-02-03T00:01:13Z

This PR implements cross-thread WindowProxy objects.

At the moment, if a Window performs a non-similar-origin navigation, the old script thread does not update its WindowProxy, since the new Window is in the new script thread. With this PR, the WindowProxy is updated to a dummy XOriginWindow object, that only implements the whitelisted methods that are allowed to be called cross-origin.

This PR does not include working implementations of some of the cross-origin Window or Location methods.

This PR causes some cross-origin wpt tests to now pass, in particular /html/browsers/origin/cross-origin-objects/cross-origin-objects.html now passes Only whitelisted properties are accessible cross-origin. There are some CORS failures in fetch, I suspect caused by the incorrect setting of origin in fetch requests.

Although there are some functions that now throw SecurityException, it is not meant to be a complete implementation, which will have to wait for XOWs to land.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix WindowProxy for browsing contexts whose active document is in another script thread #15180.
There are tests for these changes

This change is

highfive · 2017-02-03T00:01:17Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/xoriginlocation.rs, components/script/dom/mod.rs, components/script/dom/window.rs, components/script/dom/browsingcontext.rs, components/script/fetch.rs, components/script/dom/bindings/iterable.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webidls/XOriginLocation.webidl, components/script/dom/webidls/XOriginWindow.webidl, components/script/script_thread.rs, components/script/dom/xoriginwindow.rs, components/script/dom/node.rs, components/script/dom/bindings/reflector.rs
@KiChjang: components/script/dom/xoriginlocation.rs, components/script/dom/mod.rs, components/script/dom/window.rs, components/script/dom/browsingcontext.rs, components/script/fetch.rs, components/script/dom/bindings/iterable.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webidls/XOriginLocation.webidl, components/script/dom/webidls/XOriginWindow.webidl, components/script/script_thread.rs, components/script/dom/xoriginwindow.rs, components/script/dom/node.rs, components/script/dom/bindings/reflector.rs

highfive · 2017-02-03T00:01:18Z

Warning

These commits modify unsafe code. Please review it carefully!

asajeffrey · 2017-02-03T00:02:29Z

cc @avadacatavra @bzbarsky @Ms2ger
r? @jdm

Ms2ger · 2017-02-03T13:25:27Z

components/script/dom/bindings/reflector.rs

+    #[inline]
+    pub fn get_cx(&self) -> *mut JSContext {
+        unsafe { JS_GetContext(self.get_runtime()) }
+    }


Why? We might as well use Runtime::new(); that's probably faster than two out-of-line calls.

This probably isn't needed any more, originally I was fighting to try to get Servo to have globals that aren't GlobalScopes but eventually gave up. This looks like code that was left over from that fight.

Ms2ger · 2017-02-03T13:27:12Z

components/script/dom/browsingcontext.rs

@@ -61,7 +70,7 @@ impl BrowsingContext {
            let WindowProxyHandler(handler) = window.windowproxy_handler();
            assert!(!handler.is_null());

-            let cx = window.get_cx();
+            let cx = window.reflector().get_cx();


Ms2ger · 2017-02-03T13:27:54Z

components/script/dom/browsingcontext.rs

@@ -104,13 +114,13 @@ impl BrowsingContext {
    /// Change the Window that this browsing context's WindowProxy resolves to.
    // TODO: support setting the window proxy to a dummy value,
    // to handle the case when the active document is in another script thread.
-    pub fn set_window_proxy(&self, window: &Window) {
+    fn set_window_proxy<W: DomObject>(&self, window: &W, traps: &ProxyTraps) {


Allowing any DomObject here does not sound like a great strategy.

Ditto. The problem was that when I started this, XOriginWindow was a global, but not a GlobalScope. Now it is, so I can make this a GlobalScope, so we can statically check that it's only called with globals.

Ms2ger · 2017-02-03T13:28:32Z

components/script/dom/browsingcontext.rs

+    }
+
+    pub fn unset_currently_active(&self) {
+        let win = XOriginWindow::new(self);


Would CrossOriginWindow be that much harder to type?

Ooh, bikeshedding! XThreadWindow? CrossThreadWindow? DissimillarOriginWindow?

Ms2ger · 2017-02-03T13:30:11Z

components/script/dom/xoriginlocation.rs

+
+    // https://html.spec.whatwg.org/multipage/#dom-location-href
+    fn Stringifier(&self) -> DOMString {
+        DOMString::from("<XOriginWindow>")


Er yes, not sure what to put here. Stringifiers aren't allowed to throw, so we have to return something. Once we have XOWs, this will never be called so we can panic!, but in the mean time we have to return some DOMString.

Er... Why are stringifiers not allowed to throw? There's nothing that prevents a spec from defining a stringifier that sometimes throws.

@bzbarsky D'oh! Fixed this, the stringifier now throws a security exception.

Ms2ger · 2017-02-03T13:30:33Z

components/script/dom/xoriginwindow.rs

+use js::jsval::{JSVal, UndefinedValue};
+use msg::constellation_msg::PipelineId;
+
+// TODO: documentation!


Oh yes, must write some of that.

Ms2ger · 2017-02-03T13:32:01Z

tests/wpt/web-platform-tests/common/get-host-info.sub.js

@@ -4,7 +4,7 @@ function get_host_info() {
  var HTTP_PORT2 = '{{ports[http][1]}}';
  var HTTPS_PORT = '{{ports[https][0]}}';
  var ORIGINAL_HOST = '{{host}}';
-  var REMOTE_HOST = (ORIGINAL_HOST === 'localhost') ? '127.0.0.1' : ('www1.' + ORIGINAL_HOST);
+  var REMOTE_HOST = (ORIGINAL_HOST === '127.0.0.1') ? '127.0.0.2' : '127.0.0.1';


r? @jgraham

@jgraham the reason for this change is to run the test suite with a non-similar REMOTE_HOST. At the moment, ORIGINAL_HOST and REMOTE_HOST are similar-origin, so we never actually run any tests cross-thread.

Doesn't this break tests? REMOTE_HOST is supposed to be a URL that is actually reachable; I would expect the case that uses 127.0.0.2 to time out whenever it attempts to connect.

bors-servo · 2017-02-07T12:29:46Z

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

jdm · 2017-02-08T19:34:14Z

Looks like a ./mach update-manifest is required.

jdm

Nothing particularly concerning here. Well done!

jdm · 2017-02-08T20:48:26Z

components/script/dom/webidls/XOriginWindow.webidl

+
+// https://html.spec.whatwg.org/multipage/#window
+[Global, NoInterfaceObject]
+interface XOriginWindow : GlobalScope {


Note: this makes iframe.contentWindow instanceof Window return false for cross-origin iframes. It might be possible to override the hasInstance proxy trap for this, but XOriginLocation can't do that. I'm not sure what the solution is.

Oh good point. Should I just file an issue for this?

One possible solution, but it would mean rewriting codegen, would be to allow more than one Servo implementation of a webidl class, in particular to make Foo::Wrap generic in any type which implements FooMethods, not specific to Foo. This isn't an easy change though.

An issue would be good.

Filed #15476.

jdm · 2017-02-08T22:16:15Z

components/script/dom/browsingcontext.rs

+    pub fn unset_currently_active(&self) {
+        let window = XOriginWindow::new(self);
+        let globalscope = window.global();
+        self.set_window_proxy(&*globalscope, &XORIGIN_PROXY_HANDLER);


This looks like a confusing way of writing:

let window = XOriginWindow::new(self); self.set_window_proxy(&*window.upcast(), &XORIGIN_PROXY_HANDLER);

jdm · 2017-02-08T22:16:55Z

components/script/dom/browsingcontext.rs

@@ -144,6 +154,23 @@ impl BrowsingContext {
        }
    }

+    pub fn set_currently_active(&self, window: &Window) {
+        let globalscope = window.global();
+        self.set_window_proxy(&*globalscope, &PROXY_HANDLER);


let globalscope = window.upcast();

jdm · 2017-02-08T22:20:31Z

components/script/dom/browsingcontext.rs

        unsafe {
            debug!("Setting window proxy of {:p}.", self);
-            let WindowProxyHandler(handler) = window.windowproxy_handler();
+            let handler = CreateWrapperProxyHandler(traps);


We shouldn't need to create a new handler every time here. We can create it once and store it in DomStaticData like the regular windowproxy handler.

Ah, is CreateWrapperProxyHandler expensive?

It's more that we only need a singleton, and creating more will lead to memory leaks if we don't clean them up.

Won't they just get gc'd when the window proxy is unreachable?

jdm · 2017-02-08T22:30:44Z

components/script/dom/xoriginlocation.rs

+            reflector: Reflector::new(),
+            window: JS::from_ref(window),
+        };
+        unsafe {


I think we can use reflect_dom_object here and remove the unsafe_code allow.

Fixed. This was a holdover from XOriginWindow not implementing GlobalScope.

jdm · 2017-02-08T22:56:22Z

components/script/dom/xoriginwindow.rs

+        // Any timer events fired on this window are ignored.
+        let (timer_event_chan, _) = ipc::channel().unwrap();
+        let win = box XOriginWindow {
+            globalscope: GlobalScope::new_inherited(PipelineId::new(),


We may want a TODO here about storing the actual pipeline that this cross-origin window represents.

We could do that for the initial pipeline id, but to keep that id up-to-date, we'd have to send a lot of messages around (N messages ever time the pipeline changed) which seems not worth it. I'll add a comment about this.

jdm · 2017-02-08T23:02:38Z

tests/wpt/web-platform-tests/common/get-host-info.sub.js

@@ -4,7 +4,7 @@ function get_host_info() {
  var HTTP_PORT2 = '{{ports[http][1]}}';
  var HTTPS_PORT = '{{ports[https][0]}}';
  var ORIGINAL_HOST = '{{host}}';
-  var REMOTE_HOST = (ORIGINAL_HOST === 'localhost') ? '127.0.0.1' : ('www1.' + ORIGINAL_HOST);
+  var REMOTE_HOST = (ORIGINAL_HOST === '127.0.0.1') ? '127.0.0.2' : '127.0.0.1';


Doesn't this break tests? REMOTE_HOST is supposed to be a URL that is actually reachable; I would expect the case that uses 127.0.0.2 to time out whenever it attempts to connect.

asajeffrey · 2017-02-09T15:54:24Z

For some reason, github isn't letting me reply to #15358 (comment)

Good point, there's no http server running on 127.0.0.2. As a patch, I can set it to web-platform.test, but it would be nice if we had two dissimilar-origin host names. @jgraham?

jdm · 2017-02-09T16:00:21Z

Yeah, http://testthewebforward.org/docs/test-format-guidelines.html#tests-involving-multiple-origins only describes subdomains unfortunately.

jgraham · 2017-02-09T16:00:23Z

Well, https and multiple ports are supposed to give you dissimilar origins. I still don't really understand why servo can't use those things as a part of the iframe process heuristics? But if we want another top level host, that should be possible but will require more coordination to check that we have buy-in.

bzbarsky · 2017-02-09T16:12:36Z

Well, https and multiple ports are supposed to give you dissimilar origins.

Multiple ports is not enough to give you dissimilar origins. If you have "http://foo.com:10" and "http://foo.com:20" and they both set document.domain to "foo.com", they end up same origin-domain.

https is enough, though.

asajeffrey · 2017-02-09T16:22:44Z

I don't think we want to require all our cross-origin tests to run with one server http and one https.

jdm

@jgraham will need to sign off on the get-host-info.sub.js change.

jdm · 2017-02-09T19:46:10Z

components/script/dom/browsingcontext.rs

@@ -45,6 +46,9 @@ pub struct BrowsingContext {

    /// The pipeline id of the currently active document.
    /// May be None, when the currently active document is in another script thread.
+    /// We do not try to keep the pipeline id for documents in other threads,
+    /// as this would require the constelltion notifying many script threads about


s/constelltion/constellation/

jdm · 2017-02-09T19:48:04Z

components/script/dom/xoriginlocation.rs

-        let cx = globalscope.get_cx();
-        let loc = box XOriginLocation {
+    #[allow(unrooted_must_root)]
+    pub fn new_inherited(window: &XOriginWindow) -> XOriginLocation {


This should not be public.

jdm · 2017-02-09T19:48:59Z

tests/wpt/mozilla/meta/MANIFEST.json

+  "mozilla/referrer-policy/generic/subresource/mozresource.pyc": [
+   "c3d7573593042e1d218e532071225d92c4920954",
+   "support"
+  ],


You'll need to remove the pyc files from your working tree and run manifest-update again.

asajeffrey · 2017-02-16T01:02:25Z

Okay, my guess is that this test is genuinely timing out on the CI hardware, I'll try splitting it into subtests and see if that helps.

asajeffrey · 2017-02-16T15:30:32Z

I radically trimmed back /_mozilla/mozilla/cross-origin-objects/cross-origin-objects.html, hopefully it won't TIMEOUT on the CI hardware now.

@bors-servo r=jdm

bors-servo · 2017-02-16T15:30:33Z

📌 Commit 3ad8a6d has been approved by jdm

bors-servo · 2017-02-16T16:11:36Z

⌛ Testing commit 3ad8a6d with merge 0740e20...

…ow, r=jdm Allow browsing contexts to resolve to cross-origin windows  This PR implements cross-thread `WindowProxy` objects. At the moment, if a `Window` performs a non-similar-origin navigation, the old script thread does not update its `WindowProxy`, since the new `Window` is in the new script thread. With this PR, the `WindowProxy` is updated to a dummy `XOriginWindow` object, that only implements the whitelisted methods that are allowed to be called cross-origin. This PR does not include working implementations of some of the cross-origin `Window` or `Location` methods. This PR causes some cross-origin wpt tests to now pass, in particular `/html/browsers/origin/cross-origin-objects/cross-origin-objects.html ` now passes `Only whitelisted properties are accessible cross-origin`. There are some CORS failures in `fetch`, I suspect caused by the incorrect setting of `origin` in fetch requests. Although there are some functions that now throw `SecurityException`, it is not meant to be a complete implementation, which will have to wait for XOWs to land. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15180. - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15358)

bors-servo · 2017-02-16T17:01:03Z

💔 Test failed - mac-rel-wpt2

asajeffrey · 2017-02-16T22:06:34Z

Hmm:

  ▶ ERROR [expected OK] /workers/Worker_ErrorEvent_error.htm
  └   → uncaught exception: hello

 ▶ Unexpected subtest result in /_mozilla/mozilla/cross-origin-objects/cross-origin-objects.html:
  └ PASS [expected FAIL] Parentage of cross-origin windows

  ▶ Unexpected subtest result in /_mozilla/mozilla/cross-origin-objects/cross-origin-objects.html:
  │ FAIL [expected PASS] Only whitelisted properties are accessible cross-origin
  │   → assert_throws: Should throw when writing to self on Window function "function () { C[prop] = undefined; }" did not throw
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/cross-origin-objects/cross-origin-objects.html:72:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1433:20

The first one is #14775, the second is caused by the test running while C is same-origin, when it should be dissimilar-origin. This may be a timing issue, caused by reacting to the load event from the initial about:blank. I'll try rewriting the test and running it again.

asajeffrey · 2017-02-16T22:46:19Z

OK, I rewrote the test to be more robust against timing variations. @bors-servo r=jdm

bors-servo · 2017-02-16T22:46:20Z

📌 Commit e8d7655 has been approved by jdm

bors-servo · 2017-02-17T00:57:07Z

⌛ Testing commit e8d7655 with merge 6adbcb4...

…ow, r=jdm Allow browsing contexts to resolve to cross-origin windows  This PR implements cross-thread `WindowProxy` objects. At the moment, if a `Window` performs a non-similar-origin navigation, the old script thread does not update its `WindowProxy`, since the new `Window` is in the new script thread. With this PR, the `WindowProxy` is updated to a dummy `XOriginWindow` object, that only implements the whitelisted methods that are allowed to be called cross-origin. This PR does not include working implementations of some of the cross-origin `Window` or `Location` methods. This PR causes some cross-origin wpt tests to now pass, in particular `/html/browsers/origin/cross-origin-objects/cross-origin-objects.html ` now passes `Only whitelisted properties are accessible cross-origin`. There are some CORS failures in `fetch`, I suspect caused by the incorrect setting of `origin` in fetch requests. Although there are some functions that now throw `SecurityException`, it is not meant to be a complete implementation, which will have to wait for XOWs to land. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15180. - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15358)

bors-servo · 2017-02-17T02:16:24Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: jdm
Pushing 6adbcb4 to master...

asajeffrey added the A-content/script Related to the script thread label Feb 3, 2017

highfive assigned Manishearth Feb 3, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 3, 2017

highfive assigned jdm and unassigned Manishearth Feb 3, 2017

Ms2ger reviewed Feb 3, 2017

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Feb 7, 2017

asajeffrey force-pushed the script-browsingcontext-xorigin-window branch from ee3c42c to df7c985 Compare February 7, 2017 22:01

asajeffrey added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-needs-rebase There are merge conflict errors. labels Feb 7, 2017

jdm requested changes Feb 8, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 8, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 9, 2017

asajeffrey mentioned this pull request Feb 9, 2017

Cross-origin windows instanceof Window #15476

Open

jdm requested changes Feb 9, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 9, 2017

highfive removed the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Feb 9, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Feb 15, 2017

asajeffrey force-pushed the script-browsingcontext-xorigin-window branch from 1f11bf8 to 7153880 Compare February 16, 2017 15:20

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Feb 16, 2017

asajeffrey force-pushed the script-browsingcontext-xorigin-window branch 2 times, most recently from 809ecd4 to 3ad8a6d Compare February 16, 2017 15:28

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 16, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Feb 16, 2017

Allow browsing contexts to resolve to cross-origin windows.

e8d7655

asajeffrey force-pushed the script-browsingcontext-xorigin-window branch from 3ad8a6d to e8d7655 Compare February 16, 2017 22:45

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Feb 16, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 16, 2017

bors-servo merged commit e8d7655 into servo:master Feb 17, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 17, 2017

Allow browsing contexts to resolve to cross-origin windows #15358

Allow browsing contexts to resolve to cross-origin windows #15358

Conversation

asajeffrey commented Feb 3, 2017 • edited by larsbergstrom

highfive commented Feb 3, 2017

highfive commented Feb 3, 2017

asajeffrey commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Feb 7, 2017

jdm commented Feb 8, 2017

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey commented Feb 9, 2017

jdm commented Feb 9, 2017

jgraham commented Feb 9, 2017

bzbarsky commented Feb 9, 2017

asajeffrey commented Feb 9, 2017

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey commented Feb 16, 2017

asajeffrey commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

asajeffrey commented Feb 16, 2017 • edited

asajeffrey commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 17, 2017

bors-servo commented Feb 17, 2017

asajeffrey commented Feb 3, 2017 •

edited by larsbergstrom

asajeffrey commented Feb 16, 2017 •

edited