Continue message port #23637

gterzian · 2019-06-26T16:31:32Z

Fixes #7457.
Fixes #12715.
Fixes #12717.
Fixes #16095.
Fixes #18969.

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

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

This change is

highfive · 2019-06-26T16:31:37Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/messagechannel.rs, components/script/dom/messageport.rs, components/script/dom/mod.rs, components/script/dom/dissimilaroriginwindow.rs, components/script/dom/window.rs and 28 more
@cbrewster: components/constellation/Cargo.toml, components/constellation/constellation.rs
@jgraham: tests/wpt/include.ini
@paulrouget: components/constellation/Cargo.toml, components/constellation/constellation.rs
@KiChjang: components/script/dom/messagechannel.rs, components/script/dom/messageport.rs, components/script/dom/mod.rs, components/script/dom/dissimilaroriginwindow.rs, components/script/dom/window.rs and 28 more

highfive · 2019-06-26T16:31:42Z

Warning

These commits modify unsafe code. Please review it carefully!

gterzian · 2019-06-26T16:34:36Z

Still an early sketch. cc @KiChjang @jdm

Messages are still routed via the constellation, not directly script to script, and I haven't actually run it yet, just trying to have a rough sketch that compiles.

One question @KiChjang do I understand it correctly that transfer_receive will be called once for every object in the initial transfer: CustomAutoRooterGuard<Option<Vec<*mut JSObject>>>?

gterzian · 2019-06-26T16:47:37Z

components/constellation/Cargo.toml

@@ -19,7 +19,7 @@ no_wgl = ["canvas/no_wgl"]
 background_hang_monitor = { path = "../background_hang_monitor"}
 backtrace = "0.3"
 bluetooth_traits = { path = "../bluetooth_traits" }
-canvas = {path = "../canvas", default-features = false}
+canvas = {path = "../canvas", default-features = true}


note: remove change

bors-servo · 2019-06-29T20:31:01Z

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

gterzian · 2019-06-30T18:57:02Z

@jdm @KiChjang Ok so at this point, I almost dare to say that it's "working".

