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 _______________________________________________ gnome-shell-list mailing list gnome-shell-list@gnome.org https://mail.gnome.org/mailman/listinfo/gnome-shell-list
