GNOME Bugzilla – Bug 793401
Sushi hangs entire OS when previewing .doc/.odt/.xls
Last modified: 2018-04-27 15:21:30 UTC
Sushi hangs entireOS (ArchLinux) when previewing .doc/.odt/.xls gdm restart won't solve the issue. reproduceable 100%.
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.
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
-> 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
+ Trace 238588
(Actually reassigning to Evince. Evince maintainers, please look at my comment and patch above.)
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?
(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.
Attachment 371448 [details] pushed as b905958 - view: avoid getting pointer position from vertical scroll callback Thanks, I pushed this patch after implementing both suggestions.