Embedder handling of prompts and alerts #20707

gterzian · 2018-04-28T14:53:25Z

Allow embedder to allow alerts and prompts, but via constellation forwarding messages from script.

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

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

This change is

highfive · 2018-04-28T14:53:29Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@cbrewster: components/constellation/constellation.rs
@paulrouget: components/compositing/compositor_thread.rs, ports/servo/browser.rs, components/constellation/constellation.rs

gterzian · 2018-04-28T15:00:19Z

@paulrouget I ended up in circular dependency hell when trying to use compositing from script_traits, tried moving the EmbedderProxy from "compositing" to "embedder_traits", experienced similar problems(see for the previous design https://github.com/servo/servo/pull/20706/files).

It seemed to me that to have script communicate directly with the embedder, we would need to import at the very least EmbedderProxy in script_traits, and either add the embedder proxy to InitialScriptState, or just pass it as a third argument to ScriptThreadFactory::create.

Unfortunately I wasn't able to resolve the circular dependency issue, so I tried it in this way instead. Is there a problem with having an extra step go via the constellation?

paulrouget · 2018-04-30T08:01:47Z

tried moving the EmbedderProxy from "compositing" to "embedder_traits"

Can you expand on that?

I introduced embedder_traits in the hope that we could move all embedder messages there.

gterzian · 2018-04-30T08:39:27Z

As I remember it, I tried to quickly move EmbedderProxy and EmbedderMsg into embedder_traits, while still leaving EventloopWaker in compositing, which would create another cyclic dependency. But I haven't tried yet to move EventloopWaker into embedder_traits and use that from compositing.

I'll give it a try and let you know...

gterzian · 2018-04-30T11:22:41Z

@paulrouget Ok, looks like it's going to work to move everything to embedder_traits...

gterzian · 2018-04-30T12:39:54Z

I only had to adapt HistoryChanged because using script_traits from embedder_traits creates a cyclic dep... See last commit...

jdm · 2018-04-30T13:00:11Z

It's not clear to me how this compiles, because InitialScriptState has a member added to it but UnprivilegedPipelineContent::start_all wasn't modified. In particular, I expect we'll need a different mechanism for the event loop waker in the multiprocess case, since we don't have direct access to the relevant event loop. Probably an IPC channel with a dedicated listener that contains an actual event loop waker it can poke would do the trick.

gterzian · 2018-04-30T16:31:34Z

@jdm, as a matter of fact, it wasn't :) and it still fails at https://github.com/gterzian/servo/blob/c4dc72ed07acb471829004e5ada3c762900684c1/components/servo/lib.rs#L600 (the reason I'm passing it as an argument to start_all is just to avoid the need to (de)serialize if it were added to UnprivilegedPipelineContent, at least for now)

Couple of questions:

How would I 'wake up the event loop' of the content process? IpcSender::connect(token)?
It seems there is no embedder in the content process? If so, should we add some sort of dummy embedder that would listens on for EmebedderMsg, and respond on channels when necessary so as not to block script?
More in general, at which point in the content process part is the constellation created? I don't seem to get the flow of that one...

paulrouget · 2018-04-30T16:49:29Z

The content process doesn't have any constellation code. It's only layout/script threads running in there if I'm not mistaken.

So when the script thread of the content process needs to talk back to the embedder, it needs a way to make it so the embedder wakes up. The embedder proxy would call the wakeup function on send, making sure the embedder would wake up to consume the new message. This mechanism can be shared between the different threads of Servo (see the clone function that we use for that in the waker trait), but we can't clone the wakeup mechanism between processes. So you need something that would listen that wakeup event in the main process that would then wakeup the embedder. And that should be the constellation probably. But then, maybe, we just end up back with the current setup where script talk to constellation which talks to embedder.

cbrewster · 2018-04-30T17:31:47Z

IRC Chat: https://mozilla.logbot.info/servo/20180430#c14687758-c14687762

The main concern with having embedder messages being routed through the constellation was that we were adding a new message to the constellation for each new message added to the embedder. Rather than having each script thread have an embedder sender (since this appears to be more complex than originally thought), we should just add a new constellation message that takes an embedder message and forwards the embedder message to the embedder.

paulrouget · 2018-05-01T03:21:49Z

If we end up introducing a Compositor message to forward embedder messages, we might want to change components/net/filemanager_thread.rs and components/bluetooth/lib.rs. We recently started using the embedderproxy there.