I don't think I'll have time during the week to make progress, so it might be worth it if anyone has time to review the current work-in-progress(or not and I'll just pick things up later).

Some highlights:

Same-origin and cross-origin transfer and messaging seems to work(with script-to-script ipc).
There are quite a few tests we're not passing yet "as is", because we're not implementing all requires arguments to postMessage, and other things like not setting source of MessageEvent to be a cross-origin window proxy in some cases, which the tests are using. But I've done some manual testing by adapting some tests and the base case of transferring and messaging appear to work.
I switched to using the GlobalScope as the place to write the glue between ports and the constellation. I think it's a good idea because that means it will work for workers as well. It's important because even though we don't have to use an actual message ports for the "implicit port" of dedicated workers, we still have to allow for ports to be transferred into them via worker.postMessage. On that regards we're not implementing the transfer argument yet, so I haven't tested it, but in theory it should work.
I kept using the structured transfer API, if only because it's actually quite nice as a way to hide the transfer from the various postMessage clients. I've been able to write across a single MessagePortId, and I'd like to find a way to write across a second one for the entangled port(there is one content pointer which might be useful, but just assigning the id to a raw pointer gives weird results on the other side, but maybe de-constructing the id in a u64 like is already could work with the void pointer as well).
It turns out that using MessagePortId in workers requires a PipelineNameSpace to have been installed for their thread, I currently do this with a blocking call to the constellation when the worker starts...
There's a lot of code that can't quite see the light of day, and lots of notes that probably only make sense to me, please bear with me.
An entangled port and constellation share a sender to the other port, and a message type, wondering if those should be separated.

gterzian · 2019-07-03T11:20:37Z

@bors-servo try=wpt Let's see what tests have been affected outside of webmessaging.

bors-servo · 2019-07-03T11:20:41Z

⌛ Trying commit 208c4d3 with merge 9ed8848...


        Auto merge of #23637 - gterzian:continue-message-port, r=<try>

[WIP] Continue message port  Fixes #7457. Fixes #12715. Fixes #12717. Fixes #16095. Fixes #18969. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] 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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23637)

bors-servo · 2019-07-03T11:46:52Z

💔 Test failed - linux-rel-css

gterzian · 2019-07-03T11:51:34Z

One thing that is left to do for sure is setting the windowproxy of the source of the message on the message event, because a lot of test rely on that in sending some sort of acknowledgement back.

EDIT: the problem was we were using the origin of the window receiving the message, instead of the one sending the message, in the origin field of message event.

gterzian · 2019-07-03T11:55:11Z

"If this is failing: DANGER, are you sure you want to expose the new interface MessagePort to all webpages as a property on the global? Do not make a change to this file without review from jdm or Ms2ger for that specific change!"

jdm · 2019-07-04T18:23:34Z

For the record, I am about 1/3 of the way through reviewing the changes so far.

bors-servo · 2019-07-05T05:31:48Z

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

gterzian · 2019-07-05T07:09:55Z

@bors-servo try=wpt (let's get an update on the rest of the suite)


        begin messageport, transferable objects, impl

Accept transfer argument for StructuredCloneData::write Allow structured clone reads to return a boolean Add Transferable trait Add basic skeletons to MessagePort Implement transfer and transfer-receiving steps on MessagePort Use transfer and transfer_receive in StructuredClone callbacks Implement MessageChannel Freeze the array object for the MessageEvent ports attribute Implement transfer argument on window.postMessage Use ReentrantMutex instead for MessagePortInternal Accept origin as a parameter in dispatch_jsval Fix BorrowMut crash with pending_port_message Detach port on closure and check for detached during transfer Enable webmessaging tests fix webidl fix


        continue messageport, transferable, postmessage options

servo-wpt-sync · 2019-10-19T06:30:36Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#17663.

gterzian · 2019-10-19T06:30:58Z

Ok so this needed a rebase, and I've also added a re-routing of the task if the global is not managing any ports, at https://github.com/servo/servo/pull/23637/files#diff-59d233642d0ce6d687484bdd009e1017R520

@jdm r?

jdm · 2019-10-19T13:05:52Z

@bors-servo r+

bors-servo · 2019-10-19T13:05:53Z

📌 Commit 2f8932a has been approved by jdm

bors-servo · 2019-10-19T13:05:59Z

⌛ Testing commit 2f8932a with merge c466b40...


        Auto merge of #23637 - gterzian:continue-message-port, r=jdm

Continue message port  Fixes #7457. Fixes #12715. Fixes #12717. Fixes #16095. Fixes #18969. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] 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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23637)

bors-servo · 2019-10-19T15:18:01Z

💔 Test failed - status-taskcluster

jdm · 2019-10-19T15:45:49Z

@bors-servo retry

bors-servo · 2019-10-19T17:19:06Z

⌛ Testing commit 2f8932a with merge a905916...


        Auto merge of #23637 - gterzian:continue-message-port, r=jdm

Continue message port  Fixes #7457. Fixes #12715. Fixes #12717. Fixes #16095. Fixes #18969. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] 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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23637)

bors-servo · 2019-10-19T19:13:18Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing a905916 to master...

highfive assigned jdm Jun 26, 2019

highfive added the S-awaiting-review label Jun 26, 2019

gterzian force-pushed the gterzian:continue-message-port branch 3 times, most recently from 7f30795 to a9f3a5a Jun 26, 2019

gterzian reviewed Jun 26, 2019

View changes

jdm removed the S-awaiting-review label Jun 28, 2019

gterzian force-pushed the gterzian:continue-message-port branch from 0b4c36a to 9a50c92 Jun 28, 2019

highfive added the S-awaiting-review label Jun 28, 2019

highfive added the S-needs-rebase label Jun 29, 2019

gterzian force-pushed the gterzian:continue-message-port branch from d2d3bae to 35de5eb Jun 30, 2019

gterzian mentioned this pull request Jul 2, 2019

Cross-origin Iframe postMessage question #23689

Closed

highfive added the S-tests-failed label Jul 3, 2019

gterzian force-pushed the gterzian:continue-message-port branch from 208c4d3 to 29ce72d Jul 3, 2019

highfive removed the S-tests-failed label Jul 3, 2019

gterzian force-pushed the gterzian:continue-message-port branch from 11a3fcb to 4a9795a Jul 4, 2019

KiChjang and others added 2 commits Dec 11, 2016

continue messageport, transferable, postmessage options

Loading status checks…

2f8932a

gterzian force-pushed the gterzian:continue-message-port branch from 9d7b0cd to 2f8932a Oct 19, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Oct 19, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 19, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 19, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 19, 2019

bors-servo merged commit 2f8932a into servo:master Oct 19, 2019
2 checks passed

2 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 19, 2019

This was referenced Oct 19, 2019

[WIP] Fix script crash on handling new layout #23761

Closed

Support type=module script element #23545

Merged

gterzian deleted the gterzian:continue-message-port branch Oct 20, 2019

jdm mentioned this pull request Oct 23, 2019

Pass source origin in MessageEvent. #24526

Closed

4 of 5 tasks complete

gterzian mentioned this pull request Oct 24, 2019

Use origin of source document for MessageEvent resulting from postMessage #22888

Closed

atouchet mentioned this pull request Nov 6, 2019

Implement MessagePort and MessageChannel #16622

Closed

servo / servo

Continue message port #23637

Continue message port #23637

gterzian commented Jun 26, 2019 •

edited by SimonSapin

highfive commented Jun 26, 2019

highfive commented Jun 26, 2019

gterzian commented Jun 26, 2019

This comment has been minimized.

bors-servo commented Jun 29, 2019

gterzian commented Jun 30, 2019 •

edited

gterzian commented Jul 3, 2019

bors-servo commented Jul 3, 2019

bors-servo commented Jul 3, 2019

gterzian commented Jul 3, 2019 •

edited

gterzian commented Jul 3, 2019 •

edited

jdm commented Jul 4, 2019

bors-servo commented Jul 5, 2019

gterzian commented Jul 5, 2019

servo-wpt-sync commented Oct 19, 2019

gterzian commented Oct 19, 2019

jdm commented Oct 19, 2019

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

jdm commented Oct 19, 2019 •

edited

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

servo / servo

Join GitHub today

Continue message port #23637

Continue message port #23637

Conversation

gterzian commented Jun 26, 2019 • edited by SimonSapin

highfive commented Jun 26, 2019

highfive commented Jun 26, 2019

gterzian commented Jun 26, 2019

This comment has been minimized.

gterzian Jun 26, 2019

bors-servo commented Jun 29, 2019

gterzian commented Jun 30, 2019 • edited

gterzian commented Jul 3, 2019

bors-servo commented Jul 3, 2019

bors-servo commented Jul 3, 2019

gterzian commented Jul 3, 2019 • edited

gterzian commented Jul 3, 2019 • edited

jdm commented Jul 4, 2019

bors-servo commented Jul 5, 2019

gterzian commented Jul 5, 2019

servo-wpt-sync commented Oct 19, 2019

gterzian commented Oct 19, 2019

jdm commented Oct 19, 2019

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

jdm commented Oct 19, 2019 • edited

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

gterzian commented Jun 26, 2019 •

edited by SimonSapin

gterzian commented Jun 30, 2019 •

edited

gterzian commented Jul 3, 2019 •

edited

gterzian commented Jul 3, 2019 •

edited

jdm commented Oct 19, 2019 •

edited