Compiz

Merge lp:~compiz-team/compiz/compiz.fix_1088399 into lp:compiz/0.9.9

  1. compiz.fix_1088399
  2. Merge into 0.9.9
Proposed by Sam Spilsbury
Status: Merged
Approved by: Brandon Schaefer
Approved revision: 3526
Merged at revision: 3606
Proposed branch: lp:~compiz-team/compiz/compiz.fix_1088399
Merge into: lp:compiz/0.9.9
Diff against target: 1016 lines (+412/-127)
9 files modified
include/core/window.h (+6/-1)
src/event.cpp (+33/-4)
src/option/tests/CMakeLists.txt (+1/-0)
src/option/tests/option.cpp (+4/-0)
src/outputdevices.h (+5/-0)
src/privatewindow.h (+38/-29)
src/tests/CMakeLists.txt (+1/-0)
src/window.cpp (+244/-93)
tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp (+80/-0)
To merge this branch: bzr merge lp:~compiz-team/compiz/compiz.fix_1088399
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Marco Trevisan (Treviño) Pending
Brandon Schaefer Pending
Review via email: mp+147838@code.launchpad.net

This proposal supersedes a proposal from 2012-12-10.

Commit message

Ensure that we never get a BadWindow due to the sibling in a ConfigureWindow.

Issuing a ConfigureWindow request with a sibling parameter that refers
to a sibling that's actually destroyed server side results in a BadWindow
error, and causes what we might think is a correct ConfigureWindow request
to fail. This would cause even more problems because we had marked
the request as succeeding, and adjusted serverWindows appropriately,
which would cause further restacks to rely on the invalid internal state.

Unfortunately, this is effectively a race condition between the client and
the server. We receive our events and are racing the server to not
destroy the window internally before we get a chance to issue a
ConfigureWindow request relative to the destroyed window.

Because of that, we need to hold a server grab during the period in which
we check if the sibling window is still valid server side and when
we issue a ConfigureWindow request. If it is valid, then we can guaruntee
that the server will process the request relative to the sibling window,
and if not, we can select another sibling as appropriate.

Adjusted the API such that any function which looks for an appropriate sibling
or any function which calls XConfigureWindow with the sibling parameter
set requires a server grab to be active (by virtue of the fact that they
accept a const ServerLock &).

The only implicit contract between clients now is that the client must
obtain the sibling window either by using one of the designated functions
to do so which requires a ServerLock, or that the client holds a server lock
and while it holds the ServerLock, it calls XGetWindowAttributes or
XGetGeometry to check if the sibling window is valid.

configureXWindow was split into configureXWindow and
restackAndConfigureXWindow. The latter requires a server lock, the former
will warn about this potential race condition if you attempt to
restack a window using it.

Passing tests:

[ RUN ] CompizXorgSystemStackingTest.TestCreateRelativeToDestroyedWindowFindsAnotherAppropriatePosition
[ OK ] CompizXorgSystemStackingTest.TestCreateRelativeToDestroyedWindowFindsAnotherAppropriatePosition (55 ms)

