[webdriver] Move Webdriver to ServoShell #36714

longvatrong111 · 2025-04-27T21:31:24Z

Moving webdriver to servoshell:

Let servoshell manage lifecycle of webdriver
One by one, move the handling of webdriver commands from constellation to embedder

Partially fix: #37370
Partially fix webdriver test timeout with no_top_browsing_context

cc: @xiaochengh

yezhizhen

Thanks for working on this!

components/script/script_thread.rs

webbeef · 2025-04-29T05:19:55Z

Does that mean that every embedder will need to add Webdriver support code?

longvatrong111 · 2025-04-29T05:26:24Z

Does that mean that every embedder will need to add Webdriver support code?

It's a nice question. I think we can separate Webdriver support code as much as possible and reuse in different embedders.

mrobinson · 2025-04-29T06:30:37Z

Does that mean that every embedder will need to add Webdriver support code?

I think this makes sense to some extent. For instance Webdriver can open and close WebViews. Only the embedder can do this properly.

xiaochengh · 2025-04-29T08:30:31Z

Discussed with @longvatrong111 offline.

It's better to make WebDriver independent of ServoShell, as WebDriver shouldn't depend on any specific embedder.

WebDriver and real events should still share the same code paths as much as possible.

mrobinson · 2025-04-29T13:51:12Z

It's better to make WebDriver independent of ServoShell, as WebDriver shouldn't depend on any specific embedder.

I still think it isn't a good idea to make it totally independent. At the very least, servoshell should use some generic WebDriver interface, but still execute the requested actions using its internal interface. WebDriver should be driving a browser, and not just the embedding layer.

yezhizhen · 2025-04-30T02:15:30Z

As discussed with @longvatrong111 , I will move change in script_thread.rs to another PR and contribute upon it.

xiaochengh · 2025-04-30T02:57:44Z

WebDriver should be driving a browser, and not just the embedding layer.

+1 to this. We'll see how it will be carried out.

yezhizhen · 2025-04-30T03:57:35Z

As discussed with @longvatrong111 , I will move change in script_thread.rs to another PR and contribute upon it.

It seems new assertion failure is introduced for tests\wpt\tests\webdriver\tests\classic\element_click\events.py by get_events, with changes in script_thread.rs here.

assertion failed: !GetCurrentRealmOrNull(*cx).is_null() (thread Script(1,2), at components\script_bindings\realms.rs:25)

I will start by working from there.

longvatrong111 · 2025-05-06T07:11:08Z

Rebase PR. Will check all reviews 1 by 1.
Right now I'm focusing on fixing synchronization issue between Input Event and WebDriver Script Event (the part about routing) with this PR.

xiaochengh · 2025-05-22T06:55:41Z

Is there a clearer illustration / doc of what the relation between servoshell and web driver is, and what the relation should be?

mrobinson · 2025-05-23T12:21:26Z

Is there a clearer illustration / doc of what the relation between servoshell and web driver is, and what the relation should be?

Yes, sorry for the confusion.

The WebDriver is an HTTP server that can drive the browser via commands from tests that are sent to the server.

Currently: The WebDriver is activated by the Servo embedding API. When active, the Constellation spawns a thread to listen for HTTP requests and send IPC messages directly to the Constellation or ScriptThreads. This presents an issue for several reasons including:

When a client requests a new WebView to be created, the embedding API must request the embedder to create the WebView. It makes more sense for the WebDriver to create the WebView through the embedding API directly. This allows testing the embedding API and prevents a weird inversion of control. The only other time this happens is via things like window.open(), but these are more closely managed by the ScriptThread.
Some events must be processed by the compositor before being sent to the internals of Servo. Since the WebDriver is running inside Servo, these events must be sent "out" of Servo to the embedding layer and it is difficult to ensure proper ordering of event handling since some events go straight to the Constellation and some go back out to the embedding layer first.

Future: servoshell would run a WebDriver thread that runs the HTTP server. Many commands become Servo embedding API commands and a good amount of special code can be removed from Servo to deal with the WebDriver. Note that this will still require an API to send some of the more specific WebDriver commands to the Constellation and then to the ScriptThread. This means that things like opening and closing WebViews work exactly the same way they do in a browser application. In order to avoid having the WebDriver code depend directly on servoshell, servoshell may be able to provide some callbacks which the WebDriver code can use.

xiaochengh · 2025-05-28T08:25:18Z

Thanks for the clarification, that makes much more sense to me now!

So far my understanding of the objectives are: (1) Make servoshell manage the lifetime of WebDriver; (2) Make WebDriver drive Servo with the embedding API. Is that correct?

Another question: besides lifetime management, is there other communication needed between WebDriver and servoshell?

mrobinson · 2025-06-06T09:29:10Z

Another question: besides lifetime management, is there other communication needed between WebDriver and servoshell?

That's a good question. I'm not exactly sure of the answer here. I think it depends on the details of all of the WebDriver APIs. If there are cases where WebDriver wants to do something like move the browser window around, then it will need to move the servoshell window.

longvatrong111 · 2025-06-10T10:18:56Z

