gnome-shell crashed with SIGSEGV in _cogl_boxed_value_set_x()

There's some discussion of this in https://bugzilla.gnome.org/show_bug.cgi?id=787240 , but that's really about another crash, so I figure this deserves its own report.

Julian Andres Klode (in #787240), myself and Mikhail Gavrilov (in https://bugzilla.redhat.com/show_bug.cgi?id=1490072 ), and Jean-Baptiste Lallement (in https://bugs.launchpad.net/ubuntu/+source/gnome-shell/+bug/1715330 ) have all seen gnome-shell crash this way (taking down our entire sessions, natch) - a segfault in mutter _cogl_boxed_value_set_x , apparently because somehow it's getting called with NULL as its target. Myself, Julian and Mikhail saw it when opening / starting VMs in virt-manager and Boxes; the Ubuntu reproducer seems to possibly involve some kind of Ubuntu-specific update thing, but these are the cited steps:

Test Case:
Pre-requisites: Package update that downloads a payload and download fails (ie flash plugin)
1. Wait until the update-notifier dialog informing the user about the download failure shows up
2. Click on the 'Execute' buitton
3. Enter your credentials
4. Proceed with the download

Expected result
The package is downloaded

Actual result
This crash happens when the authentication window is displayed

Myself, Mikhail and Jean-Baptiste all reported from 3.25.91, but I think Julian may have tested with something more recent. I don't see any changes between 3.25.91 and 3.25.92 which really look like they ought to fix this in any case.

The bug is in gnome-shell, so moving there.

Created attachment 359569
inhibitShortcuts: Don't destroy actor when hiding

The same dialog can be shown multiple times; to not crash when that
happens we need to avoid destroying the actor when hiding, otherwise
we'll crash when running code that assumes it is still valid.

To actually reproduce this reliably, the patch from bug 787570 is needed. On my set up, this exposes a gtk+/virt-manager bug resulting in the inhibit request being created and destroyed repeatedly.

Uh, must have slipped. Reopening.

The issue is that virt-manager issues a grab() when it gains keyboard focus, and an ungrab() when it loses the keyboard focus.

On Wayland, the grab()/ungrab() being wired to the shortcut inhibit mechanism, which in turn shows the dialog, therefore takes focus away from the virt-manager causes the continuous flickering.

The solution, or at least a compromise, is to keep the dialog around even if the shortcut inhibit is cancelled by the client, so that the user is forced to make a choice that we can reuse on the next request.

Created attachment 359585
[PATCH] wayland: Keep the inhibit shortcut dialog

On Wayland, the grab()/ungrab() in gtk+/gdk are wired to the shortcut
inhibitor mechanism, which in turn shows the dialog, which can take
focus away from the client window when the dialog is shown.

If the client issues an ungrab() when the keyboard focus is lost, we
would hide the dialog, causing the keyboard focus to be returned to the
client surface, which in turn would issue a new grab(), so forth and so
on, causing a continuous show/hide of the shortcut inhibitor dialog.

To avoid this issue, keep the dialog around even if the shortcut inhibit
is canceled by the client, so that the user is forced to make a choice
that we can reuse on the next request without showing the dialog again.

Instead of hiding the dialog when the shortcut inhibitor is destroyed by
the client, we simply mark the request as canceled and do not apply the
user's choice.

Created attachment 359586
[PATCH] wayland: do not leak shortcut inhibit data

We would free the shortcut inhibit data only when the client destroys
its request, which is not the case when the clients itself is
destroyed, leading to a leak of the shortcut inhibit data.

Free the data on resource destruction instead, and simply destroy the
resource on destroy request.

Review of attachment 359585:

Looks good; just one nit about readability.

::: src/wayland/meta-wayland-inhibit-shortcuts-dialog.c
@@ +85,3 @@
+ if (data->request_canceled)
+ return;
+

Would make more sense if you put this condition in the dialog-response function. This being processed from the auto-approve path makes no sense.

Review of attachment 359586:

LGTM.

Created attachment 359592
[PATCH v2] wayland: Keep the inhibit shortcut dialog

(In reply to Jonas Ådahl from comment #8)
> Review of attachment 359585 [details] [review]:
>
> Looks good; just one nit about readability.
>
> ::: src/wayland/meta-wayland-inhibit-shortcuts-dialog.c
> @@ +85,3 @@
> + if (data->request_canceled)
> + return;
> +
>
> Would make more sense if you put this condition in the dialog-response
> function. This being processed from the auto-approve path makes no sense.

Yeap, that also simplifies the patch...

Review of attachment 359592:

lgtm.

Comment on attachment 359586
[PATCH] wayland: do not leak shortcut inhibit data

attachment 359586 pushed to git master as commit 2bf7974 - wayland: do not leak shortcut inhibit data

Comment on attachment 359592
[PATCH v2] wayland: Keep the inhibit shortcut dialog

attachment 359592 pushed to git master as commit 9c16e4e - wayland: Keep the inhibit shortcut dialog

*** Bug 768959 has been marked as a duplicate of this bug. ***

Those commit NNNN links aren't working. Can someone assign this bug to "mutter" to fix that?