GNOME Bugzilla – Bug 667652
workspaceThumbnail: stray connection to notify::minimized
Last modified: 2012-01-17 23:23:09 UTC
To reproduce: - Start shell with a single window on the first workspace - Minimize the window (right click on titlebar) - Go to the activities overview, drag the window to the second workspace (drag from the main area to the second thumbnail) - Click on the window to activate and unminimize it - crashes with an assertion failure in Mutter (meta_workspace_index: Workspace does not exist to index) The problem here is basically that when a window is removed from a workspace while it is minimized, notify::minimized is never disconnected, so the handler for the old destroyed first workspace runs and accesses a removed workspace, triggering the assertion.
Downstream bug in Red Hat Bugzilla - https://bugzilla.redhat.com/show_bug.cgi?id=773059 - we've had this crash reported ~60 times.
Created attachment 205227 [details] [review] workspaceThumbnail: disconnect handlers when workspace is removed We need to remove the handlers when the workspace is removed, not when the animation of it being removed finishes, or we can access a destroyed workspace and triggger an assertion failure in Mutter. https://bugzilla.redhat.com/show_bug.cgi?id=705664
Created attachment 205228 [details] [review] workspaceThumbnail: improve handling of notify::minimized signal There were various cases where we could lose track of a window and leave the notify::minimized signal connect after the actor was destroyed or the workspace removed. This could result in operations on a removed workspace and assertion failures inside Mutter. https://bugzilla.redhat.com/show_bug.cgi?id=773059
Comment on attachment 205227 [details] [review] workspaceThumbnail: disconnect handlers when workspace is removed Accidentally attached to the wrong bug
Created attachment 205229 [details] [review] workspaceThumbnail: disconnect handlers when workspace is removed We need to remove the handlers when the workspace is removed, not when the animation of it being removed finishes, or we can access a destroyed workspace and triggger an assertion failure in Mutter. https://bugzilla.redhat.com/show_bug.cgi?id=705664
Sorry for the confusion - thought I had two GNOME bugs, but there was only one. The two patches here both fix problems with signal tracking in workspaceThumbnail.js that can lead to crashes - first one depends on the second one. Note that the tracking of the windows in _allWindows depends on the window-added and window-removed signals, but these aren't actually emitted when a window is made sticky or not sticky. This can result in some unexpected stuff. - Window is mapped sticky - User goes to overview - Window makes itself non-sticky - thumbnails don't update - Window is destroyed - window-left-monitor is emitted, and the window thumbnail actor is is visually destroyed on all thumbnails, but left in _allWindows. But the only bad effect here is that we leak the window until the activities overview is closed. I think this patch is fine until we fix the thumbnail actors to properly track stick/unstick, which will requires changes in Mutter.
Review of attachment 205227 [details] [review]: ::: js/ui/workspaceThumbnail.js @@ -286,2 @@ _doAddWindow : function(metaWin) { - if (this.leavingOverview) Because WorkspaceThumbnail doesn't inherit from Workspace as far as I can see, I doubt this.leavingOverview was ever set to anything other than undefined. But are we missing any safety by deleting this?
Review of attachment 205228 [details] [review]: s/connect/connected/ Makes sense to me.
Review of attachment 205229 [details] [review]: This makes sense to me.
(In reply to comment #7) > Review of attachment 205227 [details] [review]: > > ::: js/ui/workspaceThumbnail.js > @@ -286,2 @@ > _doAddWindow : function(metaWin) { > - if (this.leavingOverview) > > Because WorkspaceThumbnail doesn't inherit from Workspace as far as I can see, > I doubt this.leavingOverview was ever set to anything other than undefined. > > But are we missing any safety by deleting this? leavingOverview has to do with not showing animations of windows being added while we were doing the from-overview animation - http://git.gnome.org/browse/gnome-shell/commit/?id=c018b7652f1b3c9274b430f2d8f4e784f76fb33e so there's no safety issue, though I guess when launching a window and leaving the overview it could be considered a little noisy to show the window popping up on the workspace thumbnail as the workspace thumbnail is being hidden.
Patches pushed
Created attachment 205493 [details] [review] Found some problems with the workspace tracking changes in detailed testing; the good thing is that what I didn't get were any of the crashes that I was trying to fix - this just fixes up misbehaviors. workspaceThumbnail: fix window tracking bugs - We should only call workspaceRemoved() for workspaces that are are actually being removed. - When we have multiple monitors, a window on a secondary monitor is on all workspaces, so it ends up in all workspaces _allWindows lists, so we can't use previous presence in that list to determine whether we need to go ahead and add the actor; allWindows is simply the list of windows where we are listening to notify::minimized.
Review of attachment 205493 [details] [review]: Both changes make sense to me.