@xiaochengh @mrobinson it is confirmed that the ~~implementation of~~ event route of commands in Context section are much simpler if sending events to servoshell instead of constellation. For example Close Window

yezhizhen

it is confirmed that the implementation of event route of commands in Context section are much simpler if sending events to servoshell instead of constellation. For example Close Window

I don't think so. Right now close window shares Constellation::handle_close_top_level_browsing_context with request ScriptToConstellationMessage and EmbedderToConstellationMessage::CloseWebView. Isn't that nice?

One by one, move the handling of webdriver commands from constellation to embedder

Some of them would still be easier to handle in Constellation as they require information from it. Need to be very careful with each case... :(

longvatrong111 · 2025-06-17T16:16:05Z

We need to hold a bit more, all tests are timeout in CI:
https://github.com/longvatrong111/servo/actions/runs/15704827627
It's weird because if I run some tests in local, the result is normal.

yezhizhen

The code itself looks good. Very excited for this!

Side notes:
I've been verifying the execution path, and found some redundancy for channel communication which has been there for quite a while. (Not really related to this PR)

But I wanna ask: why we sometimes use crossbeam_channel, and sometimes ipc? I assume the former is more efficient + mpmc, the latter is required for inter-process communication (to Content Process, e.g. script thread). If I'm correct, then we are misusing many ipc channels in webdriver_server/lib.rs if only talking to Constellation/embedder. Instead, should just use crossbeam_channel or even mpsc?
cc @jdm @sagudev

ports/servoshell/desktop/app.rs

longvatrong111 · 2025-06-17T16:37:57Z

Thanks for the improvements!

I'm also wondering if we can further improve it by decoupling the current implementation with desktop ServoShell, so that it becomes something that can be easily plugged into any embedder implementation (eg SimpleServo on OHOS, which might be of our interests).

Probably something like:
struct WebDriverEmbedderPlugin {
  webdriver_receiver: Receiver<EmbedderMsg>,
  embedder_proxy: EmbedderProxy,
  ...
}

impl WebDriverEmbedderPlugin<'app> {
  // Called from embedder to start a WebDriver server.
  pub fn new(...) -> Self {...}

  // Called from emb
2D0B
edder only when running.
  pub fn handle_messages(&mut self, running_state: Rc<RunningAppState>) {...}

  fn forward_command(&self, command) {... }
}

...

pub struct App {
  ...
  webdriver_plugin: Option<WebDriverEmbedderPlugin>,
  ...
}
But there is no need to do it in this PR, since there are quite some obstacles before there, like we need a common trait for desktop and egl AppState, and probably more. But it's worth leaving a TODO for it.

I'm also looking for a way to easily plug webdriver into any embedder. Thank you for the suggestion.

mrobinson · 2025-06-17T17:15:04Z

Apologies for not getting to this one yet. I will look at it first thing tomorrow morning.

sagudev · 2025-06-18T04:07:07Z

why we sometimes use crossbeam_channel, and sometimes ipc?

You guessed right, it is for cross process reasons, but even though some stuff currently live in same process (actually all because we run servo in single process mode by default), but that might not be true in the future (there is also plan to have GPU process, etc), so we use ipc-channel in more places for easier potential process split up.

Here are other interesting ipc stuff:

jdm · 2025-06-18T06:55:54Z

The code itself looks good. Very excited for this!

Side notes: I've been verifying the execution path, and found some redundancy for channel communication which has been there for quite a while. (Not really related to this PR)

But I wanna ask: why we sometimes use crossbeam_channel, and sometimes ipc? I assume the former is more efficient + mpmc, the latter is required for inter-process communication (to Content Process, e.g. script thread). If I'm correct, then we are misusing many ipc channels in webdriver_server/lib.rs if only talking to Constellation/embedder. Instead, should just use crossbeam_channel or even mpsc? cc @jdm @sagudev

You are correct; uses like

servo/components/constellation/constellation.rs

