Merge lp:~sil2100/compiz/quickfix_1141079_0.9.9 into lp:compiz/0.9.9

[Łukasz Zemczak]

Qt is stupid since it uses the stupid X11 protocol - let's not set all the attributes (especially override_redirect) every time, only in cases when it's actually needed (LP: #1141079)

- Problem:

The fix introduced in revision 3606 triggers a regression with Qt menus. It seems that passing on the override_redirect state from xcreatewindow results in this regression happening. Pop-up menus in Qt apps sometimes are detected as a separate window.

- Fix:

We try not to invalidly unnecessarily pass the additional attributes to the event when there is no need. This way the override_redirect attribute is not corrupted and all is cool.

- Tests:

Attached related gtest tests.

[Sam Spilsbury (smspillaz)]

Ugh, I hate all these broken toolkits.

Now I remember - Qt was stupid and sets the override redirect bit to zero for all windows until MapRequest time, something that IMO is broken about the X protocol (you shouldn't be able to change the override redirect state as there are no events for that, and it causes race conditions later) and also broken about Qt because it actually uses that bit of the protocol.

Can you:

 1. Put the wa.whatever assignments within the preceeding if () block, eg:

    if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa))
    {
 privateScreen.setDefaultWindowAttributes (&wa);
 wa.x = event->xcreatewindow.x;
 wa.y = event->xcreatewindow.y;
 wa.width = event->xcreatewindow.width;
 wa.height = event->xcreatewindow.height;
 wa.border_width = event->xcreatewindow.border_width;
 wa.override_redirect = event->xcreatewindow.override_redirect;
    }

    I think the big concern that I had when writing this chunk of code was that destroyed windows on the server side (eg, no XGetWindowAttributes fails) wouldn't get any attributes set, including their override_redirect attribute. For Qt windows, it means that we'll just have to work off an invalid override_redirect attribute when handling incoming restack requests relative to their now-destroyed windows, but it shouldn't be too big a deal as I think we don't restack relative to unmapped windows which haven't yet been managed. For everyone else it means we have information that should be correct in 99% of cases.

 2. Move the comment up there
 3. Add some tests for that behavior, because this Qt "feature" is easy to forget about
    - have a look in compiz_xorg_gtest_configure_window.cpp for the general integration test structure
    - I think a general test for this would look like (in the AutostartCompizXorgSystemTestWithTestHelper fixture):
      a. Grab Server -> Create Window using XCreateWindow (with XCreateWindowAttributes.override_redirect = 1) -> Destroy Window -> Sync/Ungrab Server/Sync -> call WaitForWindowCreation and additionally verify that the override_redirect bit is "1"
         i. testhelper.cpp should be modified to return some basic window attributes in the TEST_HELPER_WINDOW_READY message in TestHelperWindow ctor. That would window->overrideRedirect () (converted to either 1 or 0)
         ii. WaitForWindowCreation could return a vector of long (extracted from event.xclient.data.l)
         iii. Have a simple helper function IsOverrideRedirect (std::vector <long> &data) to inspect that vector for the appropriate bit.
         iv. EXPECT_TRUE that function.
      b. Grab Server -> Create Window using XCreateWindow (with XCreateWindowAttributes.override_redirect = 0) -> Destroy Window -> Sync/Ungrab Server/Sync -> call WaitForWindowCreation and additionally verify that the override_redirect bit is "0"
      c. Grab Server -> Create Window using XCreateWindow (with XSetWindowAttributes.override_redirect = 0) -> call XChangeWindowAttributes (with XWindowAttributes.override_redirect = 1) -> Sync/Ungrab Server/Sync -> call WaitForWindowCreation and additionally verify that the override_redirect bit is "1"
      d. Grab Server -> Create W...

[Łukasz Zemczak (sil2100)]

Thanks for the comment and review Sam (as always, excellently described and explained)! Will proceed as advised. Indeed testing this behaviour might be the right way to go. Once I'm over the current task I'm working on, I'll update the merge branches.

[Sam Spilsbury (smspillaz)]

