From b422a65a00c51d732e4a7f7e4a049e0ad830c7a9 Mon Sep 17 00:00:00 2001 From: batu_hoang <55729155+longvatrong111@users.noreply.github.com> Date: Wed, 4 Jun 2025 13:56:20 +0800 Subject: [PATCH] Add retry for hit tests with expired epoch in result (#37085) Follow up to [hit_test failed occasionally when the touch event is sent](https://github.com/servo/servo/issues/35788#top) and https://github.com/servo/servo/issues/36676#issuecomment-2882917136, this PR adds a retry for hit tests with expired epoch in result. Hit tests with outdated epoch mean it is too early to perform the hit test. Solution: retry hit test for the event on the next webrender frame. The retry should guarantee that: - Keep the correct order of events - Retry time is not too long Test cases: `./mach test-wpt --product servodriver -r tests\wpt\tests\webdriver\tests\classic\element_click\events.py` cc: @xiaochengh , @yezhizhen --------- Signed-off-by: batu_hoang --- components/compositing/compositor.rs | 68 +++++++++--- components/compositing/webview_renderer.rs | 117 +++++++++++++++------ 2 files changed, 134 insertions(+), 51 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index a76b0022122..0acbbec977a 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -281,6 +281,11 @@ impl PipelineDetails { } } +pub enum HitTestError { + EpochMismatch, + Others, +} + impl ServoRenderer { pub fn shutdown_state(&self) -> ShutdownState { self.shutdown_state.get() @@ -290,15 +295,19 @@ impl ServoRenderer { &self, point: DevicePoint, details_for_pipeline: impl Fn(PipelineId) -> Option<&'a PipelineDetails>, - ) -> Option { - self.hit_test_at_point_with_flags_and_pipeline( + ) -> Result { + match self.hit_test_at_point_with_flags_and_pipeline( point, HitTestFlags::empty(), None, details_for_pipeline, - ) - .first() - .cloned() + ) { + Ok(hit_test_results) => hit_test_results + .first() + .cloned() + .ok_or(HitTestError::Others), + Err(error) => Err(error), + } } // TODO: split this into first half (global) and second half (one for whole compositor, one for webview) @@ -308,14 +317,15 @@ impl ServoRenderer { flags: HitTestFlags, pipeline_id: Option, details_for_pipeline: impl Fn(PipelineId) -> Option<&'a PipelineDetails>, - ) -> Vec { + ) -> Result, HitTestError> { // DevicePoint and WorldPoint are the same for us. let world_point = WorldPoint::from_untyped(point.to_untyped()); let results = self.webrender_api .hit_test(self.webrender_document, pipeline_id, world_point, flags); - results + let mut epoch_mismatch = false; + let results = results .items .iter() .filter_map(|item| { @@ -323,10 +333,16 @@ impl ServoRenderer { let details = details_for_pipeline(pipeline_id)?; // If the epoch in the tag does not match the current epoch of the pipeline, - // then the hit test is against an old version of the display list and we - // should ignore this hit test for now. + // then the hit test is against an old version of the display list. match details.most_recent_display_list_epoch { - Some(epoch) if epoch.as_u16() == item.tag.1 => {}, + Some(epoch) => { + if epoch.as_u16() != item.tag.1 { + // It's too early to hit test for now. + // New scene building is in progress. + epoch_mismatch = true; + return None; + } + }, _ => return None, } @@ -340,7 +356,13 @@ impl ServoRenderer { scroll_tree_node: info.scroll_tree_node, }) }) - .collect() + .collect(); + + if epoch_mismatch { + return Err(HitTestError::EpochMismatch); + } + + Ok(results) } pub(crate) fn send_transaction(&mut self, transaction: Transaction) { @@ -619,7 +641,7 @@ impl IOCompositor { .global .borrow() .hit_test_at_point(point, details_for_pipeline); - if let Some(result) = result { + if let Ok(result) = result { self.global.borrow_mut().update_cursor(point, &result); } } @@ -649,7 +671,7 @@ impl IOCompositor { }; let dppx = webview_renderer.device_pixels_per_page_pixel(); let point = dppx.transform_point(Point2D::new(x, y)); - webview_renderer.dispatch_input_event( + webview_renderer.dispatch_point_input_event( InputEvent::MouseButton(MouseButtonEvent::new(action, button, point)) .with_webdriver_message_id(Some(message_id)), ); @@ -662,7 +684,7 @@ impl IOCompositor { }; let dppx = webview_renderer.device_pixels_per_page_pixel(); let point = dppx.transform_point(Point2D::new(x, y)); - webview_renderer.dispatch_input_event( + webview_renderer.dispatch_point_input_event( InputEvent::MouseMove(MouseMoveEvent::new(point)) .with_webdriver_message_id(Some(message_id)), ); @@ -684,7 +706,7 @@ impl IOCompositor { let scroll_delta = dppx.transform_vector(Vector2D::new(delta_x as f32, delta_y as f32)); webview_renderer - .dispatch_input_event(InputEvent::Wheel(WheelEvent { delta, point })); + .dispatch_point_input_event(InputEvent::Wheel(WheelEvent { delta, point })); webview_renderer.on_webdriver_wheel_action(scroll_delta, point); }, @@ -792,6 +814,8 @@ impl IOCompositor { let Some(webview_renderer) = self.webview_renderers.get_mut(webview_id) else { return warn!("Could not find WebView for incoming display list"); }; + // WebRender is not ready until we receive "NewWebRenderFrameReady" + webview_renderer.webrender_frame_ready.set(false); let pipeline_id = display_list_info.pipeline_id; let details = webview_renderer.ensure_pipeline_details(pipeline_id.into()); @@ -841,7 +865,8 @@ impl IOCompositor { flags, pipeline, details_for_pipeline, - ); + ) + .unwrap_or_default(); let _ = sender.send(result); }, @@ -1629,11 +1654,20 @@ impl IOCompositor { }, CompositorMsg::NewWebRenderFrameReady(..) => { found_recomposite_msg = true; - compositor_messages.push(msg) + compositor_messages.push(msg); }, _ => compositor_messages.push(msg), } } + + if found_recomposite_msg { + // Process all pending events + self.webview_renderers.iter().for_each(|webview| { + webview.dispatch_pending_point_input_events(); + webview.webrender_frame_ready.set(true); + }); + } + for msg in compositor_messages { self.handle_browser_message(msg); diff --git a/components/compositing/webview_renderer.rs b/components/compositing/webview_renderer.rs index b0e91ccb02e..84d5fd877e6 100644 --- a/components/compositing/webview_renderer.rs +++ b/components/compositing/webview_renderer.rs @@ -2,9 +2,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use std::cell::RefCell; -use std::collections::HashMap; +use std::cell::{Cell, RefCell}; use std::collections::hash_map::Keys; +use std::collections::{HashMap, VecDeque}; use std::rc::Rc; use base::id::{PipelineId, WebViewId}; @@ -25,7 +25,7 @@ use webrender_api::units::{ }; use webrender_api::{ExternalScrollId, HitTestFlags, ScrollLocation}; -use crate::compositor::{PipelineDetails, ServoRenderer}; +use crate::compositor::{HitTestError, PipelineDetails, ServoRenderer}; use crate::touch::{TouchHandler, TouchMoveAction, TouchMoveAllowed, TouchSequenceState}; // Default viewport constraints @@ -98,6 +98,10 @@ pub(crate) struct WebViewRenderer { /// Whether or not this [`WebViewRenderer`] isn't throttled and has a pipeline with /// active animations or animation frame callbacks. animating: bool, + /// Pending input events queue. Priavte and only this thread pushes events to it. + pending_point_input_events: RefCell>, + /// WebRender is not ready between `SendDisplayList` and `WebRenderFrameReady` messages. + pub webrender_frame_ready: Cell, } impl Drop for WebViewRenderer { @@ -132,6 +136,8 @@ impl WebViewRenderer { max_viewport_zoom: None, hidpi_scale_factor: Scale::new(hidpi_scale_factor.0), animating: false, + pending_point_input_events: Default::default(), + webrender_frame_ready: Cell::default(), } } @@ -309,29 +315,89 @@ impl WebViewRenderer { } } - pub(crate) fn dispatch_input_event(&mut self, event: InputEvent) { + pub(crate) fn dispatch_point_input_event(&mut self, mut event: InputEvent) -> bool { // Events that do not need to do hit testing are sent directly to the // constellation to filter down. let Some(point) = event.point() else { - return; + return false; }; + // Delay the event if the epoch is not synchronized yet (new frame is not ready), + // or hit test result would fail and the event is rejected anyway. + if !self.webrender_frame_ready.get() || !self.pending_point_input_events.borrow().is_empty() + { + self.pending_point_input_events + .borrow_mut() + .push_back(event); + return false; + } + // If we can't find a pipeline to send this event to, we cannot continue. let get_pipeline_details = |pipeline_id| self.pipelines.get(&pipeline_id); - let Some(result) = self + let result = match self .global .borrow() .hit_test_at_point(point, get_pipeline_details) - else { - return; + { + Ok(hit_test_results) => hit_test_results, + Err(HitTestError::EpochMismatch) => { + self.pending_point_input_events + .borrow_mut() + .push_back(event); + return false; + }, + _ => { + return false; + }, }; - self.global.borrow_mut().update_cursor(point, &result); + match event { + InputEvent::Touch(ref mut touch_event) => { + touch_event.init_sequence_id(self.touch_handler.current_sequence_id); + }, + InputEvent::MouseButton(_) | InputEvent::MouseMove(_) | InputEvent::Wheel(_) => { + self.global.borrow_mut().update_cursor(point, &result); + }, + _ => unreachable!("Unexpected input event type: {event:?}"), + } if let Err(error) = self.global.borrow().constellation_sender.send( EmbedderToConstellationMessage::ForwardInputEvent(self.id, event, Some(result)), ) { warn!("Sending event to constellation failed ({error:?})."); + false + } else { + true + } + } + + pub(crate) fn dispatch_pending_point_input_events(&self) { + while let Some(event) = self.pending_point_input_events.borrow_mut().pop_front() { + // Events that do not need to do hit testing are sent directly to the + // constellation to filter down. + let Some(point) = event.point() else { + continue; + }; + + // If we can't find a pipeline to send this event to, we cannot continue. + let get_pipeline_details = |pipeline_id| self.pipelines.get(&pipeline_id); + let Ok(result) = self + .global + .borrow() + .hit_test_at_point(point, get_pipeline_details) + else { + // Don't need to process pending input events in this frame any more. + // TODO: Add multiple retry later if needed. + return; + }; + + self.global.borrow_mut().update_cursor(point, &result); + + if let Err(error) = self.global.borrow().constellation_sender.send( + EmbedderToConstellationMessage::ForwardInputEvent(self.id, event, Some(result)), + ) { + warn!("Sending event to constellation failed ({error:?})."); + } } } @@ -401,29 +467,11 @@ impl WebViewRenderer { } } - self.dispatch_input_event(event); + self.dispatch_point_input_event(event); } - fn send_touch_event(&self, mut event: TouchEvent) -> bool { - let get_pipeline_details = |pipeline_id| self.pipelines.get(&pipeline_id); - let Some(result) = self - .global - .borrow() - .hit_test_at_point(event.point, get_pipeline_details) - else { - return false; - }; - - event.init_sequence_id(self.touch_handler.current_sequence_id); - let event = InputEvent::Touch(event); - if let Err(e) = self.global.borrow().constellation_sender.send( - EmbedderToConstellationMessage::ForwardInputEvent(self.id, event, Some(result)), - ) { - warn!("Sending event to constellation failed ({:?}).", e); - false - } else { - true - } + fn send_touch_event(&mut self, event: TouchEvent) -> bool { + self.dispatch_point_input_event(InputEvent::Touch(event)) } pub(crate) fn on_touch_event(&mut self, event: TouchEvent) { @@ -687,13 +735,13 @@ impl WebViewRenderer { /// fn simulate_mouse_click(&mut self, point: DevicePoint) { let button = MouseButton::Left; - self.dispatch_input_event(InputEvent::MouseMove(MouseMoveEvent::new(point))); - self.dispatch_input_event(InputEvent::MouseButton(MouseButtonEvent::new( + self.dispatch_point_input_event(InputEvent::MouseMove(MouseMoveEvent::new(point))); + self.dispatch_point_input_event(InputEvent::MouseButton(MouseButtonEvent::new( MouseButtonAction::Down, button, point, ))); - self.dispatch_input_event(InputEvent::MouseButton(MouseButtonEvent::new( + self.dispatch_point_input_event(InputEvent::MouseButton(MouseButtonEvent::new( MouseButtonAction::Up, button, point, @@ -858,7 +906,8 @@ impl WebViewRenderer { HitTestFlags::FIND_ALL, None, get_pipeline_details, - ); + ) + .unwrap_or_default(); // Iterate through all hit test results, processing only the first node of each pipeline. // This is needed to propagate the scroll events from a pipeline representing an iframe to