On Wed, Dec 18, 2013 at 3:58 PM, Owen Taylor <otay...@redhat.com> wrote: > I simply don't understand the point of doing your SwapBuffers in a thead > if you are using DRI2 and the X server is already doing the SwapBuffers > async in the background without blocking. Must have seemed like a good > idea to someone...
This is indeed one of the stranger things I have seen a driver do, though I guess this is besides the point. > People tend to exaggerate the danger of server grabs - the main problem > with them in an application is that if you are debugging with gdb in a X > terminal, a grab can make your session lock up - but that's always the > case for the compositor. Still, certainly there are opportunities for > deadlock with them. The danger is basically that you should never have a server grab whilst calling into code you don't control, because it could try to make a request on a separate X connection (this is something libraries should not do generally, though I've seen plenty of cases where they do it anyways). > > I don't think we need to be quite that broad. If the XReparentWindow() > failed, for example, we would be fine - it's trapped in frame.c. The > only things we have to worry about are a) assumptions that the code is > making that round-trip calls to the X server are succeeding b) one-way > and round-trip calls that don't have an error trap because they were > known to be always called during the window-creation phase c) window > going away before we select for DestroyNotify. > > With enough care, it might be possible to XUngrabServer() right after > selecting for DestroyNotify. Agreed. > > > 4. Ungrab server > > 5. Do anything which we don't depend on always succeeding because the window > > is going to receive a DestroyNotify later, this includes setting window > > properties and creating the window texture. > > > > Does this sound like a sensible strategy or are we missing anything? > > 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? 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. > > 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. > > 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. On a side note, this wouldn't be a problem if applications didn't have the insane behaviour of creating and destroying 20+ non-override redirect windows on startup (I know too many examples of this...). - Sam. > > - Owen > > > As a point of discussion, here's a patch to work around the issue. > > > > Make sure we call meta_workspace_add_window() with no X server grab held. > > (This function emits a window-added signal which does OpenGL operations) > > > > This is done by changing the semantics of meta_window_new_with_attrs() > > so that it is never called with a grab held. It takes its own grab > > for some operations, but the calls to > > meta_workspace_add_window() were then moved til after the point when the > > grab is released. > > > > > > Index: mutter-3.10.1/src/core/screen.c > > =================================================================== > > --- mutter-3.10.1.orig/src/core/screen.c 1999-12-31 20:15:39.695000032 > > -0600 > > +++ mutter-3.10.1/src/core/screen.c 1999-12-31 20:15:39.675000032 -0600 > > @@ -931,8 +931,6 @@ meta_screen_manage_all_windows (MetaScre > > GList *windows; > > GList *list; > > > > - meta_display_grab (screen->display); > > - > > if (screen->guard_window == None) > > screen->guard_window = create_guard_window (screen->display->xdisplay, > > screen); > > @@ -952,8 +950,6 @@ meta_screen_manage_all_windows (MetaScre > > > > g_list_foreach (windows, (GFunc)g_free, NULL); > > g_list_free (windows); > > - > > - meta_display_ungrab (screen->display); > > } > > > > /** > > Index: mutter-3.10.1/src/core/window.c > > =================================================================== > > --- mutter-3.10.1.orig/src/core/window.c 1999-12-31 20:15:39.695000032 > > -0600 > > +++ mutter-3.10.1/src/core/window.c 2013-12-17 14:08:38.700000160 -0600 > > @@ -664,46 +664,10 @@ meta_window_new (MetaDisplay *display, > > Window xwindow, > > gboolean must_be_viewable) > > { > > - XWindowAttributes attrs; > > - MetaWindow *window; > > - > > - meta_display_grab (display); > > - meta_error_trap_push (display); /* Push a trap over all of window > > - * creation, to reduce XSync() calls > > - */ > > - > > - meta_error_trap_push_with_return (display); > > - > > - if (XGetWindowAttributes (display->xdisplay,xwindow, &attrs)) > > - { > > - if(meta_error_trap_pop_with_return (display) != Success) > > - { > > - meta_verbose ("Failed to get attributes for window 0x%lx\n", > > - xwindow); > > - meta_error_trap_pop (display); > > - meta_display_ungrab (display); > > - return NULL; > > - } > > - window = meta_window_new_with_attrs (display, xwindow, > > - must_be_viewable, > > - META_COMP_EFFECT_CREATE, > > - &attrs); > > - } > > - else > > - { > > - meta_error_trap_pop_with_return (display); > > - meta_verbose ("Failed to get attributes for window 0x%lx\n", > > - xwindow); > > - meta_error_trap_pop (display); > > - meta_display_ungrab (display); > > - return NULL; > > - } > > - > > - > > - meta_error_trap_pop (display); > > - meta_display_ungrab (display); > > - > > - return window; > > + return meta_window_new_with_attrs (display, xwindow, > > + must_be_viewable, > > + META_COMP_EFFECT_CREATE, > > + NULL); > > } > > > > /* The MUTTER_WM_CLASS_FILTER environment variable is designed for > > @@ -822,6 +786,7 @@ meta_window_new_with_attrs (MetaDisplay > > MetaCompEffect effect, > > XWindowAttributes *attrs) > > { > > + XWindowAttributes _attrs; > > MetaWindow *window; > > GSList *tmp; > > MetaWorkspace *space; > > @@ -830,8 +795,6 @@ meta_window_new_with_attrs (MetaDisplay > > MetaMoveResizeFlags flags; > > MetaScreen *screen; > > > > - g_assert (attrs != NULL); > > - > > meta_verbose ("Attempting to manage 0x%lx\n", xwindow); > > > > if (meta_display_xwindow_is_a_no_focus_window (display, xwindow)) > > @@ -841,6 +804,39 @@ meta_window_new_with_attrs (MetaDisplay > > return NULL; > > } > > > > + /* Grab server */ > > + meta_display_grab (display); > > + meta_error_trap_push (display); /* Push a trap over all of window > > + * creation, to reduce XSync() calls > > + */ > > + > > + if (!attrs) > > + { > > + meta_error_trap_push_with_return (display); > > + > > + if (XGetWindowAttributes (display->xdisplay, xwindow, &_attrs)) > > + { > > + if(meta_error_trap_pop_with_return (display) != Success) > > + { > > + meta_verbose ("Failed to get attributes for window 0x%lx\n", > > + xwindow); > > + meta_error_trap_pop (display); > > + meta_display_ungrab (display); > > + return NULL; > > + } > > + } > > + else > > + { > > + meta_error_trap_pop_with_return (display); > > + meta_verbose ("Failed to get attributes for window 0x%lx\n", > > + xwindow); > > + meta_error_trap_pop (display); > > + meta_display_ungrab (display); > > + return NULL; > > + } > > + attrs = &_attrs; > > + } > > + > > screen = NULL; > > for (tmp = display->screens; tmp != NULL; tmp = tmp->next) > > { > > @@ -873,21 +869,19 @@ meta_window_new_with_attrs (MetaDisplay > > ) > > ) { > > meta_verbose ("Not managing our own windows\n"); > > + meta_error_trap_pop (display); > > + meta_display_ungrab (display); > > return NULL; > > } > > > > if (maybe_filter_window (display, xwindow, must_be_viewable, attrs)) > > { > > meta_verbose ("Not managing filtered window\n"); > > + meta_error_trap_pop (display); > > + meta_display_ungrab (display); > > return NULL; > > } > > > > - /* Grab server */ > > - meta_display_grab (display); > > - meta_error_trap_push (display); /* Push a trap over all of window > > - * creation, to reduce XSync() calls > > - */ > > - > > meta_verbose ("must_be_viewable = %d attrs->map_state = %d (%s)\n", > > must_be_viewable, > > attrs->map_state, > > @@ -1302,93 +1296,6 @@ meta_window_new_with_attrs (MetaDisplay > > > > window->on_all_workspaces = should_be_on_all_workspaces (window); > > > > - /* For the workspace, first honor hints, > > - * if that fails put transients with parents, > > - * otherwise put window on active space > > - */ > > - > > - if (window->initial_workspace_set) > > - { > > - if (window->initial_workspace == (int) 0xFFFFFFFF) > > - { > > - meta_topic (META_DEBUG_PLACEMENT, > > - "Window %s is initially on all spaces\n", > > - window->desc); > > - > > - /* need to set on_all_workspaces first so that it will be > > - * added to all the MRU lists > > - */ > > - window->on_all_workspaces_requested = TRUE; > > - window->on_all_workspaces = TRUE; > > - meta_workspace_add_window (window->screen->active_workspace, > > window); > > - } > > - else > > - { > > - meta_topic (META_DEBUG_PLACEMENT, > > - "Window %s is initially on space %d\n", > > - window->desc, window->initial_workspace); > > - > > - space = > > - meta_screen_get_workspace_by_index (window->screen, > > - window->initial_workspace); > > - > > - if (space) > > - meta_workspace_add_window (space, window); > > - } > > - } > > - > > - /* override-redirect windows are subtly different from other windows > > - * with window->on_all_workspaces == TRUE. Other windows are part of > > - * some workspace (so they can return to that if the flag is turned off), > > - * but appear on other workspaces. override-redirect windows are part > > - * of no workspace. > > - */ > > - if (!window->override_redirect) > > - { > > - if (window->workspace == NULL && > > - window->xtransient_for != None) > > - { > > - /* Try putting dialog on parent's workspace */ > > - MetaWindow *parent; > > - > > - parent = meta_display_lookup_x_window (window->display, > > - window->xtransient_for); > > - > > - if (parent && parent->workspace) > > - { > > - meta_topic (META_DEBUG_PLACEMENT, > > - "Putting window %s on same workspace as parent > > %s\n", > > - window->desc, parent->desc); > > - > > - if (parent->on_all_workspaces_requested) > > - { > > - window->on_all_workspaces_requested = TRUE; > > - window->on_all_workspaces = TRUE; > > - } > > - > > - /* this will implicitly add to the appropriate MRU lists > > - */ > > - meta_workspace_add_window (parent->workspace, window); > > - } > > - } > > - > > - if (window->workspace == NULL) > > - { > > - meta_topic (META_DEBUG_PLACEMENT, > > - "Putting window %s on active workspace\n", > > - window->desc); > > - > > - space = window->screen->active_workspace; > > - > > - meta_workspace_add_window (space, window); > > - } > > - > > - /* for the various on_all_workspaces = TRUE possible above */ > > - meta_window_set_current_workspace_hint (window); > > - > > - meta_window_update_struts (window); > > - } > > - > > g_signal_emit_by_name (window->screen, "window-entered-monitor", > > window->monitor->number, window); > > > > /* Must add window to stack before doing move/resize, since the > > @@ -1485,6 +1392,93 @@ meta_window_new_with_attrs (MetaDisplay > > meta_error_trap_pop (display); /* pop the XSync()-reducing trap */ > > meta_display_ungrab (display); > > > > + /* For the workspace, first honor hints, > > + * if that fails put transients with parents, > > + * otherwise put window on active space > > + */ > > + > > + if (window->initial_workspace_set) > > + { > > + if (window->initial_workspace == (int) 0xFFFFFFFF) > > + { > > + meta_topic (META_DEBUG_PLACEMENT, > > + "Window %s is initially on all spaces\n", > > + window->desc); > > + > > + /* need to set on_all_workspaces first so that it will be > > + * added to all the MRU lists > > + */ > > + window->on_all_workspaces_requested = TRUE; > > + window->on_all_workspaces = TRUE; > > + meta_workspace_add_window (window->screen->active_workspace, > > window); > > + } > > + else > > + { > > + meta_topic (META_DEBUG_PLACEMENT, > > + "Window %s is initially on space %d\n", > > + window->desc, window->initial_workspace); > > + > > + space = > > + meta_screen_get_workspace_by_index (window->screen, > > + window->initial_workspace); > > + > > + if (space) > > + meta_workspace_add_window (space, window); > > + } > > + } > > + > > + /* override-redirect windows are subtly different from other windows > > + * with window->on_all_workspaces == TRUE. Other windows are part of > > + * some workspace (so they can return to that if the flag is turned off), > > + * but appear on other workspaces. override-redirect windows are part > > + * of no workspace. > > + */ > > + if (!window->override_redirect) > > + { > > + if (window->workspace == NULL && > > + window->xtransient_for != None) > > + { > > + /* Try putting dialog on parent's workspace */ > > + MetaWindow *parent; > > + > > + parent = meta_display_lookup_x_window (window->display, > > + window->xtransient_for); > > + > > + if (parent && parent->workspace) > > + { > > + meta_topic (META_DEBUG_PLACEMENT, > > + "Putting window %s on same workspace as parent > > %s\n", > > + window->desc, parent->desc); > > + > > + if (parent->on_all_workspaces_requested) > > + { > > + window->on_all_workspaces_requested = TRUE; > > + window->on_all_workspaces = TRUE; > > + } > > + > > + /* this will implicitly add to the appropriate MRU lists > > + */ > > + meta_workspace_add_window (parent->workspace, window); > > + } > > + } > > + > > + if (window->workspace == NULL) > > + { > > + meta_topic (META_DEBUG_PLACEMENT, > > + "Putting window %s on active workspace\n", > > + window->desc); > > + > > + space = window->screen->active_workspace; > > + > > + meta_workspace_add_window (space, window); > > + } > > + > > + /* for the various on_all_workspaces = TRUE possible above */ > > + meta_window_set_current_workspace_hint (window); > > + > > + meta_window_update_struts (window); > > + } > > + > > window->constructing = FALSE; > > > > meta_display_notify_window_created (display, window); > > _______________________________________________ > > gnome-shell-list mailing list > > gnome-shell-list@gnome.org > > https://mail.gnome.org/mailman/listinfo/gnome-shell-list > > _______________________________________________ gnome-shell-list mailing list gnome-shell-list@gnome.org https://mail.gnome.org/mailman/listinfo/gnome-shell-list