gterzian · 2018-05-01T04:45:25Z

Thanks for the input. I have two ideas, both of which are perhaps nonsense, for consideration:

First of all, just to make sure I understand, there is no embedder or constellation, in the content process case? If that is indeed the case, I can think of two options:

In all cases, we rout all embedder messages via the constellation using a new ScriptMsg::Forward(EmbedderMsg), like @cbrewster suggested. In the content process case, perhaps we can 'fake' receiving and forwarding/handling of these messages, similar to what seems to be done with logging ?
We go the way of using embedder proxy inside script. In the content process case, this requires passing an appropriate EmbedderProxy here(basically the last thing needed for the current code to compile, I think).

It seems to be, that in both options, we will still need some sort of 'dummy' embedder for the content process case?

Do we need a EventLoopWaker under 1, or would it be enough to just listen in on the script_to_constellation_chan and handle the embedder messages coming in throught there(which normally would be forwarded to the embedder, and now would need to be handled by some dummy one?)

For option 2, Would an appropriate EventLoopWaker, As @jdm suggested earlier, perhaps be an ipc::channel, where the sending part would be held by the EventLoopWaker, and the receiving part listening from run_content_process and 'waking up' a dummy embedder to get messages via the embedder receiver?

gterzian · 2018-05-01T13:34:06Z

So, at the risk of driving everyone crazy, I'm going to share my view on what's happening with the constellation and the embedder when we're running a content process:

it all starts with a 'normal' run of servo, but with opts::multiprocess() being true.
The constellation, embedder and all, the Browser, are started normally in servo/main.rs#L176
When a new pipeline is created by the constellation, when we get to constellation/pipeline.rs#L291, a new servo 'process' is started, with the command line argument --content-process, and we also start a multiprocess 'server' that will allow the main process to communicate with the sub process about to be created.
In this new 'sub' process, when we get to servo/main.rs#L153, the constellation/browser initialization steps are skipped, and instead we jump to servo::run_content_process.
Inside servo::run_content_process, we connect to the multiprocess 'server' setup by UnprivilegedPipelineContent as part of 3 above.
We then receive the UnprivilegedPipelineContent that was initially created under 3, in the 'main process'.
Finally, in this sub process, we call UnprivilegedPipelineContent::start_all, resulting in a script thread running inside this subprocess, detached from the main process where the constellation, the embedder, and the compositor, are still running.

So at that point, in order to 'wake up the event loop', we need to wake the glutin event loop of the 'main process' that started at 1 above, not the event loop of the sub process we are in at that point.
Note that we're receiving the UnprivilegedPipelineContent that was created as part of this main process, so that's how we might be able to send along a channel to communicate back to the main process, or something like that.

Makes sense?

jdm · 2018-05-01T13:58:42Z

Yes, that is the idea I was opaquely proposing in my #20707 (comment).

gterzian · 2018-05-02T04:50:20Z

Please take a look, given everyone's input I think the most simple solution is re-using the script_to_constellation_chan and adding a ScriptMsg::Forward(EmbedderMsg).

If this makes sense, I'll move over the remaining ScriptMsg that fit in this pattern, for example FromScriptMsg::NewFavicon(url) and FromScriptMsg::HeadParsed(I think source_is_top_level_pipeline can be determined via the WindowProxy inside script)...

gterzian · 2018-05-02T04:52:11Z

ports/servo/browser.rs

