Re: mutter: reduce server grabs for window creation

[Owen Taylor <otaylor redhat com>]

On Wed, 2013-12-18 at 17:03 -0200, Sam Spilsbury wrote:

> > One thing that definitely should be noted is that we still have a server
> > grab over everything when this code is called from
> > meta_screen_manage_all_windows(). From a brief look, it appears to me
> > that this happens *after* we initialize Javascript, set up signal
> > connections, etc - so probably the only reason that your patch works for
> > you is that the driver's internal swap-buffers thread isn't doing
> > anything during initialisation.
>
> Is there any need for having a server grab with a large scope here in that case?

The origin of the grab is lost in the deeps of time - the grab around
creating all windows was added in one of the first commits to Metacity
with the comment

 /* Must grab server to avoid obvious race condition */

But I'm not sure what the "obvious" race condition is or was - maybe
simply trying to avoid having to code for vanished windows
in meta_window_new(). The grab never included the selection for
SubstructureRedirect/SubstructureNotify on the root window - that is
simply done first and stray DestroyNotify, etc, events would simply
be ignored.

The reason that XGetWindowAttributes() is separated out and passed to
meta_window_with_attrs() is that the compositor code used to handle
X windows that didn't have a corresponding MetaWindow (see commit
d538690b) - so meta_window_new_with_attrs() could be eliminated at this
point.

> I haven't had a chance to look too closely at the code, but perhaps we
> are assuming that the results of an XQueryTree will stay constant
> during initialisation. In that case, the only thing you care about is
> windows being either created or destroyed before you've selected for
> CreateNotify, ReparentNotify and DestroyNotify on the root window. So
> really, the only thing that is necessary is to have the server grab
> during XQueryTree and XSelectInput. Then you need to ostensibly
> "manage" all the windows and add them to the internal stack. After
> that, you can assume that any created or destroyed windows are handled
> as creations or destructions that occur during the event loop and not
> initialisation.

As far as I can tell, the code would simply be fine without initial
large-scope server grab.

> > This makes me less enthusiastic about the approach - we aren't avoiding
> > calling application code with a server grab, we're just not doing it in
> > the case that was causing a problem.
> >
> > Other than that, I don't have large-scale objections to the patch other
> > than the inherent bug-introducing potential of moving things around in
> > meta_window_new_with_attrs(). I would certainly wonder if we could
> > simply move the ungrab up earlier in the function and fix up anything
> > in class a) and b) described above.
>
> If I recall correctly (now that I've refreshed my memory on this), you
> only need grabs for:
>
> 1. Querying the tree and selecting for events
> 2. Doing the XGetWindowAttributes, XCreateWindow, XReparentWindow,
> XAddToSaveSet dance.
>
> One caveat with #2 is that if XGetWindowAttributes fails it means that
> the window was destroyed - but you still need to keep it around in the
> stack until you get a DestroyNotify for it because its possible that
> there were already requests made relative to it.

If Mutter gets a ConfigureRequest (etc.) for a window it doesn't
recognize, it will just ignore it, so it should be fine to drop windows
that have vanished on the floor. The only flaw I can see with this - the
current approach - is that it isn't completely robust against XID reuse.
You could fix that on startup with a grab across /everything/, but
mostly the problems would still be there later in operation (and are
inevitably there for any WM.)

> > Have you created a torture test for your app - something that repeatedly
> > creates a new window and after a short random delay destroys it?
>
> I have one I used for debugging the (notorious) stacking bugs in
> compiz here: https://code.launchpad.net/~smspillaz/+junk/stackingtest
>
> It creates 50 windows at once every second.

That sounds like something that could be useful. 

(Hmm, it's probably tricky with X server scheduling, kernel scheduling,
etc, to really test all the possibilities for when a window could be
destroyed with respect to window manager operation reliably.)

- Owen