Implement cross-thread postMessage #15679

asajeffrey · 2017-02-21T22:31:58Z

This PR implements cross-thread postMessage,

It builds on #15438 and #15478, only the last commit is part of this PR.

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

This change is

highfive · 2017-02-21T22:32:03Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/servoparser/mod.rs, components/script/dom/dissimilaroriginwindow.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/location.rs, components/script/dom/document.rs, components/script/dom/browsingcontext.rs, components/script/dom/xmlhttprequest.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/devtools.rs, components/script/dom/webidls/DissimilarOriginWindow.webidl, components/script/dom/history.rs, components/script/webdriver_handlers.rs, components/script/lib.rs, components/script/dom/domparser.rs, components/script/script_thread.rs, components/script/dom/domimplementation.rs, components/script/dom/htmliframeelement.rs, components/script/dom/webidls/Location.webidl, components/script/dom/node.rs, components/script/dom/xmldocument.rs, components/script/dom/bindings/trace.rs, components/script/origin.rs
@KiChjang: components/script/dom/servoparser/mod.rs, components/script/dom/dissimilaroriginwindow.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/location.rs, components/script/dom/document.rs, components/net/http_loader.rs, components/script/dom/browsingcontext.rs, components/script/dom/xmlhttprequest.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/devtools.rs, components/script/dom/webidls/DissimilarOriginWindow.webidl, components/script/dom/history.rs, components/script/webdriver_handlers.rs, components/script/lib.rs, components/script/dom/domparser.rs, components/script/script_thread.rs, components/script/dom/domimplementation.rs, components/script/dom/htmliframeelement.rs, components/net_traits/pub_domains.rs, components/net_traits/pub_domains.rs, components/script/dom/webidls/Location.webidl, components/script/dom/node.rs, components/script/dom/xmldocument.rs, components/script/dom/bindings/trace.rs, components/script/origin.rs, components/net_traits/request.rs, components/net_traits/request.rs

highfive · 2017-02-21T22:32:04Z

Warning

These commits modify unsafe code. Please review it carefully!

asajeffrey · 2017-02-21T22:32:17Z

cc @jdm

bors-servo · 2017-02-22T18:33:45Z

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

bors-servo · 2017-02-23T02:43:18Z

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

bors-servo · 2017-03-15T16:17:22Z

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

asajeffrey · 2017-03-15T19:11:29Z

@emilio #15536 landed, this is now ready for review!

emilio

r=me

emilio · 2017-03-15T19:18:56Z

components/constellation/constellation.rs

@@ -1795,6 +1799,21 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        }
    }

+    fn handle_post_message_msg(&mut self, frame_id: FrameId, origin: Option<ImmutableOrigin>, data: Vec<u8>) {
+        let pipeline_id = match self.frames.get(&frame_id) {
+            None => return warn!("postMessage to closed frame {}.", frame_id),


(As a side note, I seriously believe that we should convert all these to panics during testing).

That's an interesting idea. These warnings can happen during correct execution (e.g. post a message to an iframe's content window after it's been removed from the frame tree) but I don't think that should come up in wpt testing. File an issue?

emilio · 2017-03-15T19:29:43Z

tests/wpt/mozilla/tests/mozilla/cross-origin-objects/cross-origin-postMessage-child1.html

@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<head>


A lot of the tags here (and in the other tests) can go away and would make this a bit less noisy, but your choice :)

I like my html old skool :)

emilio · 2017-03-15T19:31:00Z

tests/wpt/mozilla/tests/mozilla/cross-origin-objects/cross-origin-postMessage-child1.html

+<body>
+<script>
+  window.addEventListener("message", function(e) {
+    window.location.href = e.data;


heh. this is quite tricky. I guess it works :)

I guess so :)

emilio · 2017-03-15T19:31:45Z

tests/wpt/mozilla/tests/mozilla/cross-origin-objects/cross-origin-postMessage.html

+    // child1 is a dissimilar-origin document
+    var childURL1 = new URL("cross-origin-postMessage-child1.html", document.location);
+    childURL1.hostname = "127.0.0.1";
+    // child2 is a same-orogin document


typo: origin.

asajeffrey

Thanks for the quick review!

asajeffrey · 2017-03-15T19:41:25Z

components/constellation/constellation.rs

@@ -1795,6 +1799,21 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        }
    }

