Skip to content

Commit ca898ea

Browse files
authored
paint: Propogate input event dispatch failure to Servo to resolve deadlock (servo#43381)
`dispatch_input_event_with_hit_testing` already returns a `bool` to indicate whether the dispatch failed. However, for some reason this was used nowhere. When it fails, embedder is not informed. This is fine for embedder as right now it only handles keyboard shortcuts for headed window, but a disaster for WebDriver as it is synchornously waiting for the feedback. Testing: Several tests no longer CRASH. Fixes: servo#43379 --------- Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
1 parent 23f78a0 commit ca898ea

File tree

15 files changed

+157
-78
lines changed

15 files changed

+157
-78
lines changed

components/paint/paint.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -786,12 +786,14 @@ impl Paint {
786786
.save_capture(capture_path, CaptureBits::all());
787787
}
788788

789-
pub fn notify_input_event(&self, webview_id: WebViewId, event: InputEventAndId) {
789+
/// Returning `false` means this is not going to reach the Constellation,
790+
/// and we need to directly notify the embedder that input event is handled.
791+
pub fn notify_input_event(&self, webview_id: WebViewId, event: InputEventAndId) -> bool {
790792
if self.shutdown_state() != ShutdownState::NotShuttingDown {
791-
return;
793+
return false;
792794
}
793795
self.painter_mut(webview_id.into())
794-
.notify_input_event(webview_id, event);
796+
.notify_input_event(webview_id, event)
795797
}
796798

797799
pub fn notify_scroll_event(&self, webview_id: WebViewId, scroll: Scroll, point: WebViewPoint) {

components/paint/painter.rs

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1305,29 +1305,35 @@ impl Painter {
13051305
.unwrap_or(1.)
13061306
}
13071307

1308-
pub(crate) fn notify_input_event(&mut self, webview_id: WebViewId, event: InputEventAndId) {
1309-
if let Some(webview_renderer) = self.webview_renderers.get_mut(&webview_id) {
1310-
match &event.event {
1311-
InputEvent::MouseMove(event) => {
1312-
// We only track the last mouse move position for non-touch events.
1313-
if !event.is_compatibility_event_for_touch {
1314-
let event_point = event
1315-
.point
1316-
.as_device_point(webview_renderer.device_pixels_per_page_pixel());
1317-
self.last_mouse_move_position = Some(event_point);
1318-
}
1319-
},
1320-
InputEvent::MouseLeftViewport(_) => {
1321-
self.last_mouse_move_position = None;
1322-
},
1323-
_ => {
1324-
// Disable LCP calculation on any other input event except mouse moves.
1325-
self.lcp_calculator.disable_for_webview(webview_id);
1326-
},
1327-
}
1308+
pub(crate) fn notify_input_event(
1309+
&mut self,
1310+
webview_id: WebViewId,
1311+
event: InputEventAndId,
1312+
) -> bool {
1313+
self.webview_renderers
1314+
.get_mut(&webview_id)
1315+
.is_some_and(|webview_renderer| {
1316+
match &event.event {
1317+
InputEvent::MouseMove(event) => {
1318+
// We only track the last mouse move position for non-touch events.
1319+
if !event.is_compatibility_event_for_touch {
1320+
let event_point = event
1321+
.point
1322+
.as_device_point(webview_renderer.device_pixels_per_page_pixel());
1323+
self.last_mouse_move_position = Some(event_point);
1324+
}
1325+
},
1326+
InputEvent::MouseLeftViewport(_) => {
1327+
self.last_mouse_move_position = None;
1328+
},
1329+
_ => {
1330+
// Disable LCP calculation on any other input event except mouse moves.
1331+
self.lcp_calculator.disable_for_webview(webview_id);
1332+
},
1333+
}
13281334

1329-
webview_renderer.notify_input_event(&self.webrender_api, &self.needs_repaint, event);
1330-
}
1335+
webview_renderer.notify_input_event(&self.webrender_api, &self.needs_repaint, event)
1336+
})
13311337
}
13321338

13331339
pub(crate) fn notify_scroll_event(

components/paint/webview_renderer.rs

Lines changed: 40 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -411,18 +411,17 @@ impl WebViewRenderer {
411411
render_api: &RenderApi,
412412
repaint_reason: &Cell<RepaintReason>,
413413
event_and_id: InputEventAndId,
414-
) {
414+
) -> bool {
415415
if let InputEvent::Touch(touch_event) = event_and_id.event {
416-
self.on_touch_event(render_api, repaint_reason, touch_event, event_and_id.id);
417-
return;
416+
return self.on_touch_event(render_api, repaint_reason, touch_event, event_and_id.id);
418417
}
419418

420419
if let InputEvent::Wheel(wheel_event) = event_and_id.event {
421420
self.pending_wheel_events
422421
.insert(event_and_id.id, wheel_event);
423422
}
424423

425-
self.dispatch_input_event_with_hit_testing(render_api, event_and_id);
424+
self.dispatch_input_event_with_hit_testing(render_api, event_and_id)
426425
}
427426

428427
fn send_touch_event(
@@ -458,30 +457,41 @@ impl WebViewRenderer {
458457
repaint_reason: &Cell<RepaintReason>,
459458
event: TouchEvent,
460459
id: InputEventId,
461-
) {
462-
match event.event_type {
460+
) -> bool {
461+
let result = match event.event_type {
463462
TouchEventType::Down => self.on_touch_down(render_api, event, id),
464463
TouchEventType::Move => self.on_touch_move(render_api, event, id),
465464
TouchEventType::Up => self.on_touch_up(render_api, event, id),
466465
TouchEventType::Cancel => self.on_touch_cancel(render_api, event, id),
467-
}
466+
};
468467

469468
self.touch_handler
470469
.add_touch_move_refresh_observer_if_necessary(
471470
self.refresh_driver.clone(),
472471
repaint_reason,
473472
);
473+
result
474474
}
475475

476-
fn on_touch_down(&mut self, render_api: &RenderApi, event: TouchEvent, id: InputEventId) {
476+
fn on_touch_down(
477+
&mut self,
478+
render_api: &RenderApi,
479+
event: TouchEvent,
480+
id: InputEventId,
481+
) -> bool {
477482
let point = event
478483
.point
479484
.as_device_point(self.device_pixels_per_page_pixel());
480485
self.touch_handler.on_touch_down(event.touch_id, point);
481-
self.send_touch_event(render_api, event, id);
486+
self.send_touch_event(render_api, event, id)
482487
}
483488

484-
fn on_touch_move(&mut self, render_api: &RenderApi, mut event: TouchEvent, id: InputEventId) {
489+
fn on_touch_move(
490+
&mut self,
491+
render_api: &RenderApi,
492+
mut event: TouchEvent,
493+
id: InputEventId,
494+
) -> bool {
485495
let point = event
486496
.point
487497
.as_device_point(self.device_pixels_per_page_pixel());
@@ -503,37 +513,45 @@ impl WebViewRenderer {
503513
self.pending_scroll_zoom_events.push(action);
504514
}
505515
}
516+
let mut reached_constellation = false;
506517
// When the event is touchmove, if the script thread is processing the touch
507518
// move event, we skip sending the event to the script thread.
508519
// This prevents the script thread from stacking up for a large amount of time.
509520
if !self.touch_handler.is_handling_touch_move_for_touch_id(
510521
self.touch_handler.current_sequence_id,
511522
event.touch_id,
512-
) && self.send_touch_event(render_api, event, id) &&
513-
event.is_cancelable()
514-
{
515-
self.touch_handler.set_handling_touch_move_for_touch_id(
516-
self.touch_handler.current_sequence_id,
517-
event.touch_id,
518-
TouchIdMoveTracking::Track,
519-
);
523+
) {
524+
reached_constellation = self.send_touch_event(render_api, event, id);
525+
if reached_constellation && event.is_cancelable() {
526+
self.touch_handler.set_handling_touch_move_for_touch_id(
527+
self.touch_handler.current_sequence_id,
528+
event.touch_id,
529+
TouchIdMoveTracking::Track,
530+
);
531+
}
520532
}
533+
reached_constellation
521534
}
522535

523-
fn on_touch_up(&mut self, render_api: &RenderApi, event: TouchEvent, id: InputEventId) {
536+
fn on_touch_up(&mut self, render_api: &RenderApi, event: TouchEvent, id: InputEventId) -> bool {
524537
let point = event
525538
.point
526539
.as_device_point(self.device_pixels_per_page_pixel());
527540
self.touch_handler.on_touch_up(event.touch_id, point);
528-
self.send_touch_event(render_api, event, id);
541+
self.send_touch_event(render_api, event, id)
529542
}
530543

531-
fn on_touch_cancel(&mut self, render_api: &RenderApi, event: TouchEvent, id: InputEventId) {
544+
fn on_touch_cancel(
545+
&mut self,
546+
render_api: &RenderApi,
547+
event: TouchEvent,
548+
id: InputEventId,
549+
) -> bool {
532550
let point = event
533551
.point
534552
.as_device_point(self.device_pixels_per_page_pixel());
535553
self.touch_handler.on_touch_cancel(event.touch_id, point);
536-
self.send_touch_event(render_api, event, id);
554+
self.send_touch_event(render_api, event, id)
537555
}
538556

539557
fn on_touch_event_processed(

components/servo/servo.rs

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,11 @@ enum Message {
134134
FromUnknown(EmbedderMsg),
135135
}
136136

137+
pub struct PendingHandledInputEvent {
138+
pub event_id: InputEventId,
139+
pub webview_id: WebViewId,
140+
}
141+
137142
struct ServoInner {
138143
delegate: RefCell<Rc<dyn ServoDelegate>>,
139144
paint: Rc<RefCell<Paint>>,
@@ -158,6 +163,11 @@ struct ServoInner {
158163
/// and deinitialization of the JS Engine. Multiprocess Servo instances have their
159164
/// own instance that exists in the content process instead.
160165
_js_engine_setup: Option<JSEngineSetup>,
166+
/// [`InputEventId`]s that have been handled, but for which the embedder has
167+
/// not been notified yet.
168+
pending_handled_input_events: RefCell<Vec<PendingHandledInputEvent>>,
169+
/// An [`EventLoopWaker`] used to wake up the main embedder event loop.
170+
event_loop_waker: Box<dyn EventLoopWaker>,
161171
}
162172

163173
impl ServoInner {
@@ -197,6 +207,26 @@ impl ServoInner {
197207
break;
198208
}
199209
}
210+
let pending_handled_input_events =
211+
std::mem::take(&mut *self.pending_handled_input_events.borrow_mut());
212+
for PendingHandledInputEvent {
213+
event_id,
214+
webview_id,
215+
} in pending_handled_input_events
216+
{
217+
self.paint.borrow_mut().notify_input_event_handled(
218+
webview_id,
219+
event_id,
220+
InputEventResult::DispatchFailed,
221+
);
222+
if let Some(webview) = self.get_webview_handle(webview_id) {
223+
webview.delegate().notify_input_event_handled(
224+
webview,
225+
event_id,
226+
InputEventResult::DispatchFailed,
227+
);
228+
}
229+
}
200230

201231
if self.constellation_proxy.disconnected() {
202232
self.delegate
@@ -798,7 +828,7 @@ impl Servo {
798828
time_profiler_chan: time_profiler_chan.clone(),
799829
mem_profiler_chan: mem_profiler_chan.clone(),
800830
shutdown_state: shutdown_state.clone(),
801-
event_loop_waker,
831+
event_loop_waker: event_loop_waker.clone(),
802832
#[cfg(feature = "webxr")]
803833
webxr_registry: builder.webxr_registry,
804834
});
@@ -862,6 +892,8 @@ impl Servo {
862892
webviews: Default::default(),
863893
servo_errors: ServoErrorChannel::default(),
864894
_js_engine_setup: js_engine_setup,
895+
pending_handled_input_events: Default::default(),
896+
event_loop_waker,
865897
}))
866898
}
867899

@@ -935,6 +967,10 @@ impl Servo {
935967
self.0.paint.borrow_mut()
936968
}
937969

970+
pub(crate) fn event_loop_waker(&self) -> &dyn EventLoopWaker {
971+
&*self.0.event_loop_waker
972+
}
973+
938974
pub(crate) fn webviews_mut<'a>(
939975
&'a self,
940976
) -> RefMut<'a, FxHashMap<WebViewId, Weak<RefCell<WebViewInner>>>> {
@@ -948,6 +984,13 @@ impl Servo {
948984
pub(crate) fn javascript_evaluator_mut<'a>(&'a self) -> RefMut<'a, JavaScriptEvaluator> {
949985
self.0.javascript_evaluator.borrow_mut()
950986
}
987+
988+
pub(crate) fn add_pending_handled_input_event(&self, residue_event: PendingHandledInputEvent) {
989+
self.0
990+
.pending_handled_input_events
991+
.borrow_mut()
992+
.push(residue_event);
993+
}
951994
}
952995

953996
fn create_embedder_channel(

components/servo/webview.rs

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ use crate::clipboard_delegate::{ClipboardDelegate, DefaultClipboardDelegate};
3232
#[cfg(feature = "gamepad")]
3333
use crate::gamepad_delegate::{DefaultGamepadDelegate, GamepadDelegate};
3434
use crate::responders::IpcResponder;
35+
use crate::servo::PendingHandledInputEvent;
3536
use crate::webview_delegate::{CreateNewWebViewRequest, DefaultWebViewDelegate, WebViewDelegate};
3637
use crate::{
3738
ColorPicker, ContextMenu, EmbedderControl, InputMethodControl, SelectElement, Servo,
@@ -502,21 +503,23 @@ impl WebView {
502503
pub fn notify_input_event(&self, event: InputEvent) -> InputEventId {
503504
let event: InputEventAndId = event.into();
504505
let event_id = event.id;
505-
506+
let webview_id = self.id();
507+
let servo = &self.inner().servo;
506508
// Events with a `point` first go to `Paint` for hit testing.
507509
if event.event.point().is_some() {
508-
self.inner()
509-
.servo
510-
.paint()
511-
.notify_input_event(self.id(), event);
510+
if !servo.paint().notify_input_event(self.id(), event) {
511+
servo.add_pending_handled_input_event(PendingHandledInputEvent {
512+
event_id,
513+
webview_id,
514+
});
515+
servo.event_loop_waker().wake();
516+
}
512517
} else {
513-
self.inner().servo.constellation_proxy().send(
514-
EmbedderToConstellationMessage::ForwardInputEvent(
515-
self.id(),
516-
event,
517-
None, /* hit_test */
518-
),
519-
);
518+
servo
519+
.constellation_proxy()
520+
.send(EmbedderToConstellationMessage::ForwardInputEvent(
521+
webview_id, event, None, /* hit_test */
522+
));
520523
}
521524

522525
event_id

components/shared/embedder/input_events.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ bitflags! {
3333
/// behavior (such as keybindings) when the WebView has already consumed the event for its
3434
/// own purpose.
3535
const Consumed = 1 << 1;
36+
/// Whether or not the input event failed to dispatch. This can happen when an event
37+
/// is sent while Servo is shutting down or when it is in an intermediate state.
38+
/// Typically these events should be considered to be consumed.
39+
const DispatchFailed = 1 << 2;
3640
}
3741
}
3842

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
[visual-scrollIntoView-001.html]
2-
expected: CRASH
2+
expected: TIMEOUT
33
[Element.scrollIntoView scrolls visually]
4-
expected: FAIL
4+
expected: TIMEOUT
Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
[visual-scrollIntoView-002.html]
2-
expected: CRASH
2+
expected: TIMEOUT
33
[Element.scrollIntoView scrolls visually to a position: fixed element with non-zero layout scroll offset]
4-
expected: FAIL
4+
expected: TIMEOUT
Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
[visual-scrollIntoView-003.html]
2-
expected: CRASH
2+
expected: TIMEOUT
33
[Element.scrollIntoView scrolls visually to an element in nested position: fixed elements]
4-
expected: FAIL
4+
expected: TIMEOUT

tests/wpt/meta/css/selectors/focus-visible-018-2.html.ini

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
[focus-visible-018-2.html]
2-
expected: CRASH
2+
expected: TIMEOUT
33
[Mouse focus does not show a focus ring by default in element TABLE]
44
expected: TIMEOUT
55

0 commit comments

Comments
 (0)