From 37ac4ffeb45a1b7c866508fad55f74cd2b05b7d9 Mon Sep 17 00:00:00 2001 From: batu_hoang <55729155+longvatrong111@users.noreply.github.com> Date: Wed, 30 Jul 2025 15:24:07 +0800 Subject: [PATCH] Rework on webdriver wait for navigation complete (#38234) For current implementation, when a command may trigger a navigation, webdriver only waits for document readiness state. However, not all navigations make change in document. This PR handles more cases for waiting for a navigation, and apply to `element_click`. - Before sending a command which may trigger a navigation, `webdriver` sets `load status send` to `embedder`, `constelltation` and `script thread` to listen to `navigation events`. - Webdriver check if there is a navigation with `script thread`. - If the navigation is loading a new url, webdriver checks if the request is approved with `constellation`, then waits for document readiness state. - If the navigation is a hashchange, webdriver waits untill all new generated dom events have been processed. Testing: `tests/wpt/tests/webdriver/tests/classic/element_click/navigate.py` `tests/wpt/tests/webdriver/tests/classic/element_click/user_prompts.py` https://github.com/longvatrong111/servo/actions/runs/16488690749 cc: @xiaochengh --------- Signed-off-by: batu_hoang --- components/constellation/constellation.rs | 28 ++++- components/script/dom/window.rs | 26 ++++- components/script/script_thread.rs | 43 ++++--- components/script/webdriver_handlers.rs | 36 +++--- components/shared/embedder/webdriver.rs | 12 +- components/webdriver_server/lib.rs | 110 +++++++++++------- ports/servoshell/desktop/app.rs | 16 +-- .../classic/element_click/navigate.py.ini | 24 ---- .../classic/element_click/user_prompts.py.ini | 18 --- 9 files changed, 188 insertions(+), 125 deletions(-) delete mode 100644 tests/wpt/meta/webdriver/tests/classic/element_click/user_prompts.py.ini diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 75c205111e6..6ae2eb0fe25 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -133,7 +133,7 @@ use embedder_traits::{ FocusSequenceNumber, InputEvent, JSValue, JavaScriptEvaluationError, JavaScriptEvaluationId, KeyboardEvent, MediaSessionActionType, MediaSessionEvent, MediaSessionPlaybackState, MouseButton, MouseButtonAction, MouseButtonEvent, Theme, ViewportDetails, WebDriverCommandMsg, - WebDriverCommandResponse, + WebDriverCommandResponse, WebDriverLoadStatus, WebDriverScriptCommand, }; use euclid::Size2D; use euclid::default::Size2D as UntypedSize2D; @@ -396,6 +396,9 @@ pub struct Constellation { /// and the namespaces are allocated by the constellation. next_pipeline_namespace_id: PipelineNamespaceId, + /// An [`IpcSender`] to notify navigation events to webdriver. + webdriver_load_status_sender: Option<(IpcSender, PipelineId)>, + /// An [`IpcSender`] to forward responses from the `ScriptThread` to the WebDriver server. webdriver_input_command_reponse_sender: Option>, @@ -666,6 +669,7 @@ where time_profiler_chan: state.time_profiler_chan, mem_profiler_chan: state.mem_profiler_chan.clone(), phantom: PhantomData, + webdriver_load_status_sender: None, webdriver_input_command_reponse_sender: None, document_states: HashMap::new(), #[cfg(feature = "webgpu")] @@ -1254,6 +1258,12 @@ where if allowed { self.load_url(webview_id, pipeline_id, load_data, history_handling); } else { + if let Some((sender, id)) = &self.webdriver_load_status_sender { + if pipeline_id == *id { + let _ = sender.send(WebDriverLoadStatus::NavigationStop); + } + } + let pipeline_is_top_level_pipeline = self .browsing_contexts .get(&BrowsingContextId::from(webview_id)) @@ -3518,7 +3528,12 @@ where }; if let Err(e) = result { self.handle_send_error(parent_pipeline_id, e); + } else if let Some((sender, id)) = &self.webdriver_load_status_sender { + if source_id == *id { + let _ = sender.send(WebDriverLoadStatus::NavigationStop); + } } + None }, None => { @@ -4414,6 +4429,17 @@ where .get(&browsing_context_id) .expect("ScriptCommand: Browsing context must exist at this point") .pipeline_id; + + match &cmd { + WebDriverScriptCommand::AddLoadStatusSender(_, sender) => { + self.webdriver_load_status_sender = Some((sender.clone(), pipeline_id)); + }, + WebDriverScriptCommand::RemoveLoadStatusSender(_) => { + self.webdriver_load_status_sender = None; + }, + _ => {}, + }; + let control_msg = ScriptThreadMessage::WebDriverScriptCommand(pipeline_id, cmd); let result = match self.pipelines.get(&pipeline_id) { Some(pipeline) => pipeline.event_loop.send(control_msg), diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 10eced15a57..e60a5e272b8 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -34,7 +34,7 @@ use embedder_traits::user_content_manager::{UserContentManager, UserScript}; use embedder_traits::{ AlertResponse, ConfirmResponse, EmbedderMsg, GamepadEvent, GamepadSupportedHapticEffects, GamepadUpdateType, PromptResponse, SimpleDialog, Theme, ViewportDetails, WebDriverJSError, - WebDriverJSResult, + WebDriverJSResult, WebDriverLoadStatus, }; use euclid::default::{Point2D as UntypedPoint2D, Rect as UntypedRect, Size2D as UntypedSize2D}; use euclid::{Point2D, Scale, Size2D, Vector2D}; @@ -311,6 +311,10 @@ pub(crate) struct Window { #[no_trace] webdriver_script_chan: DomRefCell>>, + /// A channel to notify webdriver if there is a navigation + #[no_trace] + webdriver_load_status_sender: RefCell>>, + /// The current state of the window object current_state: Cell, @@ -2641,6 +2645,11 @@ impl Window { // Step 6 // TODO: Fragment handling appears to have moved to step 13 if let Some(fragment) = load_data.url.fragment() { + let webdriver_sender = self.webdriver_load_status_sender.borrow().clone(); + if let Some(ref sender) = webdriver_sender { + let _ = sender.send(WebDriverLoadStatus::NavigationStart); + } + self.send_to_constellation(ScriptToConstellationMessage::NavigatedToFragment( load_data.url.clone(), history_handling, @@ -2660,6 +2669,9 @@ impl Window { new_url, CanGc::note()); event.upcast::().fire(this.upcast::(), CanGc::note()); + if let Some(sender) = webdriver_sender { + let _ = sender.send(WebDriverLoadStatus::NavigationStop); + } }); self.as_global_scope() .task_manager() @@ -2714,6 +2726,10 @@ impl Window { NavigationHistoryBehavior::Push }; + if let Some(sender) = self.webdriver_load_status_sender.borrow().as_ref() { + let _ = sender.send(WebDriverLoadStatus::NavigationStart); + } + // Step 13 ScriptThread::navigate(pipeline_id, load_data, resolved_history_handling); }; @@ -2845,6 +2861,13 @@ impl Window { *self.webdriver_script_chan.borrow_mut() = chan; } + pub(crate) fn set_webdriver_load_status_sender( + &self, + sender: Option>, + ) { + *self.webdriver_load_status_sender.borrow_mut() = sender; + } + pub(crate) fn is_alive(&self) -> bool { self.current_state.get() == WindowState::Alive } @@ -3139,6 +3162,7 @@ impl Window { devtools_marker_sender: Default::default(), devtools_markers: Default::default(), webdriver_script_chan: Default::default(), + webdriver_load_status_sender: Default::default(), error_reporter, media_query_lists: DOMTracker::new(), #[cfg(feature = "bluetooth")] diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 521a28721a6..fb393bc00d6 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1091,11 +1091,7 @@ impl ScriptThread { for event in document.take_pending_input_events().into_iter() { document.update_active_keyboard_modifiers(event.active_keyboard_modifiers); - // We do this now, because the event is consumed below, but the order doesn't really - // matter as the event will be handled before any new ScriptThread messages are processed. - self.notify_webdriver_input_event_completed(pipeline_id, &event.event); - - match event.event { + match event.event.clone() { InputEvent::MouseButton(mouse_button_event) => { document.handle_mouse_button_event(mouse_button_event, &event, can_gc); }, @@ -1151,21 +1147,37 @@ impl ScriptThread { document.handle_embedder_scroll_event(scroll_event); }, } + + self.notify_webdriver_input_event_completed(pipeline_id, event.event, &document); } ScriptThread::set_user_interacting(false); } - fn notify_webdriver_input_event_completed(&self, pipeline_id: PipelineId, event: &InputEvent) { + fn notify_webdriver_input_event_completed( + &self, + pipeline_id: PipelineId, + event: InputEvent, + document: &Document, + ) { let Some(id) = event.webdriver_message_id() else { return; }; - if let Err(error) = self.senders.pipeline_to_constellation_sender.send(( - pipeline_id, - ScriptToConstellationMessage::WebDriverInputComplete(id), - )) { - warn!("ScriptThread failed to send WebDriverInputComplete {id:?}: {error:?}",); - } + let sender = self.senders.pipeline_to_constellation_sender.clone(); + + // Webdriver should be notified once all current dom events have been processed. + document + .owner_global() + .task_manager() + .dom_manipulation_task_source() + .queue(task!(notify_webdriver_input_event_completed: move || { + if let Err(error) = sender.send(( + pipeline_id, + ScriptToConstellationMessage::WebDriverInputComplete(id), + )) { + warn!("ScriptThread failed to send WebDriverInputComplete {id:?}: {error:?}",); + } + })); } /// @@ -2516,13 +2528,16 @@ impl ScriptThread { reply, can_gc, ), - WebDriverScriptCommand::IsDocumentReadyStateComplete(response_sender) => { - webdriver_handlers::handle_try_wait_for_document_navigation( + WebDriverScriptCommand::AddLoadStatusSender(_, response_sender) => { + webdriver_handlers::handle_add_load_status_sender( &documents, pipeline_id, response_sender, ) }, + WebDriverScriptCommand::RemoveLoadStatusSender(_) => { + webdriver_handlers::handle_remove_load_status_sender(&documents, pipeline_id) + }, _ => (), } } diff --git a/components/script/webdriver_handlers.rs b/components/script/webdriver_handlers.rs index cd319977bd0..7b1b3cf8570 100644 --- a/components/script/webdriver_handlers.rs +++ b/components/script/webdriver_handlers.rs @@ -8,7 +8,9 @@ use std::ptr::NonNull; use base::id::{BrowsingContextId, PipelineId}; use cookie::Cookie; -use embedder_traits::{WebDriverFrameId, WebDriverJSError, WebDriverJSResult, WebDriverJSValue}; +use embedder_traits::{ + WebDriverFrameId, WebDriverJSError, WebDriverJSResult, WebDriverJSValue, WebDriverLoadStatus, +}; use euclid::default::{Point2D, Rect, Size2D}; use hyper_serde::Serde; use ipc_channel::ipc::{self, IpcSender}; @@ -33,9 +35,7 @@ use webdriver::error::ErrorStatus; use crate::document_collection::DocumentCollection; use crate::dom::bindings::codegen::Bindings::CSSStyleDeclarationBinding::CSSStyleDeclarationMethods; use crate::dom::bindings::codegen::Bindings::DOMRectBinding::DOMRectMethods; -use crate::dom::bindings::codegen::Bindings::DocumentBinding::{ - DocumentMethods, DocumentReadyState, -}; +use crate::dom::bindings::codegen::Bindings::DocumentBinding::DocumentMethods; use crate::dom::bindings::codegen::Bindings::ElementBinding::ElementMethods; use crate::dom::bindings::codegen::Bindings::HTMLElementBinding::HTMLElementMethods; use crate::dom::bindings::codegen::Bindings::HTMLInputElementBinding::HTMLInputElementMethods; @@ -1926,19 +1926,23 @@ pub(crate) fn handle_is_selected( .unwrap(); } -pub(crate) fn handle_try_wait_for_document_navigation( +pub(crate) fn handle_add_load_status_sender( documents: &DocumentCollection, pipeline: PipelineId, - reply: IpcSender, + reply: IpcSender, ) { - let document = match documents.find_document(pipeline) { - Some(document) => document, - None => { - return reply.send(false).unwrap(); - }, - }; - - let wait_for_document_ready = document.ReadyState() != DocumentReadyState::Complete; - - reply.send(wait_for_document_ready).unwrap(); + if let Some(document) = documents.find_document(pipeline) { + let window = document.window(); + window.set_webdriver_load_status_sender(Some(reply)); + } +} + +pub(crate) fn handle_remove_load_status_sender( + documents: &DocumentCollection, + pipeline: PipelineId, +) { + if let Some(document) = documents.find_document(pipeline) { + let window = document.window(); + window.set_webdriver_load_status_sender(None); + } } diff --git a/components/shared/embedder/webdriver.rs b/components/shared/embedder/webdriver.rs index e329f55bd93..f490b8e7a80 100644 --- a/components/shared/embedder/webdriver.rs +++ b/components/shared/embedder/webdriver.rs @@ -169,8 +169,6 @@ pub enum WebDriverCommandMsg { ), GetAlertText(WebViewId, IpcSender>), SendAlertText(WebViewId, String), - AddLoadStatusSender(WebViewId, IpcSender), - RemoveLoadStatusSender(WebViewId), } #[derive(Debug, Deserialize, Serialize)] @@ -247,7 +245,8 @@ pub enum WebDriverScriptCommand { GetTitle(IpcSender), /// Deal with the case of input element for Element Send Keys, which does not send keys. WillSendKeys(String, String, bool, IpcSender>), - IsDocumentReadyStateComplete(IpcSender), + AddLoadStatusSender(WebViewId, IpcSender), + RemoveLoadStatusSender(WebViewId), } #[derive(Clone, Debug, Deserialize, Serialize)] @@ -292,8 +291,13 @@ pub struct WebDriverCommandResponse { #[derive(Debug, Deserialize, Serialize)] pub enum WebDriverLoadStatus { + NavigationStart, + // Navigation stops for any reason + NavigationStop, + // Document ready state is complete Complete, + // Load timeout Timeout, - Canceled, + // Navigation is blocked by a user prompt Blocked, } diff --git a/components/webdriver_server/lib.rs b/components/webdriver_server/lib.rs index a3b403483ba..8c262c4fa55 100644 --- a/components/webdriver_server/lib.rs +++ b/components/webdriver_server/lib.rs @@ -499,6 +499,23 @@ impl Handler { Ok(()) } + fn add_load_status_sender(&self) -> WebDriverResult<()> { + self.send_message_to_embedder(WebDriverCommandMsg::ScriptCommand( + self.session()?.browsing_context_id, + WebDriverScriptCommand::AddLoadStatusSender( + self.session()?.webview_id, + self.load_status_sender.clone(), + ), + )) + } + + fn clear_load_status_sender(&self) -> WebDriverResult<()> { + self.send_message_to_embedder(WebDriverCommandMsg::ScriptCommand( + self.session()?.browsing_context_id, + WebDriverScriptCommand::RemoveLoadStatusSender(self.session()?.webview_id), + )) + } + // This function is called only if session and webview are verified. fn verified_webview_id(&self) -> WebViewId { self.session().unwrap().webview_id @@ -687,7 +704,7 @@ impl Handler { self.send_message_to_embedder(cmd_msg)?; // Step 8.2.1: try to wait for navigation to complete. - self.wait_for_navigation_to_complete()?; + self.wait_for_document_ready_state()?; // Step 8.3. Set current browsing context with session and current top browsing context self.session_mut()?.browsing_context_id = BrowsingContextId::from(webview_id); @@ -695,8 +712,7 @@ impl Handler { Ok(WebDriverResponse::Void) } - /// - fn wait_for_navigation_to_complete(&self) -> WebDriverResult { + fn wait_for_document_ready_state(&self) -> WebDriverResult { debug!("waiting for load"); let session = self.session()?; @@ -723,7 +739,8 @@ impl Handler { let result = select! { recv(self.load_status_receiver) -> res => { - match res { + match res { + // If the navigation is navigation to IFrame, no document state event is fired. Ok(WebDriverLoadStatus::Blocked) => { // TODO: evaluate the correctness later // Load status is block means an user prompt is shown. @@ -732,10 +749,13 @@ impl Handler { // If user prompt can't be handler, next command returns // an error anyway. Ok(WebDriverResponse::Void) - } - _ => { - Ok(WebDriverResponse::Void) - } + }, + Ok(WebDriverLoadStatus::Complete) | + Ok(WebDriverLoadStatus::NavigationStop) => Ok(WebDriverResponse::Void), + _ => Err(WebDriverError::new( + ErrorStatus::UnknownError, + "Unexpected load status received while waiting for document ready state", + )), } }, recv(after(Duration::from_millis(timeout))) -> _ => Err( @@ -746,6 +766,38 @@ impl Handler { result } + /// + fn wait_for_navigation(&self) -> WebDriverResult { + let navigation_status = match self.load_status_receiver.try_recv() { + Ok(status) => status, + // Empty channel means no navigation started. Nothing to wait for. + Err(crossbeam_channel::TryRecvError::Empty) => { + return Ok(WebDriverResponse::Void); + }, + Err(crossbeam_channel::TryRecvError::Disconnected) => { + return Err(WebDriverError::new( + ErrorStatus::UnknownError, + "Load status channel disconnected", + )); + }, + }; + + match navigation_status { + WebDriverLoadStatus::NavigationStart => self.wait_for_document_ready_state(), + // If the load status is timeout, return an error + WebDriverLoadStatus::Timeout => Err(WebDriverError::new( + ErrorStatus::Timeout, + "Navigation timed out", + )), + // If the load status is blocked, it means a user prompt is shown. + // We should handle the user prompt in the next command. + WebDriverLoadStatus::Blocked => Ok(WebDriverResponse::Void), + WebDriverLoadStatus::NavigationStop | WebDriverLoadStatus::Complete => { + unreachable!("Unexpected load status received") + }, + } + } + /// fn handle_current_url(&self) -> WebDriverResult { // Step 1. If session's current top-level browsing context is no longer open, @@ -938,7 +990,7 @@ impl Handler { webview_id, self.load_status_sender.clone(), ))?; - self.wait_for_navigation_to_complete() + self.wait_for_document_ready_state() } /// @@ -955,7 +1007,7 @@ impl Handler { webview_id, self.load_status_sender.clone(), ))?; - self.wait_for_navigation_to_complete() + self.wait_for_document_ready_state() } /// @@ -972,7 +1024,7 @@ impl Handler { self.send_message_to_embedder(cmd_msg)?; // Step 4.1: Try to wait for navigation to complete. - self.wait_for_navigation_to_complete()?; + self.wait_for_document_ready_state()?; // Step 5. Set current browsing context with session and current top browsing context. self.session_mut()?.browsing_context_id = BrowsingContextId::from(webview_id); @@ -1107,7 +1159,7 @@ impl Handler { session.window_handles.insert(new_webview_id, new_handle); } - let _ = self.wait_for_navigation_to_complete(); + let _ = self.wait_for_document_ready_state(); Ok(WebDriverResponse::NewWindow(NewWindowResponse { handle, @@ -2088,8 +2140,6 @@ impl Handler { self.handle_any_user_prompts(self.session()?.webview_id)?; let (sender, receiver) = ipc::channel().unwrap(); - let webview_id = self.session()?.webview_id; - let browsing_context_id = self.session()?.browsing_context_id; // Steps 1 - 7 + Step 8 for