From 7de9716dc04f322636c54db11b1da24aa6cf3e2a Mon Sep 17 00:00:00 2001 From: Narfinger Date: Thu, 8 May 2025 15:51:20 +0200 Subject: [PATCH] Fix a crash in the touch handler. If some move events take the long route over the script thread but get cleaned up by a fling the late one will crash the handler. Now we just ignore move events that are already cleaned up. Signed-off-by: Narfinger --- components/compositing/touch.rs | 186 +++++++++++---------- components/compositing/webview_renderer.rs | 1 + components/shared/embedder/input_events.rs | 1 + ports/servoshell/egl/app_state.rs | 70 ++++---- 4 files changed, 129 insertions(+), 129 deletions(-) diff --git a/components/compositing/touch.rs b/components/compositing/touch.rs index 76d87732b32..fedd569a051 100644 --- a/components/compositing/touch.rs +++ b/components/compositing/touch.rs @@ -292,6 +292,10 @@ impl TouchHandler { .expect("Current Touch sequence does not exist") } + fn try_get_current_touch_sequence_mut(&mut self) -> Option<&mut TouchSequenceInfo> { + self.touch_sequence_map.get_mut(&self.current_sequence_id) + } + pub(crate) fn get_touch_sequence(&self, sequence_id: TouchSequenceId) -> &TouchSequenceInfo { self.touch_sequence_map .get(&sequence_id) @@ -374,81 +378,87 @@ impl TouchHandler { id: TouchId, point: Point2D, ) -> TouchMoveAction { - let touch_sequence = self.get_current_touch_sequence_mut(); - let idx = match touch_sequence - .active_touch_points - .iter_mut() - .position(|t| t.id == id) - { - Some(i) => i, - None => { - error!("Got a touchmove event for a non-active touch point"); - return TouchMoveAction::NoAction; - }, - }; - let old_point = touch_sequence.active_touch_points[idx].point; - let delta = point - old_point; + // As `TouchHandler` is per `WebViewRenderer` which is per `WebView` we might get a Touch Sequence Move that + // started with a down on a different webview. As the touch_sequence id is only changed on touch_down this + // move event gets a touch id which is already cleaned up. + if let Some(touch_sequence) = self.try_get_current_touch_sequence_mut() { + let idx = match touch_sequence + .active_touch_points + .iter_mut() + .position(|t| t.id == id) + { + Some(i) => i, + None => { + error!("Got a touchmove event for a non-active touch point"); + return TouchMoveAction::NoAction; + }, + }; + let old_point = touch_sequence.active_touch_points[idx].point; + let delta = point - old_point; - let action = match touch_sequence.touch_count() { - 1 => { - if let Panning { ref mut velocity } = touch_sequence.state { - // TODO: Probably we should track 1-3 more points and use a smarter algorithm - *velocity += delta; - *velocity /= 2.0; - // update the touch point every time when panning. + let action = match touch_sequence.touch_count() { + 1 => { + if let Panning { ref mut velocity } = touch_sequence.state { + // TODO: Probably we should track 1-3 more points and use a smarter algorithm + *velocity += delta; + *velocity /= 2.0; + // update the touch point every time when panning. + touch_sequence.active_touch_points[idx].point = point; + TouchMoveAction::Scroll(delta, point) + } else if delta.x.abs() > TOUCH_PAN_MIN_SCREEN_PX || + delta.y.abs() > TOUCH_PAN_MIN_SCREEN_PX + { + touch_sequence.state = Panning { + velocity: Vector2D::new(delta.x, delta.y), + }; + // No clicks should be issued after we transitioned to move. + touch_sequence.prevent_click = true; + // update the touch point + touch_sequence.active_touch_points[idx].point = point; + TouchMoveAction::Scroll(delta, point) + } else { + // We don't update the touchpoint, so multiple small moves can + // accumulate and merge into a larger move. + TouchMoveAction::NoAction + } + }, + 2 => { + if touch_sequence.state == Pinching || + delta.x.abs() > TOUCH_PAN_MIN_SCREEN_PX || + delta.y.abs() > TOUCH_PAN_MIN_SCREEN_PX + { + touch_sequence.state = Pinching; + let (d0, c0) = touch_sequence.pinch_distance_and_center(); + // update the touch point with the enough distance or pinching. + touch_sequence.active_touch_points[idx].point = point; + let (d1, c1) = touch_sequence.pinch_distance_and_center(); + let magnification = d1 / d0; + let scroll_delta = c1 - c0 * Scale::new(magnification); + TouchMoveAction::Zoom(magnification, scroll_delta) + } else { + // We don't update the touchpoint, so multiple small moves can + // accumulate and merge into a larger move. + TouchMoveAction::NoAction + } + }, + _ => { touch_sequence.active_touch_points[idx].point = point; - TouchMoveAction::Scroll(delta, point) - } else if delta.x.abs() > TOUCH_PAN_MIN_SCREEN_PX || - delta.y.abs() > TOUCH_PAN_MIN_SCREEN_PX - { - touch_sequence.state = Panning { - velocity: Vector2D::new(delta.x, delta.y), - }; - // No clicks should be issued after we transitioned to move. - touch_sequence.prevent_click = true; - // update the touch point - touch_sequence.active_touch_points[idx].point = point; - TouchMoveAction::Scroll(delta, point) - } else { - // We don't update the touchpoint, so multiple small moves can - // accumulate and merge into a larger move. + touch_sequence.state = MultiTouch; TouchMoveAction::NoAction - } - }, - 2 => { - if touch_sequence.state == Pinching || - delta.x.abs() > TOUCH_PAN_MIN_SCREEN_PX || - delta.y.abs() > TOUCH_PAN_MIN_SCREEN_PX - { - touch_sequence.state = Pinching; - let (d0, c0) = touch_sequence.pinch_distance_and_center(); - // update the touch point with the enough distance or pinching. - touch_sequence.active_touch_points[idx].point = point; - let (d1, c1) = touch_sequence.pinch_distance_and_center(); - let magnification = d1 / d0; - let scroll_delta = c1 - c0 * Scale::new(magnification); - TouchMoveAction::Zoom(magnification, scroll_delta) - } else { - // We don't update the touchpoint, so multiple small moves can - // accumulate and merge into a larger move. - TouchMoveAction::NoAction - } - }, - _ => { - touch_sequence.active_touch_points[idx].point = point; - touch_sequence.state = MultiTouch; - TouchMoveAction::NoAction - }, - }; - // If the touch action is not `NoAction` and the first move has not been processed, - // set pending_touch_move_action. - if TouchMoveAction::NoAction != action && - touch_sequence.prevent_move == TouchMoveAllowed::Pending - { - touch_sequence.update_pending_touch_move_action(action); + }, + }; + // If the touch action is not `NoAction` and the first move has not been processed, + // set pending_touch_move_action. + if TouchMoveAction::NoAction != action && + touch_sequence.prevent_move == TouchMoveAllowed::Pending + { + touch_sequence.update_pending_touch_move_action(action); + } + + action + } else { + TouchMoveAction::NoAction } - - action } pub fn on_touch_up(&mut self, id: TouchId, point: Point2D) { @@ -529,22 +539,24 @@ impl TouchHandler { } pub fn on_touch_cancel(&mut self, id: TouchId, _point: Point2D) { - let touch_sequence = self.get_current_touch_sequence_mut(); - match touch_sequence - .active_touch_points - .iter() - .position(|t| t.id == id) - { - Some(i) => { - touch_sequence.active_touch_points.swap_remove(i); - }, - None => { - warn!("Got a touchcancel event for a non-active touch point"); - return; - }, - } - if touch_sequence.active_touch_points.is_empty() { - touch_sequence.state = Finished; + // A similar thing with touch move can happen here where the event is coming from a different webview. + if let Some(touch_sequence) = self.try_get_current_touch_sequence_mut() { + match touch_sequence + .active_touch_points + .iter() + .position(|t| t.id == id) + { + Some(i) => { + touch_sequence.active_touch_points.swap_remove(i); + }, + None => { + warn!("Got a touchcancel event for a non-active touch point"); + return; + }, + } + if touch_sequence.active_touch_points.is_empty() { + touch_sequence.state = Finished; + } } } } diff --git a/components/compositing/webview_renderer.rs b/components/compositing/webview_renderer.rs index 0a6bdf9ae0a..ee764d47a97 100644 --- a/components/compositing/webview_renderer.rs +++ b/components/compositing/webview_renderer.rs @@ -404,6 +404,7 @@ impl WebViewRenderer { self.dispatch_input_event(event); } + /// Send touch event to the pipeline. Returns true if the event was send fn send_touch_event(&self, mut event: TouchEvent) -> bool { let get_pipeline_details = |pipeline_id| self.pipelines.get(&pipeline_id); let Some(result) = self diff --git a/components/shared/embedder/input_events.rs b/components/shared/embedder/input_events.rs index acaa9afb3ff..34737f827fd 100644 --- a/components/shared/embedder/input_events.rs +++ b/components/shared/embedder/input_events.rs @@ -124,6 +124,7 @@ pub enum TouchEventType { pub struct TouchId(pub i32); /// An ID for a sequence of touch events between a `Down` and the `Up` or `Cancel` event. +/// The ID is the same for all events between `Down`` and `Up`` or `Cancel`` #[repr(transparent)] #[derive(Clone, Copy, Debug, Default, Deserialize, Eq, Hash, PartialEq, Serialize)] pub struct TouchSequenceId(u32); diff --git a/ports/servoshell/egl/app_state.rs b/ports/servoshell/egl/app_state.rs index d75e70c815a..00ec4200da9 100644 --- a/ports/servoshell/egl/app_state.rs +++ b/ports/servoshell/egl/app_state.rs @@ -96,9 +96,6 @@ struct RunningAppStateInner { /// The HiDPI scaling factor to use for the display of [`WebView`]s. hidpi_scale_factor: Scale, - - /// Touch events should not be processed if the compositor is paused - compositor_paused: bool, } struct ServoShellServoDelegate { @@ -317,7 +314,6 @@ impl RunningAppState { focused_webview_id: None, animating_state_changed, hidpi_scale_factor: Scale::new(hidpi_scale_factor), - compositor_paused: false, }), }); @@ -509,54 +505,46 @@ impl RunningAppState { /// Touch event: press down pub fn touch_down(&self, x: f32, y: f32, pointer_id: i32) { - if !self.inner().compositor_paused { - self.active_webview() - .notify_input_event(InputEvent::Touch(TouchEvent::new( - TouchEventType::Down, - TouchId(pointer_id), - Point2D::new(x, y), - ))); - self.perform_updates(); - } + self.active_webview() + .notify_input_event(InputEvent::Touch(TouchEvent::new( + TouchEventType::Down, + TouchId(pointer_id), + Point2D::new(x, y), + ))); + self.perform_updates(); } /// Touch event: move touching finger pub fn touch_move(&self, x: f32, y: f32, pointer_id: i32) { - if !self.inner().compositor_paused { - self.active_webview() - .notify_input_event(InputEvent::Touch(TouchEvent::new( - TouchEventType::Move, - TouchId(pointer_id), - Point2D::new(x, y), - ))); - self.perform_updates(); - } + self.active_webview() + .notify_input_event(InputEvent::Touch(TouchEvent::new( + TouchEventType::Move, + TouchId(pointer_id), + Point2D::new(x, y), + ))); + self.perform_updates(); } /// Touch event: Lift touching finger pub fn touch_up(&self, x: f32, y: f32, pointer_id: i32) { - if !self.inner().compositor_paused { - self.active_webview() - .notify_input_event(InputEvent::Touch(TouchEvent::new( - TouchEventType::Up, - TouchId(pointer_id), - Point2D::new(x, y), - ))); - self.perform_updates(); - } + self.active_webview() + .notify_input_event(InputEvent::Touch(TouchEvent::new( + TouchEventType::Up, + TouchId(pointer_id), + Point2D::new(x, y), + ))); + self.perform_updates(); } /// Cancel touch event pub fn touch_cancel(&self, x: f32, y: f32, pointer_id: i32) { - if !self.inner().compositor_paused { - self.active_webview() - .notify_input_event(InputEvent::Touch(TouchEvent::new( - TouchEventType::Cancel, - TouchId(pointer_id), - Point2D::new(x, y), - ))); - self.perform_updates(); - } + self.active_webview() + .notify_input_event(InputEvent::Touch(TouchEvent::new( + TouchEventType::Cancel, + TouchId(pointer_id), + Point2D::new(x, y), + ))); + self.perform_updates(); } /// Register a mouse movement. @@ -662,7 +650,6 @@ impl RunningAppState { if let Err(e) = self.rendering_context.take_window() { warn!("Unbinding native surface from context failed ({:?})", e); } - self.inner_mut().compositor_paused = true; self.perform_updates(); } @@ -675,7 +662,6 @@ impl RunningAppState { { warn!("Binding native surface to context failed ({:?})", e); } - self.inner_mut().compositor_paused = false; self.perform_updates(); }