Lines 4761 to 4768 in 96ef92b

    
           WebDriverCommandMsg::GetWindowSize(webview_id, response_sender) => { 
        
               let browsing_context_id = BrowsingContextId::from(webview_id); 
        
               let size = self 
        
                   .browsing_contexts 
        
                   .get(&browsing_context_id) 
        
                   .map(|browsing_context| browsing_context.viewport_details.size) 
        
                   .unwrap_or_default(); 
        
               let _ = response_sender.send(size);

do not require an IPC channel. However, WebDriverCommandMsg requires that all variants can be serialized right now (

servo/components/shared/embedder/webdriver.rs

Line 31 in 96ef92b

#[derive(Debug, Deserialize, Serialize)]

), but it's not clear to me why that enum needs that derive.

mrobinson

Great stuff!

I just have a few suggestions.

ports/servoshell/desktop/app.rs

components/constellation/constellation.rs

components/webdriver_server/lib.rs

mrobinson

Thanks! One more comment before landing, but once that's done feel free to land without another review.

ports/servoshell/desktop/app.rs

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Decompose webdriver::embedder_proxy to a sender and a waker. Make the sender channel uses only webdriver command message. Rebase main and solve conflict Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

mrobinson · 2025-06-19T09:18:10Z

I added some minor rustdoc comments and did a little re-ordering of members. Sending to the merge queue now. Thanks @longvatrong111!

@xiaochengh

…etWindowSize` to servoshell (#37555) Follow up to: #36714 Moving webdriver [context commands](https://www.w3.org/TR/webdriver2/#contexts) to be handled in embedder: - [x] New Window command - `WebdriverCommandMsg::NewWebView` - [x] Switch To Window command - `WebdriverCommandMsg::FocusWebView` - [x] Resizing commands cc: @xiaochengh --------- Signed-off-by: batu_hoang <longvatrong111@gmail.com>

longvatrong111 requested review from atbrakhi and gterzian as code owners April 27, 2025 21:31

yezhizhen reviewed Apr 29, 2025

View reviewed changes

components/script/script_thread.rs Outdated Show resolved Hide resolved

components/script/script_thread.rs Outdated Show resolved Hide resolved

components/script/script_thread.rs Outdated Show resolved Hide resolved

yezhizhen mentioned this pull request May 4, 2025

Synchronize WebdriverScriptEvent with pending input events #36830

Draft

longvatrong111 force-pushed the webdriver-improve branch 2 times, most recently from 6a25d08 to c2e54dd Compare May 6, 2025 07:08

longvatrong111 force-pushed the webdriver-improve branch from c2e54dd to def26b0 Compare May 6, 2025 07:19

longvatrong111 mentioned this pull request May 21, 2025

Synchronize dispatch_actions in WebDriver #36932

Merged

longvatrong111 marked this pull request as draft May 23, 2025 04:04

yezhizhen mentioned this pull request Jun 6, 2025

servoshell: Do not focus and raise new auxiliary WebDriver-created WebViews #37284

Merged

longvatrong111 changed the title ~~Draft: Move Webdriver to ServoShell~~ [webdriver] Move Webdriver to ServoShell Jun 7, 2025

longvatrong111 force-pushed the webdriver-improve branch from def26b0 to 158b62b Compare June 10, 2025 09:34

longvatrong111 marked this pull request as ready for review June 10, 2025 09:36

longvatrong111 mentioned this pull request Jun 10, 2025

[webdriver] Moving webdriver to servoshell #37370

Closed

5 tasks

longvatrong111 mentioned this pull request Jun 13, 2025

webdriver: Keep constellation alive and Open new top-level browsing context with new session request when none is open #37410

Merged

yezhizhen reviewed Jun 13, 2025

View reviewed changes

yezhizhen reviewed Jun 17, 2025

View reviewed changes

ports/servoshell/desktop/app.rs Show resolved Hide resolved

longvatrong111 mentioned this pull request Jun 18, 2025

Hot fix panic in webdriver #37526

Closed

mrobinson requested changes Jun 18, 2025

View reviewed changes

longvatrong111 force-pushed the webdriver-improve branch 3 times, most recently from 174bf83 to dd7b393 Compare June 19, 2025 05:51

longvatrong111 requested a review from mrobinson June 19, 2025 07:57

mrobinson approved these changes Jun 19, 2025

View reviewed changes

ports/servoshell/desktop/app.rs Outdated Show resolved Hide resolved

ports/servoshell/desktop/app.rs Outdated Show resolved Hide resolved

longvatrong111 force-pushed the webdriver-improve branch from 09002a4 to 7c5844a Compare June 19, 2025 08:49

longvatrong111 added 4 commits June 19, 2025 11:12

Move webdriver to servoshell

54be927

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Decompose webdriver::embedder_proxy

7d0bff4

Decompose webdriver::embedder_proxy to a sender and a waker. Make the sender channel uses only webdriver command message. Rebase main and solve conflict Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Rename constellation_chan as a TODO note

2cebf9c

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Move webdriver_receiver to app running state

83fb014

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

mrobinson force-pushed the webdriver-improve branch from 7c5844a to 83fb014 Compare June 19, 2025 09:17

mrobinson enabled auto-merge June 19, 2025 09:18

mrobinson added this pull request to the merge queue Jun 19, 2025

Merged via the queue into servo:main with commit d010079 Jun 19, 2025
21 checks passed

longvatrong111 mentioned this pull request Jun 19, 2025

webdriver: Move NewWebView, FocusWebView, GetWindowSize, and SetWindowSize to servoshell #37555

Merged

3 tasks

longvatrong111 deleted the webdriver-improve branch June 19, 2025 10:47

longvatrong111 mentioned this pull request Jun 24, 2025

webdriver: Evaluate script commands via the WebView API in servoshell #37663

Merged

jdm mentioned this pull request Jul 3, 2025

Move webdriver_port option to servoshell-exclusive options #37843

Closed

yezhizhen mentioned this pull request Aug 4, 2025

Running Non-testdriver Tests on Mobile Platform #38445

Open

Uh oh!

[webdriver] Move Webdriver to ServoShell #36714

[webdriver] Move Webdriver to ServoShell #36714

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!