After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 667652 - workspaceThumbnail: stray connection to notify::minimized
workspaceThumbnail: stray connection to notify::minimized
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.0.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-01-10 19:27 UTC by Owen Taylor
Modified: 2012-01-17 23:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
workspaceThumbnail: disconnect handlers when workspace is removed (3.09 KB, patch)
2012-01-13 20:26 UTC, Owen Taylor
reviewed Details | Review
workspaceThumbnail: improve handling of notify::minimized signal (5.29 KB, patch)
2012-01-13 20:27 UTC, Owen Taylor
accepted-commit_now Details | Review
workspaceThumbnail: disconnect handlers when workspace is removed (3.09 KB, patch)
2012-01-13 20:32 UTC, Owen Taylor
accepted-commit_now Details | Review
Found some problems with the workspace tracking changes in detailed testing; (2.62 KB, patch)
2012-01-17 22:54 UTC, Owen Taylor
accepted-commit_now Details | Review

Description Owen Taylor 2012-01-10 19:27:59 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.
Comment 1 Owen Taylor 2012-01-10 20:05:07 UTC
Downstream bug in Red Hat Bugzilla - https://bugzilla.redhat.com/show_bug.cgi?id=773059 - we've had this crash reported ~60 times.
Comment 2 Owen Taylor 2012-01-13 20:26:07 UTC
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
Comment 3 Owen Taylor 2012-01-13 20:27:00 UTC
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 4 Owen Taylor 2012-01-13 20:31:28 UTC
Comment on attachment 205227 [details] [review]
workspaceThumbnail: disconnect handlers when workspace is removed

Accidentally attached to the wrong bug
Comment 5 Owen Taylor 2012-01-13 20:32:05 UTC
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
Comment 6 Owen Taylor 2012-01-13 20:42:57 UTC
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.
Comment 7 Colin Walters 2012-01-14 01:56:24 UTC
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?
Comment 8 Colin Walters 2012-01-14 02:05:00 UTC
Review of attachment 205228 [details] [review]:

s/connect/connected/

Makes sense to me.
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-01-16 23:33:24 UTC
Review of attachment 205229 [details] [review]:

This makes sense to me.
Comment 10 Owen Taylor 2012-01-17 15:52:53 UTC
(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.
Comment 11 Owen Taylor 2012-01-17 15:54:32 UTC
Patches pushed
Comment 12 Owen Taylor 2012-01-17 22:54:31 UTC
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.
Comment 13 drago01 2012-01-17 23:23:09 UTC
Review of attachment 205493 [details] [review]:

Both changes make sense to me.