+                EmbedderMsg::Alert(browser_id, _message, response_chan) => {
+                    if let Err(e) = response_chan.send(true) {
+                        warn!("Failed to send alert response: {}", e);
+                        self.event_queue.push(WindowEvent::CloseBrowser(browser_id));


@paulrouget Previously the constellation would panic the pipeline if sending failed, this was the closest I got to...

I see that we used to call self.handle_send_error(pipeline_id, e). Having a mechanism to report a communication error with the pipeline, from the embedder, sounds important

@asajeffrey, we are introducing a communication between the embedder and the pipeline. Any recommendation how we could handle a send error? Should we let the constellation now or handle it in the embedder code?

paulrouget

Why not moving display_alert_dialog from window.rs to ports/?

paulrouget · 2018-05-02T05:41:17Z

ports/servo/browser.rs

+                EmbedderMsg::Alert(browser_id, _message, response_chan) => {
+                    if let Err(e) = response_chan.send(true) {
+                        warn!("Failed to send alert response: {}", e);
+                        self.event_queue.push(WindowEvent::CloseBrowser(browser_id));


I see that we used to call self.handle_send_error(pipeline_id, e). Having a mechanism to report a communication error with the pipeline, from the embedder, sounds important

@asajeffrey, we are introducing a communication between the embedder and the pipeline. Any recommendation how we could handle a send error? Should we let the constellation now or handle it in the embedder code?

paulrouget · 2018-05-02T05:41:17Z

components/script_traits/script_msg.rs

@@ -69,6 +70,8 @@ pub enum LogEntry {
 /// Messages from the script to the constellation.
 #[derive(Deserialize, Serialize)]
 pub enum ScriptMsg {
+    /// Forward a message to the embedder.
+    Forward(EmbedderMsg),


Can you make that message more explicit? ForwardToEmbedder maybe?

gterzian · 2018-05-02T06:34:23Z

Ok, after rebasing, now got a cyclic dependency on net_traits, I think due to use embedder_traits::resources::register_resources_for_tests; in net_traits::tests::pub_domain(and embedder_traits using net_traits::filemanager_thread::FilterPattern;), maybe switch to sending a Vec<String> instead of Vec<FilterPattern>?

Edit: done, see last commit...

paulrouget · 2018-05-02T07:40:38Z

components/script/lib.rs

@@ -93,7 +93,7 @@ extern crate style;
 extern crate style_traits;
 extern crate swapper;
 extern crate time;
-#[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))]
+#[cfg(any(target_os = "linux"))]


any is not useful here.

paulrouget · 2018-05-02T07:40:38Z

ports/servo/browser.rs

+#[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))]
+fn display_alert_dialog(message: &str) {
+    if !opts::get().headless {
+        tinyfiledialogs::message_box_ok("Alert!", message, MessageBoxIcon::Warning);


You probably want to run that in a different thread. See platform_get_selected_devices for example.

paulrouget · 2018-05-02T07:40:38Z

components/script/dom/window.rs

+        let window_proxy = self.window_proxy.get().unwrap();
+        let top_level_browsing_context_id = window_proxy.top_level_browsing_context_id();
+        self.send_to_constellation(ScriptMsg::ForwardToEmbedder(EmbedderMsg::Alert(top_level_browsing_context_id,
+                                                                                   s.to_string())));


Aren't we supposed to be blocking here? I believe you need a sender/receiver pair, and block on recv(). In ports/, you would use the sender to notify when the user dismissed the alert.

paulrouget · 2018-05-02T07:40:38Z

ports/servo/browser.rs

-        let filter_ref = &(filter.iter().map(|s| s.as_str()).collect::<Vec<&str>>()[..]);
-        let filter_opt = if filter.len() > 0 { Some((filter_ref, "")) } else { None };
+        let filter_ref = &(filters.iter().map(|s| s.as_str()).collect::<Vec<&str>>()[..]);
+        let filter_opt = if filters.len() > 0 { Some((filter_ref, "")) } else { None };


Could we use FilterPattern here if we move its definition to the embedder_traits? It's a bit annoying that we have to move that logic (FilterPattern to string) in filemanager_thread.rs. Feels like the embedder code is a better place. I'm not sure what's the best approach here.

jdm · 2018-05-24T12:03:24Z

error: unused variable: `title`
   --> ports/servo/browser.rs:355:18
    |
355 | fn get_url_input(title: &str, url: &str) -> Option<String> {
    |                  ^^^^^ help: consider using `_title` instead
    |
    = note: `-D unused-variables` implied by `-D warnings`

error: unused variable: `url`
   --> ports/servo/browser.rs:355:31
    |
355 | fn get_url_input(title: &str, url: &str) -> Option<String> {
    |                               ^^^ help: consider using `_url` instead

error: aborting due to 2 previous errors

error: Could not compile `servo`.

So close!


        fix url input for android

gterzian · 2018-05-24T13:11:42Z

fingers crossed

jdm · 2018-05-24T13:39:57Z

@bors-servo r=payl

bors-servo · 2018-05-24T13:40:03Z

📌 Commit a5dc1b6 has been approved by payl

bors-servo · 2018-05-24T14:34:50Z

⌛ Testing commit a5dc1b6 with merge f6762d5...


        Auto merge of #20707 - gterzian:embedder_handling_of_prompts_and_aler…

…ts_2, r=payl Embedder handling of prompts and alerts  Allow embedder to allow alerts and prompts, but via constellation forwarding messages from script. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach build-geckolib` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #19992 (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/20707)

bors-servo · 2018-05-24T14:58:39Z

💔 Test failed - linux-rel-css

jdm · 2018-05-24T15:09:40Z

@bors-servo retry

#20856

gterzian · 2018-05-24T15:10:51Z

{"status": "FAIL", "group": "default", "message": "assert_true: expected true got false", "stack": "window.onload/<@http://web-platform.test:8000/html/browsers/history/the-history-interface/007.html:32:5\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20\ntest@http://web-platform.test:8000/resources/testharness.js:548:9\nwindow.onload@http://web-platform.test:8000/html/browsers/history/the-history-interface/007.html:31:3\n", "subtest": "popstate event should fire before onload fires", "test": "/html/browsers/history/the-history-interface/007.html", "line": 156100, "action": "test_result", "expected": "PASS"}
{"status": "FAIL", "group": "default", "message": "assert_equals: expected (string) \"state1\" but got (boolean) false", "stack": "window.onload/<@http://web-platform.test:8000/html/browsers/history/the-history-interface/007.html:35:5\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20\ntest@http://web-platform.test:8000/resources/testharness.js:548:9\nwindow.onload@http://web-platform.test:8000/html/browsers/history/the-history-interface/007.html:34:3\n", "subtest": "the correct state should be restored when navigating during initial load", "test": "/html/browsers/history/the-history-interface/007.html", "line": 156103, "action": "test_result", "expected": "PASS"}
{"status": "FAIL", "group": "default", "message": "assert_equals: expected \"state1\" but got \"state2\"", "stack": "window.onload/<@http://web-platform.test:8000/html/browsers/history/the-history-interface/007.html:38:5\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20\ntest@http://web-platform.test:8000/resources/testharness.js:548:9\nwindow.onload@http://web-platform.test:8000/html/browsers/history/the-history-interface/007.html:37:3\n", "subtest": "history.state should reflect the navigated state onload", "test": "/html/browsers/history/the-history-interface/007.html", "line": 156109, "action": "test_result", "expected": "PASS"}
{"status": "FAIL", "group": "default", "message": "assert_false: expected false got true", "stack": "window.onload/</<@http://web-platform.test:8000/html/browsers/history/the-history-interface/007.html:43:7\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20\ntest@http://web-platform.test:8000/resources/testharness.js:548:9\nwindow.onload/<@http://web-platform.test:8000/html/browsers/history/the-history-interface/007.html:42:5\n", "subtest": "popstate event should not fire after onload fires", "test": "/html/browsers/history/the-history-interface/007.html", "line": 156114, "action": "test_result", "expected": "PASS"}

seems all related to "/html/browsers/history/the-history-interface/007.html", not sure what I changed that could cause this.

jdm · 2018-05-24T15:13:20Z

The fact that that failure has not appeared on any previous attempts to merge made me file #20856 for it.

bors-servo · 2018-05-24T15:21:33Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css2, mac-rel-wpt2, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, mac-rel-css1, mac-rel-wpt1, mac-rel-wpt4...

bors-servo · 2018-05-24T15:32:50Z

💔 Test failed - mac-rel-wpt4

gterzian · 2018-05-24T15:39:10Z

error: Could not read 849fba27ab7b02407d4c9ae6230a2176bf658725
fatal: cannot simplify commit 77a09b2003f000537c4f87f50fb45b6e2fd6eb75 (because of 849fba27ab7b02407d4c9ae6230a2176bf658725)
Current Rust version for Servo: nightly-2018-04-15

I think this is some sort of intermittent thing...

gterzian · 2018-05-24T15:43:11Z

@bors-servo retry

#20694

bors-servo · 2018-05-24T16:11:15Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt4...

bors-servo · 2018-05-24T16:58:44Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: payl
Pushing f6762d5 to master...

highfive assigned glennw Apr 28, 2018

highfive added the S-awaiting-review label Apr 28, 2018

gterzian mentioned this pull request Apr 28, 2018

[WIP] Script to embedder communication #20706

Closed

0 of 6 tasks complete

gterzian force-pushed the gterzian:embedder_handling_of_prompts_and_alerts_2 branch 2 times, most recently from 168e7b5 to 0bd70c6 Apr 28, 2018

gterzian force-pushed the gterzian:embedder_handling_of_prompts_and_alerts_2 branch from af550ae to c4dc72e Apr 30, 2018

gterzian force-pushed the gterzian:embedder_handling_of_prompts_and_alerts_2 branch 2 times, most recently from 4d08719 to 84de812 May 2, 2018

gterzian reviewed May 2, 2018

View changes

gterzian force-pushed the gterzian:embedder_handling_of_prompts_and_alerts_2 branch from 84de812 to 191e723 May 2, 2018

paulrouget requested changes May 2, 2018

View changes

gterzian force-pushed the gterzian:embedder_handling_of_prompts_and_alerts_2 branch 2 times, most recently from a157ca8 to 6282789 May 2, 2018

paulrouget requested changes May 2, 2018

View changes

highfive added the S-tests-failed label May 24, 2018

fix url input for android

Loading status checks…

a5dc1b6

gterzian force-pushed the gterzian:embedder_handling_of_prompts_and_alerts_2 branch from 44486bd to a5dc1b6 May 24, 2018

highfive added S-awaiting-review and removed S-tests-failed labels May 24, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels May 24, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels May 24, 2018

highfive added the S-tests-failed label May 24, 2018

bors-servo merged commit a5dc1b6 into servo:master May 24, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

paulrouget mentioned this pull request Jun 6, 2018

What is our popup story? #10056

Open

gterzian mentioned this pull request Jul 13, 2018

Event loops/Message pumps security issues #21171

Open

gterzian deleted the gterzian:embedder_handling_of_prompts_and_alerts_2 branch Jan 7, 2020

servo / servo

Join GitHub today

Embedder handling of prompts and alerts #20707

Embedder handling of prompts and alerts #20707

Conversation

gterzian commented Apr 28, 2018 • edited by SimonSapin

highfive commented Apr 28, 2018

gterzian commented Apr 28, 2018 • edited

paulrouget commented Apr 30, 2018

gterzian commented Apr 30, 2018

gterzian commented Apr 30, 2018

gterzian commented Apr 30, 2018

jdm commented Apr 30, 2018

gterzian commented Apr 30, 2018 • edited

paulrouget commented Apr 30, 2018 • edited

cbrewster commented Apr 30, 2018

paulrouget commented May 1, 2018

gterzian commented May 1, 2018 • edited

gterzian commented May 1, 2018 • edited

jdm commented May 1, 2018

gterzian commented May 2, 2018 • edited

This comment has been minimized.

gterzian May 2, 2018

This comment has been minimized.

paulrouget May 2, 2018

paulrouget left a comment

This comment has been minimized.

paulrouget May 2, 2018

This comment has been minimized.

paulrouget May 2, 2018 • edited

This comment has been minimized.

gterzian May 2, 2018

gterzian commented May 2, 2018 • edited

This comment has been minimized.

paulrouget May 2, 2018

This comment has been minimized.

paulrouget May 2, 2018

This comment has been minimized.

paulrouget May 2, 2018 • edited

This comment has been minimized.

paulrouget May 2, 2018

jdm commented May 24, 2018

gterzian commented May 24, 2018

jdm commented May 24, 2018

bors-servo commented May 24, 2018

bors-servo commented May 24, 2018

bors-servo commented May 24, 2018

jdm commented May 24, 2018

gterzian commented May 24, 2018

jdm commented May 24, 2018

bors-servo commented May 24, 2018

bors-servo commented May 24, 2018

gterzian commented May 24, 2018 • edited

gterzian commented May 24, 2018

bors-servo commented May 24, 2018

bors-servo commented May 24, 2018

gterzian commented Apr 28, 2018 •

edited by SimonSapin

gterzian commented Apr 28, 2018 •

edited

gterzian commented Apr 30, 2018 •

edited

paulrouget commented Apr 30, 2018 •

edited

gterzian commented May 1, 2018 •

edited

gterzian commented May 1, 2018 •

edited

gterzian commented May 2, 2018 •

edited

paulrouget May 2, 2018 •

edited

gterzian commented May 2, 2018 •

edited

paulrouget May 2, 2018 •

edited

gterzian commented May 24, 2018 •

edited