After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 793401 - Sushi hangs entire OS when previewing .doc/.odt/.xls
Sushi hangs entire OS when previewing .doc/.odt/.xls
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: general
3.28.x
Other Linux
: Urgent critical
: ---
Assigned To: Evince Maintainers
Evince Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-02-12 19:04 UTC by jhanyung
Modified: 2018-04-27 15:21 UTC
See Also:
GNOME target: ---
GNOME version: 3.25/3.26


Attachments
view: avoid getting pointer position from vertical scroll callback (3.61 KB, patch)
2018-04-27 01:25 UTC, Cosimo Cecchi
committed Details | Review

Description jhanyung 2018-02-12 19:04:34 UTC
Sushi hangs entireOS (ArchLinux) when previewing .doc/.odt/.xls

gdm restart won't solve the issue. 

reproduceable 100%.
Comment 1 Cosimo Cecchi 2018-04-25 20:25:31 UTC
I can reproduce this as well, unfortunately. It does not look like anything significantly changed in Sushi recently to cause this issue, but I'm investigating it.
Comment 2 Cosimo Cecchi 2018-04-27 01:25:08 UTC
Created attachment 371448 [details] [review]
view: avoid getting pointer position from vertical scroll callback

When an EvView is embedded in a ClutterGTK stage, such as when it's used
in sushi, calling into gdk_window_get_device_position() can result in an
infinite loop, as it will force an allocation cycle inside Clutter,
which results in a forced immediate draw event being delivered to the
scrolled window while it's processing the scroll event.

There may be more general issues with the ClutterGTK implementation, but
in the meantime it's easy to avoid the issue from Evince:
- if the adjustment changed value directly because of a scroll event, we
  can get the mouse pointer coordinates directly from the event
- otherwise (e.g. scrollbar, or keynav), we can defer the update of the
  cursor to an idle handler, which avoids the deadlock
Comment 3 Cosimo Cecchi 2018-04-27 01:32:00 UTC
-> evince

I tracked this down to a bad interaction between ClutterGTK and Evince. (sushi still uses ClutterGTK.)

During the vertical scroll callback (i.e. when the vertical adjustment value changes), EvView will call gdk_window_get_device_position() to get the cursor position and update it if it's now e.g. on top of a link.

Unfortunately, due to how ClutterGTK works, that can trigger an allocation cycle of the Clutter stage, which results in an immediate draw being sent to the scrolled window while it's still processing callbacks for the scroll event.
It's my understanding (but that may not be 100% correct since I'm not completely familiar with ClutterGTK internals) that effectively it results in the scroll event being "starved" and re-processed after the draw has completed, which ends up in the same code path again, effectively deadlocking.

The proposed patch sidesteps the issue by either getting the pointer coordinates without the need of a roundtrip through ClutterGTK, or deferring that to an idle handler in case that's not possible.

For those curious, the deadlock backtrace is here below.

Thanks for considering this patch.