(LP: #1088399)

Description of the change

Ensure that we never get a BadWindow due to the sibling in a ConfigureWindow.

Issuing a ConfigureWindow request with a sibling parameter that refers
to a sibling that's actually destroyed server side results in a BadWindow
error, and causes what we might think is a correct ConfigureWindow request
to fail. This would cause even more problems because we had marked
the request as succeeding, and adjusted serverWindows appropriately,
which would cause further restacks to rely on the invalid internal state.

Unfortunately, this is effectively a race condition between the client and
the server. We receive our events and are racing the server to not
destroy the window internally before we get a chance to issue a
ConfigureWindow request relative to the destroyed window.

Because of that, we need to hold a server grab during the period in which
we check if the sibling window is still valid server side and when
we issue a ConfigureWindow request. If it is valid, then we can guaruntee
that the server will process the request relative to the sibling window,
and if not, we can select another sibling as appropriate.

Adjusted the API such that any function which looks for an appropriate sibling
or any function which calls XConfigureWindow with the sibling parameter
set requires a server grab to be active (by virtue of the fact that they
accept a const ServerLock &).

The only implicit contract between clients now is that the client must
obtain the sibling window either by using one of the designated functions
to do so which requires a ServerLock, or that the client holds a server lock
and while it holds the ServerLock, it calls XGetWindowAttributes or
XGetGeometry to check if the sibling window is valid.

configureXWindow was split into configureXWindow and
restackAndConfigureXWindow. The latter requires a server lock, the former
will warn about this potential race condition if you attempt to
restack a window using it.

Passing tests:

[ RUN ] CompizXorgSystemStackingTest.TestCreateRelativeToDestroyedWindowFindsAnotherAppropriatePosition
[ OK ] CompizXorgSystemStackingTest.TestCreateRelativeToDestroyedWindowFindsAnotherAppropriatePosition (55 ms)

(LP: #1088399)

To post a comment you must log in.
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

LGTM

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:3524
http://jenkins.qa.ubuntu.com/job/compiz-ci/3/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-clang-ci/./build=pbuilder,distribution=quantal,flavor=amd64/41/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=quantal,flavor=amd64/40/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=quantal,flavor=amd64/392/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/3//rebuild/?

review: Approve (continuous-integration)
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote : Posted in a previous version of this proposal

Looks fine to me.

review: Approve
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

This was backed out due to the tornado in https://lists.launchpad.net/unity-dev/msg00597.html .

We will need to re-merge it.

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

I'm marking this for merging again.

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

286 + * the same ServerLock aliver when calling through to

should probably be (just a typo):

286 + * the same ServerLock alive when calling through to

Maybe those:

+ if (stackLayerCheck (w, clientLeader, below, lock))
+ if (existsOnServer (below, lock))
+ return below;

could be changed to:

+ if (stackLayerCheck (w, clientLeader, below, lock) && existsOnServer (below, lock))
+ return below;

There are a few of such if-if constructions, which could be merged into one if statement.

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

On Sun, Feb 10, 2013 at 7:56 PM, MC Return <email address hidden> wrote:
> 286 + * the same ServerLock aliver when calling through to
>
> should probably be (just a typo):
>
> 286 + * the same ServerLock alive when calling through to
>
>
> Maybe those:
>
> + if (stackLayerCheck (w, clientLeader, below, lock))
> + if (existsOnServer (below, lock))
> + return below;
>
> could be changed to:
>
> + if (stackLayerCheck (w, clientLeader, below, lock) && existsOnServer (below, lock))
> + return below;
>
> There are a few of such if-if constructions, which could be merged into one if statement.

Hmm, maybe.

>
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1088399/+merge/138956
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:3525
http://jenkins.qa.ubuntu.com/job/compiz-ci/10/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-clang-ci/./build=pbuilder,distribution=quantal,flavor=amd64/48/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=quantal,flavor=amd64/47/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=quantal,flavor=amd64/399/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/10//rebuild/?

review: Needs Fixing (continuous-integration)
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote : Posted in a previous version of this proposal

Confirm this also fixes:
https://bugs.launchpad.net/unity/+bug/906231

Also still looks good to me :).

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:3526
http://jenkins.qa.ubuntu.com/job/compiz-ci/15/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-clang-ci/./build=pbuilder,distribution=quantal,flavor=amd64/53/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=quantal,flavor=amd64/52/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=quantal,flavor=amd64/404/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/15//rebuild/?

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:3526
http://jenkins.qa.ubuntu.com/job/compiz-ci/21/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-clang-ci/./build=pbuilder,distribution=quantal,flavor=amd64/59/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=quantal,flavor=amd64/58/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=quantal,flavor=amd64/410/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/21//rebuild/?

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/core/window.h'
2--- include/core/window.h 2012-12-13 10:12:23 +0000
3+++ include/core/window.h 2013-02-12 05:13:48 +0000
4@@ -43,6 +43,7 @@
5 #include <core/region.h>
6 #include <core/windowgeometry.h>
7 #include <core/windowextents.h>
8+#include <core/servergrab.h>
9
10 #include <core/wrapsystem.h>
11
12@@ -433,9 +434,13 @@
13 void moveInputFocusToOtherWindow ();
14
15 /* wraps XConfigureWindow and updates serverGeometry */
16- void configureXWindow (unsigned int valueMask,
17+ void configureXWindow (unsigned int valueMask,
18 XWindowChanges *xwc);
19
20+ void restackAndConfigureXWindow (unsigned int valueMask,
21+ XWindowChanges *xwc,
22+ const ServerLock &lock);
23+
24 void moveResize (XWindowChanges *xwc,
25 unsigned int xwcm,
26 int gravity,
27
28=== modified file 'src/event.cpp'
29--- src/event.cpp 2012-12-13 10:12:23 +0000
30+++ src/event.cpp 2013-02-12 05:13:48 +0000
31@@ -1288,6 +1288,17 @@
32 if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa))
33 privateScreen.setDefaultWindowAttributes (&wa);
34
35+ /* That being said, we should store as much information as possible
36+ * about it. There may be requests relative to this window that could
37+ * use the data in the XCreateWindowEvent structure, especially the
38+ * override redirect state */
39+ wa.x = event->xcreatewindow.x;
40+ wa.y = event->xcreatewindow.y;
41+ wa.width = event->xcreatewindow.width;
42+ wa.height = event->xcreatewindow.height;
43+ wa.border_width = event->xcreatewindow.border_width;
44+ wa.override_redirect = event->xcreatewindow.override_redirect;
45+
46 foreach (CompWindow *w, screen->windows ())
47 {
48 if (w->priv->serverFrame == event->xcreatewindow.window)
49@@ -1467,7 +1478,15 @@
50 if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa))
51 privateScreen.setDefaultWindowAttributes (&wa);
52
53- PrivateWindow::createCompWindow (getTopWindow ()->id (), getTopServerWindow ()->id (), wa, event->xcreatewindow.window);
54+ /* That being said, we should store as much information as possible
55+ * about it. There may be requests relative to this window that could
56+ * use the data in the XCreateWindowEvent structure, especially the
57+ * override redirect state */
58+ wa.x = event->xreparent.x;
59+ wa.y = event->xreparent.y;
60+ wa.override_redirect = event->xreparent.override_redirect;
61+
62+ PrivateWindow::createCompWindow (getTopWindow ()->id (), getTopServerWindow ()->id (), wa, event->xreparent.window);
63 break;
64 }
65 else
66@@ -2096,8 +2115,13 @@
67
68 if (fsw->type () & CompWindowTypeFullscreenMask)
69 {
70+ ServerLock lock (screen->serverGrabInterface ());
71+
72 /* This will be the window that we must lower relative to */
73- CompWindow *sibling = PrivateWindow::findValidStackSiblingBelow (active, fsw);
74+ CompWindow *sibling =
75+ PrivateWindow::findValidStackSiblingBelow (active,
76+ fsw,
77+ lock);
78
79 if (sibling)
80 {
81@@ -2174,7 +2198,12 @@
82 */
83 if (w)
84 {
85- if (PrivateWindow::stackDocks (w, dockWindows, &xwc, &mask))
86+ ServerLock lock (screen->serverGrabInterface ());
87+ if (PrivateWindow::stackDocks (w,
88+ dockWindows,
89+ &xwc,
90+ &mask,
91+ lock))
92 {
93 Window sibling = xwc.sibling;
94 xwc.stack_mode = Above;
95@@ -2183,7 +2212,7 @@
96 foreach (CompWindow *dw, dockWindows)
97 {
98 xwc.sibling = sibling;
99- dw->configureXWindow (mask, &xwc);
100+ dw->restackAndConfigureXWindow (mask, &xwc, lock);
101 }
102 }
103 }
104
105=== modified file 'src/option/tests/CMakeLists.txt'
106--- src/option/tests/CMakeLists.txt 2012-08-01 00:42:38 +0000
107+++ src/option/tests/CMakeLists.txt 2013-02-12 05:13:48 +0000
108@@ -10,6 +10,7 @@
109 ${compiz_SOURCE_DIR}/src/point/include
110 ${compiz_SOURCE_DIR}/src/window/geometry/include
111 ${compiz_SOURCE_DIR}/src/window/extents/include
112+ ${compiz_SOURCE_DIR}/src/servergrab/include
113 ${COMPIZ_INCLUDE_DIRS}
114 )
115
116
117=== modified file 'src/option/tests/option.cpp'
118--- src/option/tests/option.cpp 2012-09-21 16:15:27 +0000
119+++ src/option/tests/option.cpp 2013-02-12 05:13:48 +0000
120@@ -1,5 +1,9 @@
121 #include <gtest/gtest.h>
122
123+/* XXX: including core.h means that we pull in
124+ * both window.h and screen.h which are cascading
125+ * includes. We should eliminate this dependency
126+ */
127 #include "core/core.h"
128 #include "core/action.h"
129 #include "core/match.h"
130
131=== modified file 'src/outputdevices.h'
132--- src/outputdevices.h 2012-11-14 03:14:31 +0000
133+++ src/outputdevices.h 2013-02-12 05:13:48 +0000
134@@ -25,6 +25,11 @@
135 #include <core/output.h>
136 #include <core/rect.h>
137 #include <core/region.h>
138+
139+/* XXX: Including screen.h includes window.h and other unnecessary
140+ * headers which cause cascading header dependencies. We should seek to
141+ * eliminate this dependency
142+ */
143 #include <core/screen.h>
144
145 namespace compiz
146
147=== modified file 'src/privatewindow.h'
148--- src/privatewindow.h 2012-12-13 10:12:23 +0000
149+++ src/privatewindow.h 2013-02-12 05:13:48 +0000
150@@ -127,19 +127,23 @@
151
152 bool isInvisible() const;
153
154- static bool stackLayerCheck (CompWindow *w,
155- Window clientLeader,
156- CompWindow *below);
157-
158- static bool avoidStackingRelativeTo (CompWindow *w);
159-
160- static CompWindow * findSiblingBelow (CompWindow *w,
161- bool aboveFs);
162-
163- static CompWindow * findLowestSiblingBelow (CompWindow *w);
164-
165- static bool validSiblingBelow (CompWindow *w,
166- CompWindow *sibling);
167+ static bool stackLayerCheck (CompWindow *w,
168+ Window clientLeader,
169+ CompWindow *below,
170+ const ServerLock &lock);
171+
172+ static bool avoidStackingRelativeTo (CompWindow *w, const ServerLock &lock);
173+
174+ static CompWindow * findSiblingBelow (CompWindow *w,
175+ bool aboveFs,
176+ const ServerLock &lock);
177+
178+ static CompWindow * findLowestSiblingBelow (CompWindow *w,
179+ const ServerLock &lock);
180+
181+ static bool validSiblingBelow (CompWindow *w,
182+ CompWindow *sibling,
183+ const ServerLock &lock);
184
185 void saveGeometry (int mask);
186
187@@ -148,19 +152,22 @@
188 void reconfigureXWindow (unsigned int valueMask,
189 XWindowChanges *xwc);
190
191- static bool stackDocks (CompWindow *w,
192- CompWindowList &updateList,
193- XWindowChanges *xwc,
194- unsigned int *mask);
195-
196- static bool stackTransients (CompWindow *w,
197- CompWindow *avoid,
198- XWindowChanges *xwc,
199- CompWindowList &updateList);
200-
201- static void stackAncestors (CompWindow *w,
202- XWindowChanges *xwc,
203- CompWindowList &updateList);
204+ static bool stackDocks (CompWindow *w,
205+ CompWindowList &updateList,
206+ XWindowChanges *xwc,
207+ unsigned int *mask,
208+ const ServerLock &lock);
209+
210+ static bool stackTransients (CompWindow *w,
211+ CompWindow *avoid,
212+ XWindowChanges *xwc,
213+ CompWindowList &updateList,
214+ const ServerLock &lock);
215+
216+ static void stackAncestors (CompWindow *w,
217+ XWindowChanges *xwc,
218+ CompWindowList &updateList,
219+ const ServerLock &lock);
220
221 static bool isAncestorTo (CompWindow *transient,
222 CompWindow *ancestor);
223@@ -172,11 +179,13 @@
224 int addWindowSizeChanges (XWindowChanges *xwc,
225 CompWindow::Geometry old);
226
227- int addWindowStackChanges (XWindowChanges *xwc,
228- CompWindow *sibling);
229+ int addWindowStackChanges (XWindowChanges *xwc,
230+ CompWindow *sibling,
231+ const ServerLock &lock);
232
233 static CompWindow * findValidStackSiblingBelow (CompWindow *w,
234- CompWindow *sibling);
235+ CompWindow *sibling,
236+ const ServerLock &lock);
237
238 void ensureWindowVisibility ();
239
240
241=== modified file 'src/tests/CMakeLists.txt'
242--- src/tests/CMakeLists.txt 2012-12-13 10:12:23 +0000
243+++ src/tests/CMakeLists.txt 2013-02-12 05:13:48 +0000
244@@ -10,6 +10,7 @@
245 ${COMPIZ_MAIN_SOURCE_DIR}/pluginclasshandler/include
246 ${COMPIZ_MAIN_SOURCE_DIR}/window/geometry/include
247 ${COMPIZ_MAIN_SOURCE_DIR}/window/extents/include
248+ ${COMPIZ_MAIN_SOURCE_DIR}/servergrab/include
249 ${COMPIZ_INCLUDE_DIRS}
250 )
251
252
253=== modified file 'src/window.cpp'
254--- src/window.cpp 2013-02-03 18:02:18 +0000
255+++ src/window.cpp 2013-02-12 05:13:48 +0000
256@@ -2552,11 +2552,62 @@
257 }
258 }
259
260+namespace
261+{
262+/* There is a race condition where we can request to restack
263+ * a window relative to a sibling that's been destroyed on
264+ * the server side, but not yet on the client side (eg DestroyNotify).
265+ * In that case the server will report a BadWindow error and refuse
266+ * to process the ConfigureRequest event. This leaves
267+ * serverWindows in an indeterminate state, because we've
268+ * effectively recorded that we successfully put the window
269+ * in a new stack position, even though it will fail later on
270+ * and leave the window in the same stack position. That leaves
271+ * the door open for cascading errors, where other windows successfully
272+ * stack on top of the window which was not successfully restacked, so
273+ * they will all receive invalid stack positions.
274+ *
275+ * In order to alleviate that condition, we need to hold a server grab to
276+ * ensure that the window cannot be destroyed while we are stacking another
277+ * window relative to it, or, if it was destroyed to ensure that querying
278+ * whether or not it exists will return a useful value
279+ *
280+ * Any function which walks the window stack to determine an appropriate
281+ * sibling should always employ this as the last check before returning
282+ * that sibling or considering other windows. It is never a good idea
283+ * to restack relative to a sibling that could have been destroyed. As
284+ * a side effect of this function requiring a ServerLock, any other function
285+ * that uses this one will also require one, and the caller should keep
286+ * the same ServerLock alive when calling through to
287+ * CompWindow::restackAndConfigureXWindow
288+ */
289+bool existsOnServer (CompWindow *window,
290+ const ServerLock &lock)
291+{
292+ /* We only stack relative to frame windows, and we know
293+ * whether or not they exist on the server side, don't
294+ * query whether or not they do */
295+ if (window->frame ())
296+ return true;
297+ else
298+ {
299+ XWindowAttributes attrib;
300+ if (!XGetWindowAttributes (screen->dpy (),
301+ ROOTPARENT (window),
302+ &attrib))
303+ return false;
304+ }
305+
306+ return true;
307+}
308+}
309+
310
311 bool
312-PrivateWindow::stackLayerCheck (CompWindow *w,
313- Window clientLeader,
314- CompWindow *below)
315+PrivateWindow::stackLayerCheck (CompWindow *w,
316+ Window clientLeader,
317+ CompWindow *below,
318+ const ServerLock &lock)
319 {
320 if (isAncestorTo (w, below))
321 return true;
322@@ -2586,7 +2637,8 @@
323 }
324
325 bool
326-PrivateWindow::avoidStackingRelativeTo (CompWindow *w)
327+PrivateWindow::avoidStackingRelativeTo (CompWindow *w,
328+ const ServerLock &lock)
329 {
330 if (w->overrideRedirect ())
331 return true;
332@@ -2611,8 +2663,9 @@
333 stack above, normal windows can be stacked above fullscreen windows
334 (and fullscreen windows over others in their layer) if aboveFs is true. */
335 CompWindow *
336-PrivateWindow::findSiblingBelow (CompWindow *w,
337- bool aboveFs)
338+PrivateWindow::findSiblingBelow (CompWindow *w,
339+ bool aboveFs,
340+ const ServerLock &lock)
341 {
342 CompWindow *below;
343 CompWindow *t = screen->findWindow (w->transientFor ());
344@@ -2645,7 +2698,7 @@
345 for (below = screen->serverWindows ().back (); below;
346 below = below->serverPrev)
347 {
348- if (below == w || avoidStackingRelativeTo (below))
349+ if (below == w || avoidStackingRelativeTo (below, lock))
350 continue;
351
352 /* always above desktop windows */
353@@ -2665,7 +2718,8 @@
354 if (below->priv->type & (CompWindowTypeFullscreenMask |
355 CompWindowTypeDockMask))
356 {
357- if (stackLayerCheck (w, clientLeader, below))
358+ if (stackLayerCheck (w, clientLeader, below, lock) &&
359+ existsOnServer (below, lock))
360 return below;
361 }
362 else
363@@ -2694,7 +2748,8 @@
364 /* fullscreen and normal layer */
365 if (allowedRelativeToLayer)
366 {
367- if (stackLayerCheck (w, clientLeader, below))
368+ if (stackLayerCheck (w, clientLeader, below, lock) &&
369+ existsOnServer (below, lock))
370 return below;
371 }
372 break;
373@@ -2708,7 +2763,8 @@
374 /* goes through the stack, top-down and returns the lowest window we
375 can stack above. */
376 CompWindow *
377-PrivateWindow::findLowestSiblingBelow (CompWindow *w)
378+PrivateWindow::findLowestSiblingBelow (CompWindow *w,
379+ const ServerLock &lock)
380 {
381 CompWindow *below, *lowest = screen->serverWindows ().back ();
382 CompWindow *t = screen->findWindow (w->transientFor ());
383@@ -2736,11 +2792,12 @@
384 for (below = screen->serverWindows ().back (); below;
385 below = below->serverPrev)
386 {
387- if (below == w || avoidStackingRelativeTo (below))
388+ if (below == w || avoidStackingRelativeTo (below, lock))
389 continue;
390
391 /* always above desktop windows */
392- if (below->priv->type & CompWindowTypeDesktopMask)
393+ if ((below->priv->type & CompWindowTypeDesktopMask) &&
394+ existsOnServer (below, lock))
395 return below;
396
397 switch (type) {
398@@ -2755,13 +2812,12 @@
399 if (below->priv->type & (CompWindowTypeFullscreenMask |
400 CompWindowTypeDockMask))
401 {
402- if (!stackLayerCheck (below, clientLeader, w))
403+ if (!stackLayerCheck (below, clientLeader, w, lock) &&
404+ existsOnServer (lowest, lock))
405 return lowest;
406 }
407- else
408- {
409+ else if (existsOnServer (lowest, lock))
410 return lowest;
411- }
412 break;
413 default:
414 {
415@@ -2780,7 +2836,8 @@
416 /* fullscreen and normal layer */
417 if (allowedRelativeToLayer)
418 {
419- if (!stackLayerCheck (below, clientLeader, w))
420+ if (!stackLayerCheck (below, clientLeader, w, lock) &&
421+ existsOnServer (lowest, lock))
422 return lowest;
423 }
424 break;
425@@ -2790,12 +2847,20 @@
426 lowest = below;
427 }
428
429- return lowest;
430+ if (existsOnServer (lowest, lock))
431+ return lowest;
432+ else
433+ {
434+ compLogMessage ("core", CompLogLevelDebug,
435+ "couldn't find window to stack above");
436+ return NULL;
437+ }
438 }
439
440 bool
441-PrivateWindow::validSiblingBelow (CompWindow *w,
442- CompWindow *sibling)
443+PrivateWindow::validSiblingBelow (CompWindow *w,
444+ CompWindow *sibling,
445+ const ServerLock &lock)
446 {
447 CompWindow *t = screen->findWindow (w->transientFor ());
448 Window clientLeader = w->priv->clientLeader;
449@@ -2819,7 +2884,7 @@
450 if (w->priv->transientFor || w->priv->isGroupTransient (clientLeader))
451 clientLeader = None;
452
453- if (sibling == w || avoidStackingRelativeTo (sibling))
454+ if (sibling == w || avoidStackingRelativeTo (sibling, lock))
455 return false;
456
457 /* always above desktop windows */
458@@ -2836,13 +2901,12 @@
459 if (sibling->priv->type & (CompWindowTypeFullscreenMask |
460 CompWindowTypeDockMask))
461 {
462- if (stackLayerCheck (w, clientLeader, sibling))
463+ if (stackLayerCheck (w, clientLeader, sibling, lock) &&
464+ existsOnServer (sibling, lock))
465 return true;
466 }
467- else
468- {
469+ else if (existsOnServer (sibling, lock))
470 return true;
471- }
472 break;
473 default:
474 {
475@@ -2861,7 +2925,8 @@
476 /* fullscreen and normal layer */
477 if (allowedRelativeToLayer)
478 {
479- if (stackLayerCheck (w, clientLeader, sibling))
480+ if (stackLayerCheck (w, clientLeader, sibling, lock) &&
481+ existsOnServer (sibling, lock))
482 return true;
483 }
484 break;
485@@ -3257,10 +3322,11 @@
486 }
487
488 bool
489-PrivateWindow::stackDocks (CompWindow *w,
490- CompWindowList &updateList,
491- XWindowChanges *xwc,
492- unsigned int *mask)
493+PrivateWindow::stackDocks (CompWindow *w,
494+ CompWindowList &updateList,
495+ XWindowChanges *xwc,
496+ unsigned int *mask,
497+ const ServerLock &lock)
498 {
499 CompWindow *firstFullscreenWindow = NULL;
500 CompWindow *belowDocks = NULL;
501@@ -3272,15 +3338,20 @@
502 {
503 /* If there is another toplevel window above the fullscreen one
504 * then we need to stack above that */
505- if ((dw->priv->managed && !dw->priv->unmanaging) &&
506- !(dw->priv->state & CompWindowStateHiddenMask) &&
507- !PrivateWindow::isAncestorTo (w, dw) &&
508- !(dw->type () & (CompWindowTypeFullscreenMask |
509- CompWindowTypeDockMask)) &&
510+ bool currentlyManaged = dw->priv->managed && !dw->priv->unmanaging;
511+ bool visible = !(dw->state () & CompWindowStateHiddenMask);
512+ bool ancestorToClient = PrivateWindow::isAncestorTo (w, dw);
513+ bool acceptableType = !(dw->type () & (CompWindowTypeFullscreenMask |
514+ CompWindowTypeDockMask));
515+ if (currentlyManaged &&
516+ visible &&
517+ acceptableType &&
518+ !ancestorToClient &&
519 !dw->overrideRedirect () &&
520 dw->isViewable ())
521 {
522- belowDocks = dw;
523+ if (existsOnServer (dw, lock))
524+ belowDocks = dw;
525 }
526 }
527 else if (dw->type () & CompWindowTypeFullscreenMask)
528@@ -3291,15 +3362,21 @@
529 firstFullscreenWindow = dw;
530 for (CompWindow *dww = dw->serverPrev; dww; dww = dww->serverPrev)
531 {
532- if ((dw->priv->managed && !dw->priv->unmanaging) &&
533- !(dw->priv->state & CompWindowStateHiddenMask) &&
534- !(dww->type () & (CompWindowTypeFullscreenMask |
535- CompWindowTypeDockMask)) &&
536+ bool currentlyManaged = dw->priv->managed && !dw->priv->unmanaging;
537+ bool visible = !(dw->priv->state & CompWindowStateHiddenMask);
538+ bool acceptableType = !(dww->type () & (CompWindowTypeFullscreenMask |
539+ CompWindowTypeDockMask));
540+ if (currentlyManaged &&
541+ visible &&
542+ acceptableType &&
543 !dww->overrideRedirect () &&
544 dww->isViewable ())
545 {
546- belowDocks = dww;
547- break;
548+ if (existsOnServer (dww, lock))
549+ {
550+ belowDocks = dww;
551+ break;
552+ }
553 }
554 }
555 }
556@@ -3322,10 +3399,11 @@
557 }
558
559 bool
560-PrivateWindow::stackTransients (CompWindow *w,
561- CompWindow *avoid,
562- XWindowChanges *xwc,
563- CompWindowList &updateList)
564+PrivateWindow::stackTransients (CompWindow *w,
565+ CompWindow *avoid,
566+ XWindowChanges *xwc,
567+ CompWindowList &updateList,
568+ const ServerLock &lock)
569 {
570 CompWindow *t;
571 Window clientLeader = w->priv->clientLeader;
572@@ -3341,7 +3419,7 @@
573 if (t->priv->transientFor == w->priv->id ||
574 t->priv->isGroupTransient (clientLeader))
575 {
576- if (!stackTransients (t, avoid, xwc, updateList))
577+ if (!stackTransients (t, avoid, xwc, updateList, lock))
578 return false;
579
580 if (xwc->sibling == t->priv->id ||
581@@ -3349,7 +3427,10 @@
582 return false;
583
584 if (t->priv->mapNum || t->priv->pendingMaps)
585- updateList.push_back (t);
586+ {
587+ if (existsOnServer (t, lock))
588+ updateList.push_back (t);
589+ }
590 }
591 }
592
593@@ -3357,9 +3438,10 @@
594 }
595
596 void
597-PrivateWindow::stackAncestors (CompWindow *w,
598- XWindowChanges *xwc,
599- CompWindowList &updateList)
600+PrivateWindow::stackAncestors (CompWindow *w,
601+ XWindowChanges *xwc,
602+ CompWindowList &updateList,
603+ const ServerLock &lock)
604 {
605 CompWindow *transient = NULL;
606
607@@ -3375,7 +3457,7 @@
608 ancestor = screen->findWindow (w->priv->transientFor);
609 if (ancestor)
610 {
611- if (!stackTransients (ancestor, w, xwc, updateList))
612+ if (!stackTransients (ancestor, w, xwc, updateList, lock))
613 return;
614
615 if (ancestor->priv->type & CompWindowTypeDesktopMask)
616@@ -3386,9 +3468,10 @@
617 return;
618
619 if (ancestor->priv->mapNum || ancestor->priv->pendingMaps)
620- updateList.push_back (ancestor);
621+ if (existsOnServer (ancestor, lock))
622+ updateList.push_back (ancestor);
623
624- stackAncestors (ancestor, xwc, updateList);
625+ stackAncestors (ancestor, xwc, updateList, lock);
626 }
627 }
628 else if (w->priv->isGroupTransient (w->priv->clientLeader))
629@@ -3405,7 +3488,7 @@
630 (a->priv->serverFrame && xwc->sibling == a->priv->serverFrame))
631 break;
632
633- if (!stackTransients (a, w, xwc, updateList))
634+ if (!stackTransients (a, w, xwc, updateList, lock))
635 break;
636
637 if (a->priv->type & CompWindowTypeDesktopMask)
638@@ -3416,16 +3499,35 @@
639 break;
640
641 if (a->priv->mapNum || a->priv->pendingMaps)
642- updateList.push_back (a);
643+ if (existsOnServer (a, lock))
644+ updateList.push_back (a);
645 }
646 }
647 }
648 }
649
650 void
651-CompWindow::configureXWindow (unsigned int valueMask,
652+CompWindow::configureXWindow (unsigned int valueMask,
653 XWindowChanges *xwc)
654 {
655+ if (valueMask & (CWSibling | CWStackMode))
656+ compLogMessage ("core", CompLogLevelWarn,
657+ "use CompWindow::restackAndConfigureXWindow " \
658+ "while holding a ServerLock from the time the "\
659+ "sibling is determined to the end of that operation "\
660+ "to avoid race conditions when restacking relative "\
661+ "to destroyed windows for which we have not yet "\
662+ "received a DestroyNotify for");
663+
664+ if (priv->id)
665+ priv->reconfigureXWindow (valueMask, xwc);
666+}
667+
668+void
669+CompWindow::restackAndConfigureXWindow (unsigned int valueMask,
670+ XWindowChanges *xwc,
671+ const ServerLock &lock)
672+{
673 if (priv->managed && (valueMask & (CWSibling | CWStackMode)))
674 {
675 CompWindowList transients;
676@@ -3440,10 +3542,10 @@
677 have to restack all the windows again. */
678
679 /* transient children above */
680- if (PrivateWindow::stackTransients (this, NULL, xwc, transients))
681+ if (PrivateWindow::stackTransients (this, NULL, xwc, transients, lock))
682 {
683 /* ancestors, siblings and sibling transients below */
684- PrivateWindow::stackAncestors (this, xwc, ancestors);
685+ PrivateWindow::stackAncestors (this, xwc, ancestors, lock);
686
687 for (CompWindowList::reverse_iterator w = ancestors.rbegin ();
688 w != ancestors.rend (); ++w)
689@@ -3462,7 +3564,7 @@
690 xwc->sibling = ROOTPARENT (*w);
691 }
692
693- if (PrivateWindow::stackDocks (this, docks, xwc, &valueMask))
694+ if (PrivateWindow::stackDocks (this, docks, xwc, &valueMask, lock))
695 {
696 Window sibling = xwc->sibling;
697 xwc->stack_mode = Above;
698@@ -3953,8 +4055,9 @@
699 }
700
701 int
702-PrivateWindow::addWindowStackChanges (XWindowChanges *xwc,
703- CompWindow *sibling)
704+PrivateWindow::addWindowStackChanges (XWindowChanges *xwc,
705+ CompWindow *sibling,
706+ const ServerLock &lock)
707 {
708 int mask = 0;
709
710@@ -4044,11 +4147,13 @@
711 }
712 }
713
714+ ServerLock lock (screen->serverGrabInterface ());
715+
716 mask = priv->addWindowStackChanges (&xwc,
717- PrivateWindow::findSiblingBelow (this, aboveFs));
718+ PrivateWindow::findSiblingBelow (this, aboveFs, lock), lock);
719
720 if (mask)
721- configureXWindow (mask, &xwc);
722+ restackAndConfigureXWindow (mask, &xwc, lock);
723 }
724
725 CompWindow *
726@@ -4091,10 +4196,12 @@
727 XWindowChanges xwc = XWINDOWCHANGES_INIT;
728 int mask;
729
730+ ServerLock lock (screen->serverGrabInterface ());
731+
732 mask = priv->addWindowStackChanges (&xwc,
733- PrivateWindow::findLowestSiblingBelow (this));
734+ PrivateWindow::findLowestSiblingBelow (this, lock), lock);
735 if (mask)
736- configureXWindow (mask, &xwc);
737+ restackAndConfigureXWindow (mask, &xwc, lock);
738
739 /* when lowering a window, focus the topmost window if
740 the click-to-focus option is on */
741@@ -4115,8 +4222,10 @@
742 void
743 CompWindow::restackAbove (CompWindow *sibling)
744 {
745+ ServerLock lock (screen->serverGrabInterface ());
746+
747 for (; sibling; sibling = sibling->serverNext)
748- if (PrivateWindow::validSiblingBelow (this, sibling))
749+ if (PrivateWindow::validSiblingBelow (this, sibling, lock))
750 break;
751
752 if (sibling)
753@@ -4124,16 +4233,17 @@
754 XWindowChanges xwc = XWINDOWCHANGES_INIT;
755 int mask;
756
757- mask = priv->addWindowStackChanges (&xwc, sibling);
758+ mask = priv->addWindowStackChanges (&xwc, sibling, lock);
759 if (mask)
760- configureXWindow (mask, &xwc);
761+ restackAndConfigureXWindow (mask, &xwc, lock);
762 }
763 }
764
765 /* finds the highest window under sibling we can stack above */
766 CompWindow *
767-PrivateWindow::findValidStackSiblingBelow (CompWindow *w,
768- CompWindow *sibling)
769+PrivateWindow::findValidStackSiblingBelow (CompWindow *w,
770+ CompWindow *sibling,
771+ const ServerLock &lock)
772 {
773 CompWindow *lowest, *last, *p;
774
775@@ -4144,16 +4254,16 @@
776
777 for (p = sibling; p; p = p->serverNext)
778 {
779- if (!avoidStackingRelativeTo (p))
780+ if (!avoidStackingRelativeTo (p, lock))
781 {
782- if (!validSiblingBelow (p, w))
783+ if (!validSiblingBelow (p, w, lock))
784 return NULL;
785 break;
786 }
787 }
788
789 /* get lowest sibling we're allowed to stack above */
790- lowest = last = findLowestSiblingBelow (w);
791+ lowest = last = findLowestSiblingBelow (w, lock);
792
793 /* walk from bottom up */
794 for (p = screen->serverWindows ().front (); p; p = p->serverNext)
795@@ -4164,10 +4274,10 @@
796 return lowest;
797
798 /* skip windows that we should avoid */
799- if (w == p || avoidStackingRelativeTo (p))
800+ if (w == p || avoidStackingRelativeTo (p, lock))
801 continue;
802
803- if (validSiblingBelow (w, p))
804+ if (validSiblingBelow (w, p, lock))
805 {
806 /* update lowest as we find windows below sibling that we're
807 allowed to stack above. last window must be equal to the
808@@ -4190,11 +4300,35 @@
809 XWindowChanges xwc = XWINDOWCHANGES_INIT;
810 unsigned int mask;
811
812+ ServerLock lock (screen->serverGrabInterface ());
813+
814 mask = priv->addWindowStackChanges (&xwc,
815- PrivateWindow::findValidStackSiblingBelow (this, sibling));
816-
817- if (mask)
818- configureXWindow (mask, &xwc);
819+ PrivateWindow::findValidStackSiblingBelow (this, sibling, lock), lock);
820+
821+ if (mask)
822+ restackAndConfigureXWindow (mask, &xwc, lock);
823+}
824+
825+namespace
826+{
827+void addSizeChangesSyncAndReconfigure (PrivateWindow *priv,
828+ XWindowChanges &xwc,
829+ unsigned int mask,
830+ ServerLock *lock)
831+{
832+ mask |= priv->addWindowSizeChanges (&xwc, priv->serverGeometry);
833+
834+ if (priv->mapNum && (mask & (CWWidth | CWHeight)))
835+ priv->window->sendSyncRequest ();
836+
837+ if (mask)
838+ {
839+ if (lock)
840+ priv->window->restackAndConfigureXWindow (mask, &xwc, *lock);
841+ else
842+ priv->window->configureXWindow (mask, &xwc);
843+ }
844+}
845 }
846
847 void
848@@ -4240,7 +4374,9 @@
849 if (stackingMode == CompStackingUpdateModeInitialMap)
850 aboveFs = true;
851
852- sibling = PrivateWindow::findSiblingBelow (this, aboveFs);
853+ ServerLock lock (screen->serverGrabInterface ());
854+
855+ sibling = PrivateWindow::findSiblingBelow (this, aboveFs, lock);
856
857 if (sibling &&
858 (stackingMode == CompStackingUpdateModeInitialMapDeniedFocus))
859@@ -4255,9 +4391,9 @@
860 * assuing that is allowed (if, for example, our window has
861 * the "above" state, then lowering beneath the active
862 * window may not be allowed). */
863- if (p && PrivateWindow::validSiblingBelow (p, this))
864+ if (p && PrivateWindow::validSiblingBelow (p, this, lock))
865 {
866- p = PrivateWindow::findValidStackSiblingBelow (this, p);
867+ p = PrivateWindow::findValidStackSiblingBelow (this, p, lock);
868
869 /* if we found a valid sibling under the active window, it's
870 our new sibling we want to stack above */
871@@ -4268,16 +4404,17 @@
872
873 /* If sibling is NULL, then this window will go on the bottom
874 * of the stack */
875- mask |= priv->addWindowStackChanges (&xwc, sibling);
876+ mask |= priv->addWindowStackChanges (&xwc, sibling, lock);
877+ addSizeChangesSyncAndReconfigure (priv,
878+ xwc,
879+ mask,
880+ &lock);
881 }
882-
883- mask |= priv->addWindowSizeChanges (&xwc, priv->serverGeometry);
884-
885- if (priv->mapNum && (mask & (CWWidth | CWHeight)))
886- sendSyncRequest ();
887-
888- if (mask)
889- configureXWindow (mask, &xwc);
890+ else
891+ addSizeChangesSyncAndReconfigure (priv,
892+ xwc,
893+ mask,
894+ NULL);
895 }
896
897 void
898@@ -5511,6 +5648,15 @@
899 * for buttons that we don't actually need at that point
900 * anyways)
901 */
902+class DummyServerGrab :
903+ public ServerGrabInterface
904+{
905+ public:
906+
907+ void grabServer () {}
908+ void syncServer () {}
909+ void ungrabServer () {}
910+};
911
912 void
913 PrivateWindow::updatePassiveButtonGrabs ()
914@@ -5534,8 +5680,13 @@
915 {
916 if (screen->getCoreOptions().optionGetRaiseOnClick ())
917 {
918+ /* We do not actually need a server grab here since
919+ * there is no risk to our internal state */
920+ DummyServerGrab grab;
921+ ServerLock lock (&grab);
922+
923 CompWindow *highestSibling =
924- PrivateWindow::findSiblingBelow (window, true);
925+ PrivateWindow::findSiblingBelow (window, true, lock);
926
927 /* Check if this window is permitted to be raised */
928 for (CompWindow *above = window->serverNext;
929
930=== modified file 'tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp'
931--- tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp 2012-12-05 14:33:06 +0000
932+++ tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp 2013-02-12 05:13:48 +0000
933@@ -293,3 +293,83 @@
934 EXPECT_EQ (*it++, w3);
935 EXPECT_EQ (*it++, w2);
936 }
937+
938+TEST_F (CompizXorgSystemStackingTest, TestCreateRelativeToDestroyedWindowFindsAnotherAppropriatePosition)
939+{
940+ ::Display *dpy = Display ();
941+ ct::PropertyNotifyXEventMatcher matcher (dpy, "_NET_CLIENT_LIST_STACKING");
942+
943+ Window dock = ct::CreateNormalWindow (dpy);
944+
945+ /* Make it a dock */
946+ MakeDock (dpy, dock);
947+
948+ /* Immediately map the dock window and clear the event queue for it */
949+ XMapRaised (dpy, dock);
950+
951+ ASSERT_TRUE (Advance (dpy, ct::WaitForEventOfTypeOnWindow (dpy, dock, ReparentNotify, -1, -1)));
952+ ASSERT_TRUE (Advance (dpy, ct::WaitForEventOfTypeOnWindow (dpy, dock, MapNotify, -1, -1)));
953+
954+ /* Dock window needs to be in the client list */
955+ ASSERT_TRUE (Advance (dpy, ct::WaitForEventOfTypeOnWindowMatching (dpy,
956+ DefaultRootWindow (dpy),
957+ PropertyNotify,
958+ -1,
959+ -1,
960+ matcher)));
961+ std::list <Window> clientList = ct::NET_CLIENT_LIST_STACKING (dpy);
962+ ASSERT_EQ (clientList.size (), 1);
963+
964+ Window w1 = ct::CreateNormalWindow (dpy);
965+ Window w2 = ct::CreateNormalWindow (dpy);
966+
967+ XMapRaised (dpy, w1);
968+
969+ /* All reparented and mapped */
970+ ASSERT_TRUE (Advance (dpy, ct::WaitForEventOfTypeOnWindow (dpy,w1, ReparentNotify, -1, -1)));
971+ ASSERT_TRUE (Advance (dpy, ct::WaitForEventOfTypeOnWindow (dpy, w1, MapNotify, -1, -1)));
972+
973+ /* Grab the server so that we can guaruntee that all of these requests
974+ * happen before compiz gets them */
975+ XGrabServer (dpy);
976+ XSync (dpy, false);
977+
978+ /* Map the second window, so it ideally goes above w1. Compiz will
979+ * receive the MapRequest for this first */
980+ XMapRaised (dpy, w2);
981+
982+ /* Create window that has w2 as its ideal above-candidate
983+ * (compiz will receive the CreateNotify for this window
984+ * after the MapRequest but before the subsequent MapNotify) */
985+ Window w3 = ct::CreateNormalWindow (dpy);
986+
987+ XMapRaised (dpy, w3);
988+
989+ /* Destroy w2 */
990+ XDestroyWindow (dpy, w2);
991+
992+ XUngrabServer (dpy);
993+ XSync (dpy, false);
994+
995+ /* Reparented and mapped */
996+ ASSERT_TRUE (Advance (dpy, ct::WaitForEventOfTypeOnWindow (dpy, w3, ReparentNotify, -1, -1)));
997+ ASSERT_TRUE (Advance (dpy, ct::WaitForEventOfTypeOnWindow (dpy, w3, MapNotify, -1, -1)));
998+
999+ /* Update _NET_CLIENT_LIST_STACKING twice */
1000+ ASSERT_TRUE (Advance (dpy, ct::WaitForEventOfTypeOnWindowMatching (dpy,
1001+ DefaultRootWindow (dpy),
1002+ PropertyNotify,
1003+ -1,
1004+ -1,
1005+ matcher)));
1006+
1007+ /* Check the client list to see that dock > w3 > w1 */
1008+ clientList = ct::NET_CLIENT_LIST_STACKING (dpy);
1009+
1010+ std::list <Window>::iterator it = clientList.begin ();
1011+
1012+ EXPECT_EQ (3, clientList.size ());
1013+ EXPECT_EQ (w1, (*it++));
1014+ EXPECT_EQ (w3, (*it++));
1015+ EXPECT_EQ (dock, (*it++));
1016+}

Subscribers

People subscribed via source and target branches