Looks like its on the right track!

(I'm changing this to WIP so I can keep track). Thanks heaps.

[Łukasz Zemczak (sil2100)]

Hi Sam! Could you check the testing code?

Since I don't know if I'm doing anything wrong, but it seems the override_redirect attribute is not properly propagated with the XCreateWindow() call when using this branch. The CreateDestroyWindowOverrideRedirectTrue test I created fails, and when debugging in detail it seems that the CompWindow that reaches the TestHelperWindow ctor for the window I created (with the override_redirect attribute set) has the override_redirect flag reset to 0 again ;/ Not sure where that happens...
Do you have any clue what's wrong by any chance?

review: Needs Information

[Sam Spilsbury (smspillaz)]

Heya,

Haven't had a chance to run the code yet, but I spotted something here:

97 + Window w = XCreateWindow (dpy, DefaultRootWindow (dpy),
98 + 0, 0, 100, 100, 0, CopyFromParent,
99 + InputOutput, CopyFromParent, 0,
100 + &attr);

You need to specify a changemask for attr, eg

unsigned int mask = CWOverrideRedirect;

97 + Window w = XCreateWindow (dpy, DefaultRootWindow (dpy),
98 + 0, 0, 100, 100, 0, CopyFromParent,
99 + InputOutput, CopyFromParent, mask,
100 + &attr);

[Sam Spilsbury (smspillaz)]

Looks great for now.

There is some further refactoring that I think could be done, but this should probably be merged into a change focused on refactoring and not on blocking this work.

review: Approve

[Daniel van Vugt (vanvugt)]

Please target all fixes to lp:compiz first (0.9.10). And then lp:compiz/0.9.9 (the new raring branch) second.

review: Needs Resubmitting

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-autolanding/19/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-amd64-autolanding/19/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-i386-autolanding/23/console

review: Needs Fixing (continuous-integration)

[Łukasz Zemczak (sil2100)]

Waiting for https://code.launchpad.net/~sil2100/compiz/quickfix_1141079_trunk/+merge/157084 - it currently fails due to jenkins CI compiz issues.

[Sam Spilsbury (smspillaz)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3645
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~sil2100/compiz/quickfix_1141079_0.9.9/+merge/156779/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-ci/1/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/1/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-0.9.9-ci/1/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3645
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~sil2100/compiz/quickfix_1141079_0.9.9/+merge/156779/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-ci/2/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/2/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-0.9.9-ci/2/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3645
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~sil2100/compiz/quickfix_1141079_0.9.9/+merge/156779/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-ci/3/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-0.9.9-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/3

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-0.9.9-ci/3/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/event.cpp'
--- src/event.cpp	2013-02-10 05:01:50 +0000
+++ src/event.cpp	2013-04-03 09:12:32 +0000
@@ -1285,19 +1285,20 @@
 	 * the window to the window list as we might get configure requests
 	 * which require us to stack other windows relative to it. Setting
 	 * some default values if this is the case. */
-	if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa))
+	if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa)) {
 	    privateScreen.setDefaultWindowAttributes (&wa);
 
-	/* That being said, we should store as much information as possible
-	 * about it. There may be requests relative to this window that could
-	 * use the data in the XCreateWindowEvent structure, especially the
-	 * override redirect state */
-	wa.x = event->xcreatewindow.x;
-	wa.y = event->xcreatewindow.y;
-	wa.width = event->xcreatewindow.width;
-	wa.height = event->xcreatewindow.height;
-	wa.border_width = event->xcreatewindow.border_width;
-	wa.override_redirect = event->xcreatewindow.override_redirect;
+	    /* That being said, we should store as much information as possible
+	     * about it. There may be requests relative to this window that could
+	     * use the data in the XCreateWindowEvent structure, especially the
+	     * override redirect state */
+	    wa.x = event->xcreatewindow.x;
+	    wa.y = event->xcreatewindow.y;
+	    wa.width = event->xcreatewindow.width;
+	    wa.height = event->xcreatewindow.height;
+	    wa.border_width = event->xcreatewindow.border_width;
+	    wa.override_redirect = event->xcreatewindow.override_redirect;
+	}
 
 	foreach (CompWindow *w, screen->windows ())
 	{
@@ -1475,16 +1476,17 @@
 		 * the window to the window list as we might get configure requests
 		 * which require us to stack other windows relative to it. Setting
 		 * some default values if this is the case. */
-		if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa))
+		if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa)) {
 		    privateScreen.setDefaultWindowAttributes (&wa);
 
-		/* That being said, we should store as much information as possible
-		 * about it. There may be requests relative to this window that could
-		 * use the data in the XCreateWindowEvent structure, especially the
-		 * override redirect state */
-		wa.x = event->xreparent.x;
-		wa.y = event->xreparent.y;
-		wa.override_redirect = event->xreparent.override_redirect;
+		    /* That being said, we should store as much information as possible
+		     * about it. There may be requests relative to this window that could
+		     * use the data in the XCreateWindowEvent structure, especially the
+		     * override redirect state */
+		    wa.x = event->xreparent.x;
+		    wa.y = event->xreparent.y;
+		    wa.override_redirect = event->xreparent.override_redirect;
+		}
 
 		PrivateWindow::createCompWindow (getTopWindow ()->id (), getTopServerWindow ()->id (), wa, event->xreparent.window);
 		break;
 
