Merge lp:~vanvugt/compiz/fix-1019337.2 into lp:compiz/0.9.8

[Daniel van Vugt]

Fix crash LP: #1019337 properly this time.

The original fix did not handle the case where:
    (win != NULL && xid != None && !valid_window(xid))
so would still exit with an X error.

Now we trap and handle such errors gracefully, instead of hitting the
default handler which kills gtk-window-decorator.

Fix crash LP: #1019337 properly this time.

The original fix did not handle the case where:
    (win != NULL && xid != None && !valid_window(xid))
so would still exit with an X error.

Now we trap and handle such errors gracefully, instead of hitting the
default handler which kills gtk-window-decorator.

UNBLOCK

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:3285
http://jenkins.qa.ubuntu.com/job/compiz-ci/14/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./distribution=quantal,flavor=amd64/14/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./distribution=quantal,flavor=i386/14/console

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

Also good,

23 if (result == Success && error == Success && data)

I don't think there's a reason to check for result and error being Success here - my understanding of the protocol is that XGetWindowProperty is a synchronous call and returns its error code (in this case being Success, BadValue, BadAtom or BadWindow) (http://www.x.org/docs/XProtocol/proto.pdf)). However, the error traps are necessary because we don't want the process to abort.

We should find other unguarded X Protocol calls which may also generate such errors too and guard them as well.

If it turns out that the actual XErrorEvent trapped may return something different to XGetWindowProperty then it's an Approve from me, otherwise remove the other redundant check and consider it Approved.

(No tests required in gtk-window-decorator now, there is no testing framework for it)

[Tim Penhey (thumper)]

I'm not going to comment on which result is more important, I just notice that gdk_error_trap_pop is defined to return either X error code or 0 on success.

We are comparing the X function return (an int) against Success, and the g_ func return (a gint) against Success. I suggest we change the error check to be (error == 0). Even if Success is defined to be zero somewhere, I think this makes the API clearer.

[Daniel van Vugt (vanvugt)]

In X error codes, "Success" is what you're meant to use:

% grep "#define Success" /usr/include/X11/X.h
#define Success 0 /* everything's okay */

[Daniel van Vugt (vanvugt)]

Yes, it is possible result != error. Because all the docs I found said that result is either Success or undefined. However 'error' tells you what the error was.

However it's a moot point since all we're testing for is Success. So there's no need to check 'error == Success' too. It's redundant but harmless.

[Łukasz Zemczak (sil2100)]

Looks safe. Didier also pointed that it can't possibly do any harm, so I'm approving.

review: Approve

=== modified file 'gtk/window-decorator/wnck.c'
--- gtk/window-decorator/wnck.c	2012-07-11 04:34:54 +0000
+++ gtk/window-decorator/wnck.c	2012-07-16 05:39:21 +0000
@@ -50,10 +50,13 @@
 	    if (xid == None)
 		return "bare";
 
+	    gdk_error_trap_push ();
 	    result = XGetWindowProperty (gdk_x11_get_default_xdisplay (), xid,
 					 net_wm_state_atom,
 					 0L, 1024L, FALSE, XA_ATOM, &actual, &format,
 					 &n, &left, &data);
+	    gdk_flush ();
+	    gdk_error_trap_pop ();
 
 	    if (result == Success && data)
 	    {