GNOME Bugzilla – Bug 722434
Fix crash in app switcher corner case
Last modified: 2014-01-22 18:58:38 UTC
To reproduce the issue (from https://bugzilla.redhat.com/show_bug.cgi?id=1012844#c22): 1) Run gpk-application 2) Activate "Open Software Log" in the app menu 3) Switch windows using Alt+` The problem is that both gpk-log and gpk-application show up in the popup, but both windows are associated with the latter, so gpk-log ends up with an empty window list - we are not prepared for this case and hit a Clutter assertion later. The 1st patch just guards against hitting the assertion - there's always the possibility of bugs resulting in an empty window list, and crashing is rather harsh (not to mention that the stderr warnings when not crashing are a much better pointer than the backtrace of the crash in this case). The 2nd patch addresses the actual issue which leads to an empty window list in the popup, but as it touches how we associated windows to apps it may have undesirable side effects (the patch does change behavior elsewhere, for instance running apps in the dash will end up with the same grouping as the app switcher - I consider that an improvement, but there may well be other cases). The 3rd patch is a safe+boring alternative to the 2nd one.
Created attachment 266567 [details] [review] switcherPopup: Fix spacing calculation for empty lists Without special-casing, our current spacing calculation results in negative size requests for empty lists, which will trigger a Clutter assert later. While the list is never supposed to be empty, bugs happen; crashing users' systems is the least graceful way of handling this, so don't do it.
Created attachment 266568 [details] [review] window-tracker: Always enable transient_for redirection It is possible to associate an application's window with a different application using the transient_for hint. However we currently only consider the hint in get_window_app() and not when making the original association, which opens the door to some confusing inconsistencies; for instance, get_window_app() will not necessarily return the same value for all windows retrieved via shell_app_get_windows(). Fix this by looking at the transient_for hint when making the original association, not just in get_window_app().
Created attachment 266569 [details] [review] altTab: Always filter out items with no windows When restricting the switcher popup to the current workspace, we filter out running apps with an empty window list (namely: no open windows on the current workspace). However we may end up with an empty window list even when not restricting items to the current workspace when all windows of a running app are associated with a different application via the transient_for hint. To fix this, just filter out items with an empty window list unconditionally.
I must say I like the second patch, and it makes sense to me: if a window is transient to another, it logically belongs to the same group of the first window, and therefore should be in the same application. I would even say it's an application bug that TRANSIENT_FOR is set for two apps with two different WM_CLASS value. So I support the small behavior change, which should not break anything in practice.
Review of attachment 266567 [details] [review]: This is a good idea anyway.
Review of attachment 266568 [details] [review]: ::: src/shell-window-tracker.c @@ +390,3 @@ + transient_for = meta_window_get_transient_for (window); + if (transient_for != NULL) + window = transient_for; Should we loop until we find a window with null transient-for? Say, a non-modal dialog that opens a dialog...
Review of attachment 266569 [details] [review]: I like the patch because it removes a check, but I'd like a comment saying that the condition is only true if (workspace != null || there were bugs)
Created attachment 266990 [details] [review] window-tracker: Always enable transient_for redirection (In reply to comment #6) > Should we loop until we find a window with null transient-for? Sounds correct to me, yeah.
Review of attachment 266990 [details] [review]: LGTM
Created attachment 266991 [details] [review] altTab: Always filter out items with no windows (In reply to comment #7) > I like the patch because it removes a check, but I'd like a comment saying that > the condition is only true if (workspace != null || there were bugs) Mmmh, my idea was actually to drop this patch when going with the WindowTracker fix - that way we'd still fail when we end up with no windows for some other reason (but without crashing with the first patch applied), which would give us the chance to fix those bugs. That said, what do you think of this alternative (or a less harsh log(), though that'd make issues less likely to be reported)?
Review of attachment 266991 [details] [review]: Sounds a fine compromise to me. We still crash, but at least it's clear where it comes from.
(In reply to comment #11) > We still crash, but at least it's clear where it comes from. Better (IMHO): we fail to show the popup and log an error that hopefully points to the problem, but don't crash the wm.
(In reply to comment #12) > (In reply to comment #11) > > We still crash, but at least it's clear where it comes from. > > Better (IMHO): we fail to show the popup and log an error that hopefully points > to the problem, but don't crash the wm. Mh, I thought this would leave a grab around, but we don't have one until we show(), so awesome :)
Attachment 266567 [details] pushed as 427790f - switcherPopup: Fix spacing calculation for empty lists Attachment 266990 [details] pushed as cca1405 - window-tracker: Always enable transient_for redirection Attachment 266991 [details] pushed as 9d8f827 - altTab: Always filter out items with no windows