GNOME Bugzilla – Bug 754357
gnome-shell segfaults when pasting Xwayland clipboard into gnome-terminal
Last modified: 2015-10-02 10:30:51 UTC
Hello, I'm running Gnome 3.17.90 on Arch Linux with the nouveau driver to test out the Wayland session. When I open Chromium and copy some text and try to paste it into Gnome Terminal the Gnome session crashes and I get dropped to GDM. In coredumpctl I saw that /usr/bin/gnome-shell segfaulted. I had to rebuild mutter with debug info to get info on the last call, so I don't know if that is more of a mutter problem. Core was generated by `/usr/bin/gnome-shell --wayland --display-server'. Program terminated with signal SIGSEGV, Segmentation fault.
+ Trace 235405
When I copy text in gnome-terminal and paste it into the Chromium browser line for example everything is fine.
OK this is strange. It seems to depend from which application you copy and paste into. I used Firefox/Chromium as Xwayland apps and gedit/gnome-terminal as native apps. Copy in Chromium paste in Terminal -> Crash Copy in Chromium paste in gedit -> works Copy in Firefox paste in gedit -> works Copy in Firefox paste in Terminal -> Text doesn't show up and no crash.
This also seems depend on how you copy and paste. In my above example I always right-clicked and selected copy/paste from the context menu. When I Ctrl-C and Ctrl+Shift+V from Firefox into Gnome Terminal the text shows up. When I Ctrl-C from Chromium and Ctrl+Shift+V in Terminal it also work. So this somehow seems to be related to copying using the context menu "paste" option in Gnome Terminal.
putting this on the blocker list for now, since inconsistent clipboard behavior is one of the most annoying issues in the wayland session atm.
Moving this to mutter, since thats where the crash seems to happen. It's also mutter that does the X/Wayland clipboard integration.
As far as I investigated, the misbehavior observed with Chromium was fixed the the patch in gtk+ bug #754158, reverting it on master brings the crash seen in comment #0. I'm attaching nonetheless a patch to protect against the misbehavior on mutter, this would result in no paste happening though unless the gtk+ change is present. But this didn't fix the case of firefox, this is what happens in that case: - Currently we do send wl_data_device.data_offer and wl_data_device.selection everytime the focus changes, even across surfaces of the same client. - Pasting through keyboard shortcuts does word as expected, the troubled case is pasting through the context menu. In that case the focus switch from the context menu to the terminal window triggers a whole new wl_data_offer being sent to the terminal, while there's already ongoing requests on the previous one (so far the TARGETS atom has been requested, which is handled internally by GTK+, which triggers the emission of a GDK_SELECTION_NOTIFY event). - wl_data_device.selection is translated to GDK_OWNER_CHANGE events in GDK, this ends up translating to GtkClipboard::owner-change, which is handled by gnome-terminal code. - At this point it gets royally confusing, there's 3 ongoing petitions for the TARGETS atom, one as a GDK_SELECTION_NOTIFY event corresponding to the previous wl_data_offer, not yet handled by the main loop, another as a result of the wl_data_device.selection event induced by the focus change, and the third one coming from the handling of GtkClibpboard::owner-change in gnome-terminal code. - The UTF8_STRING target is actually picked, GTK+ confusingly thinks the conversion failed due to older events coming at the wrong time, which goes on to trying to pick the next suitable target and actually cancels the transaction on the mutter side, the more generic TEXT target could succeed or not depending on the presence of non-ASCII chars. I think the source of the problem is the emission of wl_data_device.selection events during focus changes within the same client, I'm attaching a second patch to fix that, could be worth though to make gtk+ a bit more resilient here.
Created attachment 311286 [details] [review] xwayland: Protect against crash on x11->wayland transfer cancellation If the transfer is cancelled, the X11SelectionData will be cleared from the MetaSelectionBridge, although x11_data_write_cb() was invariably expecting it to be non-NULL.
Created attachment 311287 [details] [review] wayland: Make focus tracking in MetaWaylandDataDevice per-client Each keyboard focus change ends up calling the MetaWaylandDataDevice counterpart, we don't need though to notify the current selection again. In order to fix this, keep track of the current client, and only emit the relevant signals when the focus switches to another client. The situations where wl_data_device.selection were emitted during focus changes between surfaces of the same client was inocuous most of the times, although it's prone to inducing confusing behavior on context menu clipboard actions, as the closing menu triggers a focus change, which triggers a whole new wl_data_offer being created and given on wl_data_device.selection, at a time where there's already ongoing requests on the previous data offer.
Review of attachment 311286 [details] [review]: Not completely sure I follow. Is the ::: src/wayland/meta-xwayland-selection.c @@ +425,3 @@ } + if (data) Can't this 'data' be a new data unrelated to this cancelled write callback? Shouldn't we just return if there was an error?
Review of attachment 311287 [details] [review]: Looks good to me, but the commit subject doesn't seem correct, since focus tracking is already per client. What was changed is that we don't resend the same state again and again when the focus doesn't change.
(In reply to Jonas Ådahl from comment #8) > Review of attachment 311286 [details] [review] [review]: > > Not completely sure I follow. Is the > > ::: src/wayland/meta-xwayland-selection.c > @@ +425,3 @@ > } > > + if (data) > > Can't this 'data' be a new data unrelated to this cancelled write callback? > Shouldn't we just return if there was an error? You're right, this could conceivably happen. In practice this doesn't happen only because there is enough time until the next meta_x11_source_send() to receive the cancellation orderly. I'm updating the patch. (In reply to Jonas Ådahl from comment #9) > Review of attachment 311287 [details] [review] [review]: > > Looks good to me, but the commit subject doesn't seem correct, since focus > tracking is already per client. What was changed is that we don't resend the > same state again and again when the focus doesn't change. Right, I meant that this function ends up called upon every keyboard focus change, how about "wayland: Avoid resending new data offers on intra-client focus changes"?
Created attachment 311293 [details] [review] xwayland: Protect against crash on x11->wayland transfer cancellation If the transfer is cancelled, the X11SelectionData will be cleared from the MetaSelectionBridge, although x11_data_write_cb() was invariably expecting it to be non-NULL.
> (In reply to Jonas Ådahl from comment #9) > > Review of attachment 311287 [details] [review] [review] [review]: > > > > Looks good to me, but the commit subject doesn't seem correct, since focus > > tracking is already per client. What was changed is that we don't resend the > > same state again and again when the focus doesn't change. > > Right, I meant that this function ends up called upon every keyboard focus > change, how about "wayland: Avoid resending new data offers on intra-client > focus changes"? Sounds good to me.
Review of attachment 311293 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +430,3 @@ + Display *xdisplay = GDK_DISPLAY_XDISPLAY (gdk_display_get_default ()); + XDeleteProperty (xdisplay, selection->window, + gdk_x11_get_xatom_by_name ("_META_SELECTION")); I wonder, don't we need to take care of this property even when we cancel or if there is I/O error? @@ +434,3 @@ + else + { + x11_selection_data_finish (selection, TRUE); This one as well. I guess we need to still call this function, but when canceling before we end up here, and if its just an I/O error, still call it here. (sorry, made it sound like I thought we should early out on error before).
(In reply to Jonas Ådahl from comment #13) > Review of attachment 311293 [details] [review] [review]: > > ::: src/wayland/meta-xwayland-selection.c > @@ +430,3 @@ > + Display *xdisplay = GDK_DISPLAY_XDISPLAY (gdk_display_get_default > ()); > + XDeleteProperty (xdisplay, selection->window, > + gdk_x11_get_xatom_by_name ("_META_SELECTION")); > > I wonder, don't we need to take care of this property even when we cancel or > if there is I/O error? I'm not so sure, in INCR transfers the deletion of that property just hints the X11 selection owner to keep sending further data. Even though there's no ICCCM specified means to cancel that, GTK+ implements an abort timeout on INCR transfers, and generally seems safe around wiping previous data before starting a new transfer. > > @@ +434,3 @@ > + else > + { > + x11_selection_data_finish (selection, TRUE); > > This one as well. I guess we need to still call this function, but when > canceling before we end up here, and if its just an I/O error, still call it > here. (sorry, made it sound like I thought we should early out on error > before). Right, this one should still be called in case of non-cancelled errors, with FALSE on the second argument though, a third iteration coming.
Created attachment 311345 [details] [review] xwayland: Protect against crash on x11->wayland transfer cancellation If the transfer is cancelled, the X11SelectionData will be cleared from the MetaSelectionBridge, although x11_data_write_cb() was invariably expecting it to be non-NULL.
Review of attachment 311345 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +434,3 @@ + if (data) + x11_selection_data_finish (selection, success); 'data' here can still be a replaced X11SelectionData, can't it? I think we need to: 1. call x11_selection_data_finish (selection, FALSE) when canceling (meaning not only in this function, but also when we cancel from) 2. If we were canceled, return early (since 'data' is someone else's now) 3. Still do what you do here (x11_selection_data_finish(..)), but only for when we were not canceled.
(In reply to Jonas Ådahl from comment #16) > Review of attachment 311345 [details] [review] [review]: > > ::: src/wayland/meta-xwayland-selection.c > @@ +434,3 @@ > > + if (data) > + x11_selection_data_finish (selection, success); > > 'data' here can still be a replaced X11SelectionData, can't it? I think we > need to: > 1. call x11_selection_data_finish (selection, FALSE) when canceling > (meaning not only in this function, but also when we cancel from) This vector is already covered, the only relevant thing happening on x11_selection_data_finish here is: g_clear_pointer (&selection->x11_selection, (GDestroyNotify) x11_selection_data_free); Which is what triggers the cancellation in the first place, so data would just be NULL on this callback (and that was the crash reported :) > 2. If we were canceled, return early (since 'data' is someone else's now) > 3. Still do what you do here (x11_selection_data_finish(..)), but only for > when we were not canceled. I'm taking a different approach, I'm just passing the "current" X11SelectionData in the callback user data, so we can bail out if it differs with the current one. Yet another patch coming.
Created attachment 311450 [details] [review] xwayland: Protect against crash on x11->wayland transfer cancellation If the transfer is cancelled, the X11SelectionData will be cleared from the MetaSelectionBridge, although x11_data_write_cb() was invariably expecting it to be non-NULL. Generally, make this async callback safer against cancellation, and possible modification of newer operations that are unrelated to this callback. We now explicitly pass the X11SelectionData in the callback data, and bail out if there's a different operation going on.
Review of attachment 311450 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +438,3 @@ + */ + if (data != selection->x11_selection) + return; I think that this way still won't be reliable. In the g_cancellable_cancel() documentation it says "The convention within GIO is that cancelling an asynchronous operation causes it to complete asynchronously. That is, if you cancel the operation from the same thread in which it is running, then the operation's #GAsyncReadyCallback will not be invoked until the application returns to the main loop." This means that it is theoretically possible that we'll have the following happening: 1. Started an async write on 'data' == 0x123 2. Cancelled the async write, freeing 'data' 3. Started an async write on a new 'data' == 0x123 (that happened to get the same address, even though unlikely) 4. Ended up in this function due to the cancelling in step 2, with the error being a G_IO_ERROR_CANCELLED and data == selection->x11_selection, resulting in us finishing or deleting properties when we shouldn't.
Created attachment 311781 [details] [review] xwayland: Protect against crash on x11->wayland transfer cancellation If the transfer is cancelled, the X11SelectionData will be cleared from the MetaSelectionBridge, although x11_data_write_cb() was invariably expecting it to be non-NULL. If the write was cancelled, all the actions done in x11_data_write_cb() are moot, so just return early. If there's other errors happening (eg. "connection closed" if the target client happens to crash), we should still attempt at clearing the data anyway.
Review of attachment 311781 [details] [review]: Looks good now, assuming the assumption mentioned below is correct. ::: src/wayland/meta-xwayland-selection.c @@ +439,2 @@ { + x11_selection_data_finish (selection, success); I guess we don't need to send DND_FINISHED message when we are cancelled rigth?
(In reply to Jonas Ådahl from comment #21) > Review of attachment 311781 [details] [review] [review]: > > Looks good now, assuming the assumption mentioned below is correct. > > ::: src/wayland/meta-xwayland-selection.c > @@ +439,2 @@ > { > + x11_selection_data_finish (selection, success); > > I guess we don't need to send DND_FINISHED message when we are cancelled > rigth? x11_selection_data_finish() does the following check to send DND_FINISHED or not: if (selection == &selection->x11_selection->selection_data->dnd.selection) In the case of cancellation, selection->x11_selection is already NULL, so it must be done before calling x11_selection_data_free(). There's several places where we call that directly, x11_selection_data_finish() itself is one, and most others are cancellation/shutdown paths where messaging is dubious IMO. The only place that got me wondering is meta_xwayland_selection_handle_selection_notify(), this will happen if XConvertSelection on the X11 drag source failed. However the drag dest can conceivably request another target if this happens, so we should probably not tear down the whole source DnD data. So all in all, I think we're good here.
Comment on attachment 311781 [details] [review] xwayland: Protect against crash on x11->wayland transfer cancellation Attachment 311781 [details] pushed as da0aac6 - xwayland: Protect against crash on x11->wayland transfer cancellation
Comment on attachment 311287 [details] [review] wayland: Make focus tracking in MetaWaylandDataDevice per-client This one was pushed too as: commit b18542f2b6e2120b4a7bb4a961beb306c9e809f7 Author: Carlos Garnacho <carlosg@gnome.org> Date: Mon Sep 14 15:35:13 2015 +0200 wayland: Avoid resending new data offers on intra-client focus changes git-bz didn't pick it up because of the local commit log changes.