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 613124 - Invalid visibility-related asserts in MutterWindow
Invalid visibility-related asserts in MutterWindow
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2010-03-17 09:59 UTC by Tomas Frydrych
Modified: 2011-01-04 16:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to remove asserts (1.72 KB, patch)
2010-03-17 09:59 UTC, Tomas Frydrych
rejected Details | Review
If enabling a compositor with existing windows, set them visible as necessary (1.39 KB, patch)
2010-08-17 19:09 UTC, Owen Taylor
committed Details | Review
Update window->visible_to_compositor before calling into compositor (1.79 KB, patch)
2011-01-04 16:48 UTC, Owen Taylor
committed Details | Review

Description Tomas Frydrych 2010-03-17 09:59:37 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.
Comment 1 Owen Taylor 2010-04-12 13:26:14 UTC
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?
Comment 2 Owen Taylor 2010-08-16 17:59:50 UTC
Ping?
Comment 3 Tomas Frydrych 2010-08-17 07:23:19 UTC
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.
Comment 4 Owen Taylor 2010-08-17 19:09:00 UTC
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?
Comment 5 Tomas Frydrych 2010-08-23 09:23:44 UTC
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).
Comment 6 Owen Taylor 2010-08-23 17:48:31 UTC
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.
Comment 7 Owen Taylor 2011-01-04 16:35:59 UTC
Review of attachment 156339 [details] [review]:

Not appropriate from discussions
Comment 8 Owen Taylor 2011-01-04 16:41:13 UTC
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
Comment 9 Owen Taylor 2011-01-04 16:47:35 UTC
Implemented moving the flag updating before calling into the compositor and pushed
Comment 10 Owen Taylor 2011-01-04 16:48:13 UTC
The following fix has been pushed:
0a821d2 Update window->visible_to_compositor before calling into compositor
Comment 11 Owen Taylor 2011-01-04 16:48:16 UTC
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.