GNOME Bugzilla – Bug 613124
Invalid visibility-related asserts in MutterWindow
Last modified: 2011-01-04 16:48:16 UTC
Created attachment 156339 [details] [review] Patch to remove asserts When a window is moved from one workspace onto another, its visibility status can change while it is enqueued for visibility calculation. For example, window can be initially visible, then enqued to be hidden due to change of its workspace, and then enqueued again (before the original queue request has been processed) to be shown due to the change of active workspace. The net effect of this is that the mutter_window_show() and mutter_window_hide() can be called on the window that is already in the correct state. I can't think of a way to fix this without introducing further complexity to the queue (i.e., so that two subsequent queue requests can cancel out). This patch changes the two functions to simply return immediately if this scenario happens.
I'm not understanding this - calls to meta_compositor_hide_window/show_window are guarded by the 'window->visible_to_compositor' flag. It's hard to see how you would get two consecutive calls to hide_window() or show_window() unless there was some strange reentrancy going on. Do you have a specific way of reproducing this problem?
Ping?
IIRC it happens when the window is initially visible to the compositor, then is enqueued for hiding, and then, before the idle responsible for processing the queue is called, the window is again enqueued again to be visible. Since there is only one entry in the queue for each window, when the idle finally triggers, you will get meta_window_show() on window that is already (or rather still) visible. I don't have a specific test case, but the MeeGo system log used to be be flooded with these asserts.
Created attachment 168122 [details] [review] If enabling a compositor with existing windows, set them visible as necessary Here's my best guess - maybe somehow with mutter-moblin windows can get hidden or shown before the compositor is created, so MetaWindow.visible_to_compositor gets out of sync with MutterWindow.visible. Other than this, I can't see any way the situation is possible; the queue handling shouldn't matter - meta_window_show()/hide() does not gate the calls to meta_compositor_hide/show_window() - that's done based on MetaWindow.visible_to_compositor. (I might be missing some other way that MutterWindow.visible or MetaWindow.visible_to_compositor change, but I can't find any such cases.) Is it possible to test this patch?
The second patch does not fix this issue, but I have finally worked out what is going on, goes like this: 1. When a new Meego window maps, it gets automatically moved onto a new workspace; this happens inside the meta_compositor_show_window() call in meta_window_show(). At this point the visible_to_compositor flag is still FALSE, as it only gets set after meta_compositor_show_window() returns. 2. The workspace switch involves call to meta_window_focus() which calls meta_window_flush_calc_showing() which will call meta_window_show_again(). So, a simple way to avoid this is to set the visible_to_compositor flag before the meta_compositor_show_window(). As this flag is only used by meta_window_show/hide/unamange() this should not have any undesirable side effects (which my limited testing seems to confirm).
No problem so strange that it can't be triggered by a bit of unexpected reentrancy ... :-) I think it would be better to do your "move newly mapped windows to a new workspace" from MetaDisplay::window-created than from inside a compositor hook that is supposed to be about *drawing* the windows. However, I have no objection to moving changing the flag to before calling meta_compositor_show_window and meta_compositor_hide_window for better reentrancy protection. Sounds like a reasonable change.
Review of attachment 156339 [details] [review]: Not appropriate from discussions
Comment on attachment 168122 [details] [review] If enabling a compositor with existing windows, set them visible as necessary Pushed my patch, it's correct if irrelevant. Attachment 168122 [details] pushed as 78092a4 - If enabling a compositor with existing windows, set them visible as necessary
Implemented moving the flag updating before calling into the compositor and pushed
The following fix has been pushed: 0a821d2 Update window->visible_to_compositor before calling into compositor
Created attachment 177496 [details] [review] Update window->visible_to_compositor before calling into compositor To deal with reentrancy from compositor plugins doing things like moving windows between workspaces in an effect callback, update the visible_to_compositor flag before calling into the compositor.