Thread 1 "sushi-start" received signal SIGINT, Interrupt.
g_type_check_instance_is_a (type_instance=type_instance@entry=0x555555ff2310 [ClutterStage], iface_type=<optimized out>) at ../../../../gobject/gtype.c:4016
4016	../../../../gobject/gtype.c: No such file or directory.
(gdb) bt
  • #0 g_type_check_instance_is_a
    at ../../../../gobject/gtype.c line 4016
  • #1 clutter_actor_get_text_direction
    at clutter-actor.c line 16509
  • #2 clutter_actor_adjust_width
    at clutter-actor.c line 9311
  • #3 clutter_actor_adjust_allocation
    at clutter-actor.c line 9907
  • #4 clutter_actor_allocate
    at clutter-actor.c line 10025
  • #5 _clutter_stage_maybe_relayout
    at clutter-stage.c line 1096
  • #6 clutter_actor_get_abs_allocation_vertices
    at clutter-actor.c line 3052
  • #7 clutter_actor_transform_stage_point
    at clutter-actor.c line 15158
  • #8 offscreen_window_from_parent
    at gtk-clutter-offscreen.c line 91
  • #12 <emit signal 0x7fffef38bf19 "from-embedder" on instance 0x555555f26e30 [GdkX11Window]>
    at ../../../../gobject/gsignal.c line 3487
  • #13 from_embedder
    at ../../../../gdk/gdkoffscreenwindow.c line 233
  • #14 gdk_offscreen_window_get_device_state
    at ../../../../gdk/gdkoffscreenwindow.c line 306
  • #15 gdk_window_get_device_position_double
    at ../../../../gdk/gdkwindow.c line 5025
  • #16 gdk_window_get_device_position
    at ../../../../gdk/gdkwindow.c line 5076
  • #17 ev_document_misc_get_pointer_position
    at ev-document-misc.c line 568
  • #18 on_adjustment_value_changed
    at ev-view.c line 7847
  • #22 <emit signal ??? on instance 0x555555cd1c30 [GtkAdjustment]>
    at ../../../../gobject/gsignal.c line 3447
  • #23 emit_value_changed
    at ../../../../gtk/gtkadjustment.c line 349
  • #24 adjustment_set_value
    at ../../../../gtk/gtkadjustment.c line 450
  • #25 gtk_adjustment_set_value_internal
    at ../../../../gtk/gtkadjustment.c line 545
  • #26 gtk_adjustment_set_value
    at ../../../../gtk/gtkadjustment.c line 567
  • #27 _gtk_scrolled_window_set_adjustment_value
    at ../../../../gtk/gtkscrolledwindow.c line 3626
  • #28 gtk_scrolled_window_scroll_event
    at ../../../../gtk/gtkscrolledwindow.c line 3513
  • #29 _gtk_marshal_BOOLEAN__BOXEDv
    at ../../../../gtk/gtkmarshalers.c line 128
  • #30 _g_closure_invoke_va
    at ../../../../gobject/gclosure.c line 867
  • #31 g_signal_emit_valist
    at ../../../../gobject/gsignal.c line 3300
  • #32 g_signal_emit
    at ../../../../gobject/gsignal.c line 3447
  • #33 gtk_widget_event_internal
    at ../../../../gtk/gtkwidget.c line 7740
  • #34 propagate_event_up
    at ../../../../gtk/gtkmain.c line 2592
  • #35 propagate_event
    at ../../../../gtk/gtkmain.c line 2694
  • #36 gtk_main_do_event
    at ../../../../gtk/gtkmain.c line 1915
  • #37 _gdk_event_emit
    at ../../../../gdk/gdkevents.c line 73
  • #38 gdk_event_source_dispatch
    at ../../../../../gdk/x11/gdkeventsource.c line 367
  • #39 g_main_dispatch
    at ../../../../glib/gmain.c line 3182
  • #40 g_main_context_dispatch
    at ../../../../glib/gmain.c line 3847
  • #41 g_main_context_iterate
    at ../../../../glib/gmain.c line 3920
  • #42 g_main_context_iteration
    at ../../../../glib/gmain.c line 3981
  • #43 g_application_run
    at ../../../../gio/gapplication.c line 2401
  • #44 ffi_call_unix64
  • #45 ffi_call
  • #46 0x00007ffff719ec91 in
  • #47 0x00007ffff71a04b2 in
  • #48 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)
    at ./js/src/jscntxtinlines.h line 239
  • #49 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)
    at ./js/src/vm/Interpreter.cpp line 447
  • #50 js::CallFromStack(JSContext*, JS::CallArgs const&)
    at ./js/src/vm/Interpreter.cpp line 510
  • #51 Interpret(JSContext*, js::RunState&)
    at ./js/src/vm/Interpreter.cpp line 2922
  • #52 js::RunScript(JSContext*, js::RunState&)
    at ./js/src/vm/Interpreter.cpp line 405
  • #53 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*)
    at ./js/src/vm/Interpreter.cpp line 686
  • #54 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*)
    at ./js/src/vm/Interpreter.cpp line 719
  • #55 ExecuteScript(JSContext*, JS::HandleObject, JS::HandleScript, JS::Value*)
    at ./js/src/jsapi.cpp line 4350
  • #56 ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::HandleScript, JS::Value*)
    at ./js/src/jsapi.cpp line 4369
  • #57 gjs_eval_with_scope
  • #58 gjs_context_eval
  • #59 main
    at main.c line 127

Comment 4 Cosimo Cecchi 2018-04-27 01:34:48 UTC
(Actually reassigning to Evince. Evince maintainers, please look at my comment and patch above.)
Comment 5 Carlos Garcia Campos 2018-04-27 12:56:30 UTC
Review of attachment 371448 [details] [review]:

Ok, solution looks simple. Thanks!

::: libview/ev-view-private.h
@@ +169,3 @@
 	gint scroll_y;	
 
+	guint scroll_update_cursor_id;

I would just call this update_cursor_idle_id

::: libview/ev-view.c
@@ +7907,3 @@
+
+static void
+queue_scroll_cursor_update (EvView *view)

This is not really queued, I would use schedule instead.

@@ +7977,3 @@
+			x = ((GdkEventScroll *)event)->x;
+			y = ((GdkEventScroll *)event)->y;
+			ev_view_handle_cursor_over_xy (view, x, y);

I guess we could use event->scroll.window/x/y instead of the triple cast no?
Comment 6 Emmanuele Bassi (:ebassi) 2018-04-27 13:10:00 UTC
(In reply to Carlos Garcia Campos from comment #5)

> @@ +7977,3 @@
> +			x = ((GdkEventScroll *)event)->x;
> +			y = ((GdkEventScroll *)event)->y;
> +			ev_view_handle_cursor_over_xy (view, x, y);
> 
> I guess we could use event->scroll.window/x/y instead of the triple cast no?

I'd actually strongly encourage you to use gdk_event_get_coords() instead of directly accessing the event data structure — something that won't be possible at all with GTK+ 4 and later; of course, the whole event handling will likely need to be ported to gestures, but it's a good idea to start using the GdkEvent accessor functions wherever possible.
Comment 7 Cosimo Cecchi 2018-04-27 15:21:26 UTC
Attachment 371448 [details] pushed as b905958 - view: avoid getting pointer position from vertical scroll callback

Thanks, I pushed this patch after implementing both suggestions.