=== modified file 'tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp'
--- tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp	2013-04-02 06:17:37 +0000
+++ tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp	2013-04-03 09:12:32 +0000
@@ -212,6 +212,14 @@
 	bool VerifySetFrameExtentsResponse (Window w, int left, int right, int top, int bottom);
 	bool VerifyWindowSize (Window w, int x, int y, int width, int height);
 
+	/* Helper functions for the Create*WindowOverrideRedirect* tests */
+	Window GrabAndCreateWindowWithAttrs (::Display *dpy,
+					     XSetWindowAttributes &attr);
+	void UngrabSyncAndTestOverride (::Display *dpy,
+					Window w,
+					bool overrideAssert);
+
+
     protected:
 
 	ReparentedWindow CreateWindow (::Display *);
@@ -750,3 +758,92 @@
 				   currentWidth,
 				   currentHeight));
 }
+
+Window
+CompizXorgSystemConfigureWindowTest::GrabAndCreateWindowWithAttrs (::Display *dpy,
+								   XSetWindowAttributes &attr)
+{
+    /* NOTE: We need to ungrab the server after this function */
+    XGrabServer (dpy);
+
+    /* Create a window with our custom attributes */
+    return XCreateWindow (dpy, DefaultRootWindow (dpy),
+			  0, 0, 100, 100, 0, CopyFromParent,
+			  InputOutput, CopyFromParent, CWOverrideRedirect,
+			  &attr);
+}
+
+void
+CompizXorgSystemConfigureWindowTest::UngrabSyncAndTestOverride (::Display *dpy,
+								Window w,
+								bool overrideAssert)
+{
+    XSync (dpy, false);
+
+    XUngrabServer (dpy);
+
+    XSync (dpy, false);
+
+    /* Check if the created window had the attributes passed correctly */
+    std::vector <long> data = WaitForWindowCreation (w);
+    EXPECT_EQ (overrideAssert, IsOverrideRedirect (data));
+}
+
+
+TEST_F (CompizXorgSystemConfigureWindowTest, CreateDestroyWindowOverrideRedirectTrue)
+{
+    ::Display *dpy = Display ();
+    XSetWindowAttributes attr;
+    attr.override_redirect = 1;
+
+    Window w = GrabAndCreateWindowWithAttrs (dpy, attr);
+
+    XDestroyWindow (dpy, w);
+
+    UngrabSyncAndTestOverride (dpy, w, true);
+}
+
+TEST_F (CompizXorgSystemConfigureWindowTest, CreateDestroyWindowOverrideRedirectFalse)
+{
+    ::Display *dpy = Display ();
+    XSetWindowAttributes attr;
+    attr.override_redirect = 0;
+
+    Window w = GrabAndCreateWindowWithAttrs (dpy, attr);
+
+    XDestroyWindow (dpy, w);
+
+    UngrabSyncAndTestOverride (dpy, w, false);
+}
+
+TEST_F (CompizXorgSystemConfigureWindowTest, CreateChangeWindowOverrideRedirectTrue)
+{
+    ::Display *dpy = Display ();
+    XSetWindowAttributes attr;
+    attr.override_redirect = 0;
+
+    Window w = GrabAndCreateWindowWithAttrs (dpy, attr);
+
+    attr.override_redirect = 1;
+    XChangeWindowAttributes (dpy, w, CWOverrideRedirect, &attr);
+
+    UngrabSyncAndTestOverride (dpy, w, true);
+
+    XDestroyWindow (dpy, w);
+}
+
+TEST_F (CompizXorgSystemConfigureWindowTest, CreateChangeWindowOverrideRedirectFalse)
+{
+    ::Display *dpy = Display ();
+    XSetWindowAttributes attr;
+    attr.override_redirect = 1;
+
+    Window w = GrabAndCreateWindowWithAttrs (dpy, attr);
+
+    attr.override_redirect = 0;
+    XChangeWindowAttributes (dpy, w, CWOverrideRedirect, &attr);
+
+    UngrabSyncAndTestOverride (dpy, w, false);
+
+    XDestroyWindow (dpy, w);
+}
 