+    fn handle_post_message_msg(&mut self, frame_id: FrameId, origin: Option<ImmutableOrigin>, data: Vec<u8>) {
+        let pipeline_id = match self.frames.get(&frame_id) {
+            None => return warn!("postMessage to closed frame {}.", frame_id),


That's an interesting idea. These warnings can happen during correct execution (e.g. post a message to an iframe's content window after it's been removed from the frame tree) but I don't think that should come up in wpt testing. File an issue?

asajeffrey · 2017-03-15T19:41:41Z

tests/wpt/mozilla/tests/mozilla/cross-origin-objects/cross-origin-postMessage-child1.html

@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<head>


I like my html old skool :)

asajeffrey · 2017-03-15T19:42:29Z

tests/wpt/mozilla/tests/mozilla/cross-origin-objects/cross-origin-postMessage-child1.html

+<body>
+<script>
+  window.addEventListener("message", function(e) {
+    window.location.href = e.data;


I guess so :)

asajeffrey · 2017-03-15T19:42:45Z

tests/wpt/mozilla/tests/mozilla/cross-origin-objects/cross-origin-postMessage.html

+    // child1 is a dissimilar-origin document
+    var childURL1 = new URL("cross-origin-postMessage-child1.html", document.location);
+    childURL1.hostname = "127.0.0.1";
+    // child2 is a same-orogin document


asajeffrey · 2017-03-15T19:59:12Z

@bors-servo r=emilio

bors-servo · 2017-03-15T19:59:13Z

📌 Commit f9c5d0c has been approved by emilio

bors-servo · 2017-03-15T19:59:16Z

⌛ Testing commit f9c5d0c with merge 1caf8a7...

Implement cross-thread postMessage  This PR implements cross-thread postMessage, It builds on #15438 and #15478, only the last commit is part of this PR. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [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/15679)

bors-servo · 2017-03-15T20:36:34Z

☀️ 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: emilio
Pushing 1caf8a7 to master...

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

highfive assigned emilio Feb 21, 2017

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

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

asajeffrey force-pushed the script-postMessage-xorigin branch from 6ea3d04 to 53043b3 Compare February 22, 2017 19:18

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

asajeffrey force-pushed the script-postMessage-xorigin branch from 53043b3 to 73acf86 Compare February 23, 2017 14:34

asajeffrey added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Feb 27, 2017

asajeffrey force-pushed the script-postMessage-xorigin branch from 73acf86 to 72cd111 Compare February 27, 2017 21:19

asajeffrey removed the S-needs-rebase There are merge conflict errors. label Feb 27, 2017

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

asajeffrey force-pushed the script-postMessage-xorigin branch from 72cd111 to f560c80 Compare March 15, 2017 19:10

emilio approved these changes Mar 15, 2017

View reviewed changes

asajeffrey commented Mar 15, 2017

View reviewed changes

Implement dissimilar-origin postMessage.

f9c5d0c

asajeffrey force-pushed the script-postMessage-xorigin branch from f560c80 to f9c5d0c Compare March 15, 2017 19:57

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. S-needs-rebase There are merge conflict errors. labels Mar 15, 2017

emilio mentioned this pull request Mar 15, 2017

Run tests with the constellation panicking instead of warning. #15968

Open

bors-servo merged commit f9c5d0c into servo:master Mar 15, 2017

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

bors-servo mentioned this pull request Mar 15, 2017

Implement dissimilar-origin window.parent and window.top #15799

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cross-thread postMessage #15679

Implement cross-thread postMessage #15679

asajeffrey commented Feb 21, 2017 •

edited by larsbergstrom

highfive commented Feb 21, 2017

highfive commented Feb 21, 2017

asajeffrey commented Feb 21, 2017

bors-servo commented Feb 22, 2017

bors-servo commented Feb 23, 2017

bors-servo commented Mar 15, 2017

asajeffrey commented Mar 15, 2017

emilio left a comment

emilio Mar 15, 2017

asajeffrey Mar 15, 2017

emilio Mar 15, 2017

asajeffrey Mar 15, 2017

emilio Mar 15, 2017

asajeffrey Mar 15, 2017

emilio Mar 15, 2017

asajeffrey Mar 15, 2017

asajeffrey left a comment

asajeffrey Mar 15, 2017

asajeffrey Mar 15, 2017

asajeffrey Mar 15, 2017

asajeffrey Mar 15, 2017

asajeffrey commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bors-servo commented Mar 15, 2017

Implement cross-thread postMessage #15679

Implement cross-thread postMessage #15679

Conversation

asajeffrey commented Feb 21, 2017 • edited by larsbergstrom

highfive commented Feb 21, 2017

highfive commented Feb 21, 2017

asajeffrey commented Feb 21, 2017

bors-servo commented Feb 22, 2017

bors-servo commented Feb 23, 2017

bors-servo commented Mar 15, 2017

asajeffrey commented Mar 15, 2017

emilio 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

asajeffrey 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

asajeffrey commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bors-servo commented Mar 15, 2017

bors-servo commented Mar 15, 2017

asajeffrey commented Feb 21, 2017 •

edited by larsbergstrom