GNOME Bugzilla – Bug 756698
wayland: mutter crashes with multiple monitors
Last modified: 2015-11-29 18:29:09 UTC
Created attachment 313465 [details] [review] Set initial cursor position somewhere onto a monitor In multi-monitor setup, if the monitors are positioned so that points 0x0 is not on any monitor's area, mutter crashes because it tires to get monitor on which is the 0x0 point (while creating screen). Present on 3.18 and master.
sorry, meant: point 0x0 is not in any monitor's area
Review of attachment 313465 [details] [review]: ::: src/backends/meta-backend.c @@ +258,3 @@ } +static void get_cursor_initial_position(MetaMonitorManager *manager, int *x, int *y) static void on its own line. Space between the function name and ( @@ +271,3 @@ + { + /* use the first monitor, we just need any valid position. + * We could use the corner point, but let's center it */ Shouldn't we use the primary monitor information? Or really the monitor which happens to have the top panel in the shell? @@ +298,3 @@ + * the point 0x0 must not necessarily be in any monitor's area */ + get_cursor_initial_position (priv->monitor_manager, &init_x, &init_y); + meta_cursor_renderer_set_position (priv->cursor_renderer, init_x, init_y); Why not make it meta_backend_set_initial_cursor_position () so we can keep this function a bit simpler? It'd be self explanatory so we wouldn't need the comment either.
Review of attachment 313465 [details] [review]: Can we simply call the cursor constrain callback to fix this up?
(In reply to Jasper St. Pierre from comment #3) > Review of attachment 313465 [details] [review] [review]: > > Can we simply call the cursor constrain callback to fix this up? It'd be nice to center it before the pointer shows when starting up though.
Not necessarily -- we usually put it at the bottom right of the primary, 20px from the bottom, in an X session. Centering the cursor on the layout means that on a typical desktop multimonitor layout, the cursor will be right in the middle of the two monitors, which looks ugly. Perhaps we should leave initial positioning up to the shell.
(In reply to Jasper St. Pierre from comment #5) > Not necessarily -- we usually put it at the bottom right of the primary, > 20px from the bottom, in an X session. Centering the cursor on the layout > means that on a typical desktop multimonitor layout, the cursor will be > right in the middle of the two monitors, which looks ugly. The centering here is per monitor though, so it would never end up between the monitors. > > Perhaps we should leave initial positioning up to the shell. Yea, I guess this is the only way to center on the output where the panel is. We'd have to do it this early though some how.
Also, I assume the same thing might happen on hotplug, so I think we should probably just constrain the cursor in all cases, and if the shell wants to special-case starting position, it should.
(In reply to Jasper St. Pierre from comment #7) > Also, I assume the same thing might happen on hotplug, so I think we should > probably just constrain the cursor in all cases, and if the shell wants to > special-case starting position, it should. Actually, the cursor is centered via meta_backend_warp_pointer() on the primary monitor on hot-plug (if needed) and after the backend's initialization. After that the constrains shouldn't let it leave the visible area. The problem I'm solving is that the rest of backend's initialization may use the renderer's position information and if it is invalid, mutter can crash. Namely, root_cursor_prepare_at() can be called with invalid x and y and get NULL for a monitor at the [x,y] point. Adding a NULL check there would solve the segfault, but I thought that setting some valid initial position would be better solution.
Downstream report: https://bugzilla.redhat.com/show_bug.cgi?id=1277239 That *seems* to imply that after the reporter hit this once he hits it any time he starts up with the external display connected, but it doesn't quite state it 100%, so I'm not quite sure.
Looking through my stack trace in bug https://bugzilla.redhat.com/show_bug.cgi?id=1277239 my issue is most likely caused by this problem: Using host libthread_db library "/lib64/libthread_db.so.1". Core was generated by `/usr/bin/gnome-shell --wayland --display-server'. Program terminated with signal SIGSEGV, Segmentation fault.
+ Trace 235667
Dump of assembler code for function root_cursor_prepare_at: 0x00007fbec439edf0 <+0>: push %rbx 0x00007fbec439edf1 <+1>: mov %rdi,%rbx 0x00007fbec439edf4 <+4>: mov %rcx,%rdi 0x00007fbec439edf7 <+7>: callq 0x7fbec4361710 <meta_screen_get_monitor_for_point@plt> 0x00007fbec439edfc <+12>: mov %rbx,%rdi => 0x00007fbec439edff <+15>: mov 0x30(%rax),%esi 0x00007fbec439ee02 <+18>: pop %rbx 0x00007fbec439ee03 <+19>: jmpq 0x7fbec435dbf0 <meta_cursor_sprite_set_theme_scale@plt> End of assembler dump. This is a showstopper bug and it has "bricked" the Wayland session for me on that machine.
Marek, are you making a new patch? I guess that at least the constrain-on-hotplug/init would fix the issue, and then smart positioning, we'll deal with later?
(In reply to Jonas Ådahl from comment #11) > Marek, are you making a new patch? I guess that at least the > constrain-on-hotplug/init would fix the issue, and then smart positioning, > we'll deal with later? yep
Created attachment 314790 [details] [review] Set cursor position in cursor-render on hot-plug and after initialization
Review of attachment 314790 [details] [review]: ::: src/backends/meta-backend.c @@ +95,2 @@ static void +get_primary_monitor_center(MetaMonitorManager *monitor_manager, int *x, int *y) Why not just leave this as a center_pointer () helper? That way we don't need to make the call site more complex. There is no need to look at coordinates there. @@ +128,3 @@ + /* we must make sure that cursor renderer does not have invalid + * cursor position until the warp_pointer call takes effect + * (it is usually an event) */ Seeing now that this together with the incorrect stage size is what is causing the problem. Given this, I don't think this patch is enough. If the following happens: 1. pointer movement to (0, 0) -> queue clutter motion event 2. hotplug monitor (causing layout with no monitor at (0, 0)) -> queue motion event to a valid (x, y), set renderer coords to (x, y) 3. process event (0, 0) -> renderer now at invalid position 4. *crash* So given this, it seems to me that we need to either constrain the pointer when processing the motion event, or deal with the case where the pointer momentarily is outside of the screen. @@ +287,3 @@ + /* update monitors information, the cursor tracker initialization needs it */ + on_monitors_changed(priv->monitor_manager, backend); Space mising before (, and you should add a helper doing what you need to do, not abuse a callback.
(In reply to Jonas Ådahl from comment #14) > Review of attachment 314790 [details] [review] [review]: > > ::: src/backends/meta-backend.c > @@ +95,2 @@ > static void > +get_primary_monitor_center(MetaMonitorManager *monitor_manager, int *x, int > *y) > > Why not just leave this as a center_pointer () helper? That way we don't > need to make the call site more complex. There is no need to look at > coordinates there. > > @@ +128,3 @@ > + /* we must make sure that cursor renderer does not have invalid > + * cursor position until the warp_pointer call takes effect > + * (it is usually an event) */ > > Seeing now that this together with the incorrect stage size is what is > causing the problem. Given this, I don't think this patch is enough. If the > following happens: > > 1. pointer movement to (0, 0) -> queue clutter motion event > 2. hotplug monitor (causing layout with no monitor at (0, 0)) -> queue > motion event to a valid (x, y), set renderer coords to (x, y) > 3. process event (0, 0) -> renderer now at invalid position > 4. *crash* Hmm, yes... the pointer is constrained at point 1. when queuing an event, not at point 3. when processing the event > So given this, it seems to me that we need to either constrain the pointer > when processing the motion event, or deal with the case where the pointer > momentarily is outside of the screen. Yes, the first would be more robust (would it be enough just to drop these events?), but the later would be only one NULL check (at least for this particular bug - we would need to check the code for other places where this could fail)
I think it'll be the two "prepare-at" signal listeners that currently can't handle a pointer outside of a monitor, because they cannot find out what scale to use. I suppose we could either fix that some how, or drop the events higher up in the path, in the renderer or tracker or something.
Created attachment 315109 [details] [review] events: drop events with invalid coords Something like this?
Hmm.. I wonder if it woukdn't be better to let the renderer ignore it. Thr problem I see with dropping it this early is uneven button events which might mess things up somewhere.
Created attachment 315126 [details] [review] cursor-renderer: do not render cursor if it is not on monitor
Review of attachment 315126 [details] [review]: tiny style nit ::: src/backends/meta-cursor-renderer.c @@ +123,3 @@ +{ + MetaMonitorManager *monitor_manager = meta_backend_get_monitor_manager (meta_get_backend ()); + if (meta_monitor_manager_get_monitor_at_point (monitor_manager, return meta_monitor_manager_get_monitor_at_point (...) >= 0;
(In reply to Jasper St. Pierre from comment #20) > Review of attachment 315126 [details] [review] [review]: > > tiny style nit > > ::: src/backends/meta-cursor-renderer.c > @@ +123,3 @@ > +{ > + MetaMonitorManager *monitor_manager = meta_backend_get_monitor_manager > (meta_get_backend ()); > + if (meta_monitor_manager_get_monitor_at_point (monitor_manager, > > return meta_monitor_manager_get_monitor_at_point (...) >= 0; Is there a way for me to test this on my machine to see if it works?
AGS: yeah, I can do an F23 build of mutter with the patch applied. I'll do it for you in a few minutes.
Review of attachment 315126 [details] [review]: Looks good to me, with the style nit that Jasper pointed out fixed.
(In reply to Jasper St. Pierre from comment #20) > Review of attachment 315126 [details] [review] [review]: > > tiny style nit > > ::: src/backends/meta-cursor-renderer.c > @@ +123,3 @@ > +{ > + MetaMonitorManager *monitor_manager = meta_backend_get_monitor_manager > (meta_get_backend ()); > + if (meta_monitor_manager_get_monitor_at_point (monitor_manager, > > return meta_monitor_manager_get_monitor_at_point (...) >= 0; I had it this way originally, but then I rewrote it because it did not look so clear. I'll attach new patch
Created attachment 315402 [details] [review] cursor-renderer: do not render cursor if it is not on monitor
Review of attachment 315402 [details] [review]: OK.
*** Bug 758801 has been marked as a duplicate of this bug. ***