=== modified file 'tests/xorg-gtest/include/compiz-xorg-gtest.h'
--- tests/xorg-gtest/include/compiz-xorg-gtest.h	2013-02-23 10:47:18 +0000
+++ tests/xorg-gtest/include/compiz-xorg-gtest.h	2013-04-03 09:12:32 +0000
@@ -214,7 +214,8 @@
 	    protected:
 
 		Atom FetchAtom (const char *);
-		void WaitForWindowCreation (Window w);
+		std::vector <long> WaitForWindowCreation (Window w);
+		bool IsOverrideRedirect (std::vector <long> &data);
 
 		virtual int  GetEventMask ();
 
 
=== modified file 'tests/xorg-gtest/plugins/testhelper/src/testhelper.cpp'
--- tests/xorg-gtest/plugins/testhelper/src/testhelper.cpp	2013-02-14 12:12:53 +0000
+++ tests/xorg-gtest/plugins/testhelper/src/testhelper.cpp	2013-04-03 09:12:32 +0000
@@ -192,6 +192,7 @@
 
     std::vector <long> data;
     data.push_back (static_cast <long> (window->id ()));
+    data.push_back (static_cast <long> (window->overrideRedirect ()));
     ct::SendClientMessage (screen->dpy (),
 			   ts->fetchAtom (ctm::TEST_HELPER_WINDOW_READY),
 			   screen->root (),
 
=== modified file 'tests/xorg-gtest/src/compiz-xorg-gtest.cpp'
--- tests/xorg-gtest/src/compiz-xorg-gtest.cpp	2013-02-23 08:23:05 +0000
+++ tests/xorg-gtest/src/compiz-xorg-gtest.cpp	2013-04-03 09:12:32 +0000
@@ -738,7 +738,7 @@
 	std::auto_ptr <ct::MessageAtoms> mMessages;
 };
 
-void
+std::vector <long>
 ct::AutostartCompizXorgSystemTestWithTestHelper::WaitForWindowCreation (Window w)
 {
     ::Display *dpy = Display ();
@@ -758,7 +758,19 @@
 
     }
 
-    ASSERT_TRUE (requestAcknowledged);
+    EXPECT_TRUE (requestAcknowledged);
+
+    std::vector <long> data;
+    for (int i = 0; i < 5; ++i)
+	data.push_back(static_cast <long> (event.xclient.data.l[i]));
+
+    return data;
+}
+
+bool
+ct::AutostartCompizXorgSystemTestWithTestHelper::IsOverrideRedirect (std::vector <long> &data)
+{
+    return (data[1] > 0) ? true : false;
 }
 
 Atom