GNOME Bugzilla – Bug 122016
Alt+Tab sometimes has extra windows
Last modified: 2004-12-22 21:47:04 UTC
Last night I had one gaim and one epiphany (or maybe evolution) window open in a workspace. The alt-tab window listed two gaims and an epiphany. Activating either gaim from the alt-tab list activated the gaim window. Today I had an epiphany, an evolution, a terminal, and an emacs open. Alt-tab listed an extra evolution. When I moved the evolution to a new workspace, the old workspace still listed one evolution.
That kicks ass. I'm guessing it's related to the new mru list changes, maybe we get stale mru list entries.
*** Bug 122499 has been marked as a duplicate of this bug. ***
Figured out why this happens. The person who wrote the per-workspace MRU code made a very common mistake and didn't save back the result of g_list_*. Weirdness will result any time sticky state is toggled for a window. Patch attached.
Actually forgetting to save it back was only part of the problem; really it was something more subtle. Somehow the window is being added to a workspace when the window already appears in the MRU list; I can't figure out exactly why this is happening. I can fix the bug by adding a check in add_window, however.
Created attachment 20030 [details] [review] fix
We need to find out the root cause, rather than just the add_window check. The failure to assign back the GList* is a major "doh", need to be sure that's in the next stable release.
*** Bug 120809 has been marked as a duplicate of this bug. ***
blame the first 2 dup enties here on mid-air collisions ;-)
committed the assign back to g_list part to fix the crash; still can't track down the duplicate entry bug. Probably something obvious. It might help if someone else takes a look at it though, but I just can't see it for some reason.
OK I figured it out; it was so obvious it was staring me in the face the whole time. Trivial patch already committed to CVS.
Created attachment 20143 [details] [review] patch committed
Reopening, appears to still happen in vers metacity-2.6.2-1. Cannot really see the common factor. I have 8 workspaces (2 rows of 4). Workspaces with an '*' below exhibit the behavior where alt-tab is cycling through windows that are not on the current workspace. 1 2* 3* 4* 5 6 7* 8 The workspaces below have windows in there workspace: 1* 2 3 4* 5* 6* 7* 8* For the workspaces where alt-tab is cycling through windows on other workspaces, neither the gnome window list or the gnome window menu show these extra windows. They appear to be behaving normally. Is there any other information that may be helpful, that I have not included? Can someone please open this bug, I do not have sufficient bugzilla authority to change bug_status.
Are you using the "Put on all workspaces feature" to cause this problem? Do you have any steps to reproduce the bug? This may be related to #123511.
"Put on all workspaces feature" is not enabled. I will restart metacity (killall -9 metacity ... gnome restarts it), and it appears that the alt-tab window list behavior is normal. I was just about to close the bug...when it started happening again. Perhaps this is related to metacity running after a period of time? I could not reproduce this immediately after starting metacity. However, a few hours later the bug is still there. Steps to reproduce: 1) start metacity 2) open typical set of apps over 8 workspaces 3) wait some time () 4) alt-tab incorrectly includes windows on other workspaces = not all workspaces are affected Is it possible that I have stale data floating around $HOME (perhaps gconf keys or .metacity/sessions/)? I removed .metacity and the .gconf/apps/metacity...but the problem still occurs. I haven't yet been able to reproduce with newly created local users.
the wait some time part here is the crucial bit. What are you doing during this time? Moving windows to different workspaces? Switching back and forth between workspaces? Something else? Obviously if you just leave the computer on and get a cup of coffee metacity isn't going to do anything.
>> the wait some time part here is the crucial bit. Yes I agree...I had been trying to reproduce with the new local user doing the following, but haven't had luck: - 8 workspaces on 2 rows - not all workspaces have windows - some workspaces have multiple windows - add new windows (~ about 8 open while this is happening) - add/remove windows ( I do this frequently for terminals windows) - xscreensaver kicks in at some point (passwd required) - switch workspaces (w/ keyboard shortcuts <alt><shift> arrow) - move windows between workspaces (w/ keyboard shortcuts <alt><ctrl> arrows) I have even reinstalled my machine (unrelated issue, retaining only my maildir and $HOME/.phoenix) and it still happens.
Still happening...VERY frustrating...anything that I can do to assist or provide more info?
if you could give me a sequence of things to do that causes the problem, that would be fantastic. But just to be precise: does this happen with metacity HEAD? Or latest gnome-2-4 branch?
*** Bug 126457 has been marked as a duplicate of this bug. ***
Okay, I've managed to reproduce this on a fresh user acct on my system. However, you will not be pleased with the recreate since there is no step by step way to do this. I'm attaching a screenshot to demonstate my setup and the bug. - create new user - startx (default fedora gnome-session) - change the viewport to 2x4 - tinker with the panel (adjust height to 28, removed icons) - open at least 1 window in every viewport - start cycling through the viewports (using keyboard shortcuts) + cycle window list in each viewport (using keyboard shortcuts) - keep adding more windows to some viewports Then, I notice it starts happening. evolution appears in the alt-tab windowlist even though it is on another viewport. The same then happens for xchat (which is on a different viewport, and not the same one evolution is on). While the window list (alt-tab) is showing extra windows that shouldn't be there, the Window Menu panel applet has the correct information (showing windows inthe current viewport above the line, and all others below). Perhaps metacity's mapping of windows to viewport is out of sync with what "Window Menu" is using? I've been able to reproduce with the smaller scenario: - startx as a new user (default gnome fedora 2x2 desktop) - open terminal window - run `zenity --question` 20+ times - move some of those windows to another viewport + 1 on one viewport + 3 on another + the rest on another - now go crazy with the viewport switching (keyboard shortcut) - window list is hosed * will attach xshot next
Created attachment 21277 [details] screenshot demonstating recreate #1
Created attachment 21278 [details] screenshot demonstating recreate #2
OK I committed a change to HEAD that might help. Try HEAD and tell me if you can still reproduce: 2003-11-16 Rob Adams <readams@readams.net> * src/window.c (meta_window_notify_focus): add paranoia check to make sure a window is really on a workspace before inserting it at the beginning of the MRU list. Maybe there's a race condition with focusing and workspace switching. Hopefully a fix for #122016.
What I'd like to see instead of paranoia checks would be assertions or meta_warning() scattered around every time we modify the mru_list, to verify the list's correctness. If the list is broken in a way that would produce this bug, print a warning with the code location that messed up the list. It should be straightforward to track down the root problem at that point with confidence. Probably the assertions/warnings should only be in a local tree, not in cvs, since they'll be slow and make sort of a mess.
The patch seems to have fixed it...after running with the fix since Nov 20 I have not observed the problem. So not sure how you would like it fixed (w/ asserts as hp discussed) or with the current patch. Let me know if you require additional testing.
*** Bug 128467 has been marked as a duplicate of this bug. ***
so if that fixed it then the problem must be a race condition between focusing and switching workspaces somehow. I'll look into it more when I get a chance, and try to come up with the real fix (the other one was always intended to be a temporary experiment)
Why do we remove the window from the mru list if the workspace doesn't contain the window? Would removing this "!" fix the bug or at least some bug? /* Remove window from MRU lists that it doesn't belong in */ tmp = window->screen->workspaces; while (tmp) { workspace = (MetaWorkspace *) tmp->data; if (!meta_workspace_contains_window (workspace, window)) workspace->mru_list = g_list_remove (workspace->mru_list, window); tmp = tmp->next; }
Nevermind, I'm on crack.
New theory: if you have a sticky window, it's in the mru list for all workspaces. meta_window_free() does not remove it from said mru list. Thus, you eventually crash metacity if you close an "on all workspaces" window, such as the panel or one you've set on all workspaces explicitly. Don't know if this is _the_ crash, but it looks like a crash.
Created attachment 22398 [details] [review] fix attempt
I think that the real problem (at least for what your patch fixes) is that in workspace_remove_window we simply remove the window from that workspace's mru_list. Shouldn't we 1) only remove it from the mru list if it's not a sticky window, and 2) remove it from all mru lists if it's a sticky window no longer on any workspace. That would naturally fix the problem that change window_free does and making it more robust in general. Of course, there's some way that the extra windows are showing up that has nothing to do with stickiness...
Yes, that sounds like a good fix. I realized there's probably an additional problem: if you add a window to a workspace while the window is sticky, then the window will be in the mru list twice, and only get removed once. At least I bet it will be.
Created attachment 22411 [details] [review] Various sticky MRU fixes, plus random user_time support
attached patch (committed) implements the fix I described earlier. It also includes some random preliminary USER_TIME atom support that won't hurt anything and I didn't want to try to disentangle. Note that this also fixes Bug 120907 since I also changes new_with_attrs so that it sets on_all_workspaces before adding to the workspaces, which ensures that initially sticky windows get added to the MRU lists.
Please, don't just commit large patches without waiting for comments, especially when I'm clearly paying attention. ;-) The mru fixes need to be committed to the stable branch, but not the USER_TIME changes. Let's revert the patch, work on two distinct patches, then commit the mru fix to both branches and USER_TIME on HEAD once we have all the details finalized. Otherwise it's too easy to screw up and get different changes on each branch. It would be better to discuss the USER_TIME patch on the appropriate bug rather than here. Braces in the wrong place here: if (window && !frame_was_receiver) { meta_window_property_notify (window, event); + } Why does this occur twice? + hooks[i].property = display->atom_net_wm_user_time; + hooks[i].init_func = init_net_wm_user_time; + hooks[i].reload_func = reload_net_wm_user_time; + ++i; + + hooks[i].property = display->atom_net_wm_user_time; + hooks[i].init_func = init_net_wm_user_time; + hooks[i].reload_func = reload_net_wm_user_time; + ++i; + I don't understand this: + /* need to set on_all_workspaces first so that it will be + * added to all the MRU lists + */ window->on_all_workspaces = TRUE; Another double-code segment? + else if (event->atom == window->display->atom_net_wm_user_time) + { + meta_verbose ("Property notify on %s for _NET_WM_USER_TIME\n", window->desc); + + meta_window_reload_property (window, + window->display->atom_net_wm_user_time); + } + else if (event->atom == window->display->atom_net_wm_user_time) + { + meta_verbose ("Property notify on %s for _NET_WM_USER_TIME\n", window->desc); + + meta_window_reload_property (window, + window->display->atom_net_wm_user_time); + } This is O(n), you should check for zero-length list as "window->workspaces == NULL" instead: + if (g_list_length (window->workspaces) == 0) Again later in the function: + if (g_list_length (window->workspaces) == 0) Should not assign and declare in the same line: + GList* tmp = window->screen->workspaces; Indentation is wrong, 2 spaces: + else + workspace->mru_list = g_list_remove (workspace->mru_list, window); Going back up in the function, when would the window be in the mru list for the workspace when not on the workspace and not sticky: + else if (!g_list_find (workspace->mru_list, window)) + workspace->mru_list = g_list_prepend (workspace->mru_list, window); If we can't say when the g_list_find() returns non-NULL, then we should not do the O(n) search here, or only do it as: else { g_assert (g_list_find (workspace->mru_list, window) == NULL); workspace->mru_list = g_list_prepend (... }
somehow that patch got applied twice. That's what caused all those weird things to happen. The problem is that I forgot I had that other stuff in my tree until after I finished with this, and I didn't really want to separate the patches. The mru list stuff was so simple that I figured I'd shoot first and ask questions later, but the whole thing ended up now working out so well. I've cleaned up the mess, and separated out the user time stuff, which I think I'll sit on for now since I've got it split out now. Attaching the MRU list only patch to the bug. The reason that we have to set on_all_workspaces before adding it to the workspace is that the code in workspace_add_window will take care of the mru lists for workspaces on all windows. This would be a problem any time a window is on all workspaces when it is mapped, and it's the reason for example the panels and dock windows weren't showing up in the alt-tab list.
Created attachment 22417 [details] [review] mru patch
actually that patch is almost right. There also needs to be line in there about removing the existing workspace->mru_list = g_list_append (workspace->mru_list, window); from add_window. Man, what a mess.
Created attachment 22436 [details] [review] added some assertions and minor cleanups
Let me know if this latest iteration looks good.
Yea, that looks good to me. Did you want me to commit it?
I checked it in both branches since I had it all ready to go. Thanks for the fix. Let's see if these crashes go away ;-)
Created attachment 22449 [details] [review] minor fix
minor fix here. Since now sticky windows won't be cleared out of a MRU list when they're removed, this assertion will typically fail when reducing the number of workspaces. Very simple fix, though, which ought to be committed to both branches.
minor fix committed to HEAD and branch.
*** Bug 129913 has been marked as a duplicate of this bug. ***
*** Bug 132880 has been marked as a duplicate of this bug. ***
*** Bug 133652 has been marked as a duplicate of this bug. ***
*** Bug 133934 has been marked as a duplicate of this bug. ***
Trying to Reopen this bug (but I don't have permissions) because I am starting to hit the assertions added by this patch (possible bug in the fix). Running with: metacity-2.6.5-1 when I bring up the <alt>Tab window list, I'm triggering an assert (that I believe was added in the patches below) and then metacity dies. Window manager warning: Log level 6: file workspace.c: line 135 (meta_workspace_add_window): assertion failed: (g_list_find (workspace->mru_list, window) == NULL) Let me know what additional information I can supply to help debuging (strace etc...)
Perhaps bug 131196 should be marked as a duplicate of this one (or vice versa)? jlaska's description matches stefan's problem...
Elijah, this should probably be on the GNOME 2.6 blocker list.
Created attachment 24882 [details] [review] Possible fix
I think that this is being caused by FocusIn events being received for windows on workspaces that we've switched away from. This is a race condition, leading to intermittent, difficult to reproduce crashes. The patch has been committed to HEAD. Please somebody tell me this is fixed.
*** Bug 131196 has been marked as a duplicate of this bug. ***
*** Bug 134919 has been marked as a duplicate of this bug. ***
That patch looks reasonable to me, though who knows if it's really the bug. Have people confirmed they were still seeing the bug on HEAD, or were the continuing reports in 2.6.5 only?
There was at least one person who saw it in 2.7.0. I added the assert and he then came back with the assertion failing there. There are only a few places now where we add or remove windows from the MRU lists, and that was the only one that was even remotely possible that could be causing the bug. We'll have to see.
I was seeing it in fairly recent head (last week?) though I can't seem to duplicate it now. [never could dup it on command, though.]
A possible course of action is to write this: void validate_mru_lists (void) { /* ... check that all windows in mru list are in display's list of windows... */ /* ... check that window->workspaces and workspace->windows agree on which windows are where ... */ /* ... check whatever else we can think of ... */ } Fail an assertion if anything is wrong... and call validate_mru_lists() after *every* codepath that touches the list... Obviously you'd only include this in betas, not final code.
Running HEAD 12:00 CET since yesterday, no crash or wrong window in Alt+Tab so far. I guess if I do not have a acrash by tomorrow evening the thing should be fixed.
Running with metacity-2.7.0 I am seeing the assert failure: Window manager warning: Log level 6: file workspace.c: line 135 (meta_workspace_add_window): assertion failed: (g_list_find (workspace->mru_list, window) == NULL)
We think that the bug may be fixed in HEAD as of 2/27; was your metacity compiled more recently than that, jlaska?
> We think that the bug may be fixed in HEAD as of 2/27; was your > metacity compiled more recently than that, jlaska? oops, appears not... http://ftp.gnome.org/pub/GNOME/sources/metacity/2.7/ LATEST-IS-2.7.0 15-Feb-2004 23:49 3.0M metacity-2.7.0.tar.bz2 15-Feb-2004 23:49 1.8M
I'm seeing this with metacity-2.7.0-1 from rawhide. I also managed to reproduce it twice in a row on the first try: 0. I have a 4x2 layout 1. have some windows open. 2. have a real window (not the desktop) active (focused) 3. hold down ctrl and alt 4. hack around madly on the cursor keys for something like 5 secs you should achieve at least 5 hits/sec :) now this list should be hosed. In particular the above mentioned window appears in the list on a workspace different from the one the window really is on. Observations: - This happens independently from whether a window was set to be on all workspaces or not. - This also happens with alt+f1-f8 (which switch workspaces for me) - When I switch to such a ghost window metacity crashes: Предупреждение менеджера окон: Log level 6: file workspace.c: line 135 (meta_workspace_add_window): assertion failed: (g_list_find (workspace->mru_list, window) == NULL) 0x005d0a90 in g_logv () from /usr/lib/libglib-2.0.so.0 (gdb) bt
+ Trace 44750
... I hope this helps.
2.7.0 is before the fix. Please ONLY post here if you see the problem in metacity HEAD updated and compiled _after_ the evening of Friday, 2/27/04. Thanks.
Rob, can I suggest maybe rolling a tarball now and announcing that, and calling specifically for that version instead of a CVS date?
Rob, did you try my reproduction procedure?
FWIW: I checked out metacity from HEAD yesterday (mar 02) afternoon. I have not been able to reproduce the easy-to-hit asserts that I had been hitting previously. (2x8 workspace -- windows on all, lots of keyboard alt-tab + switching workspaces). Will post if the problem occurs again.
With CVS HEAD as of 2004-02-26 (i.e. BEFORE Rob's fix) and using Markus's duplication steps (although simplified a little bit--just a 1x4 workspace setup with one window on each workspace), I can reliably get Metacity to crash with the log message "Window manager warning: Log level 6: file window.c: line 4138 (meta_window_notify_focus): assertion failed: (link)" [Note that this is a crash in a slightly different place than mentioned in this bug report, but it's due to the assert Rob added for bug 131196 (which is a dup) in order to check his theory about why this was crashing]. With CVS HEAD as of today I cannot get this crash to occur. I vote we mark it fixed... :)
OK, declaring fixed.