Compiz Core

Merge lp://qastaging/~smspillaz/compiz-core/compiz-core.decor_sync_996901 into lp://qastaging/compiz-core/0.9.8

compiz-core.decor_sync_996901
Merge into 0.9.8

Proposed by Sam Spilsbury on 2012-05-10

Status:	Merged
Merged at revision:	3137
Proposed branch:	lp://qastaging/~smspillaz/compiz-core/compiz-core.decor_sync_996901
Merge into:	lp://qastaging/compiz-core/0.9.8
Diff against target:	1474 lines (+856/-67) 15 files modified gtk/window-decorator/decorator.c (+55/-25) gtk/window-decorator/events.c (+12/-1) gtk/window-decorator/gtk-window-decorator.c (+9/-0) gtk/window-decorator/gtk-window-decorator.h (+8/-0) gtk/window-decorator/wnck.c (+13/-11) include/decoration.h (+18/-0) libdecoration/decoration.c (+86/-0) plugins/decor/CMakeLists.txt (+3/-1) plugins/decor/src/decor.cpp (+99/-22) plugins/decor/src/decor.h (+35/-7) plugins/decor/src/pixmap-requests/CMakeLists.txt (+62/-0) plugins/decor/src/pixmap-requests/include/pixmap-requests.h (+188/-0) plugins/decor/src/pixmap-requests/src/pixmap-requests.cpp (+93/-0) plugins/decor/src/pixmap-requests/tests/CMakeLists.txt (+15/-0) plugins/decor/src/pixmap-requests/tests/pixmap-requests/src/test-decor-pixmap-requests.cpp (+160/-0)
To merge this branch:	bzr merge lp://qastaging/~smspillaz/compiz-core/compiz-core.decor_sync_996901
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Daniel van Vugt		Approve on 2012-05-15
Alan Griffiths	2012-05-10	Approve on 2012-05-14
Review via email: mp+105344@code.qastaging.launchpad.net

Commit message

Add synchronization primitives to the decoration protocol so that there isn't
a race where we bind a texture that's being freed. (LP: #454218) (LP: #929989)

Also fix a crash in the first attempt at this (LP: #996901)

Description of the change

Add synchronization primitives to the decoration protocol so that there isn't a race where we bind a texture that's being freed.

Things changed

* update_window_decoration_size made semi-private, users should use request_update_window_decoration_size instead, which prepares for a pixmap update and sends a message to the decorated process that a new pixmap is waiting.
* decor_post_pending : tells the decorated process that a new decoration is pending
* decor_post_request : tells the decorator that a new pixmap can be drawn to
* decor_post_delete_pixmap : tells the decorator that a pixmap is no longer in use by the decorated process and can be deleted by the server

I also did some necessary refactoring so we could get this under test, namely:

DecorPixmap class (implements DecorPixmapInterface): a simple RAII class which ensures that a DecorPixmapDeletionInterface is invoked to delete the server side pixmap when the DecorPixmap class is destructed

X11DecorPixmapReceiver class (implements DecorPixmapReceiverInterface): a per-decoration owned class which tracks the update state of the pixmap owned by the decoration from the decorator, calls through to appropriate methods on a DecorPixmapRequestorInterface to request new pixmaps (ensuring that chattyness is reduced to when a) the decorator tells us that a new pixmap is ready and b) when a new pixmap was already pending and we need to fetch the newest one)

X11DecorPixmapRequestor class (implements DecorPixmapRequestorInterface): a per-window owned class which handles dispatch to the decorator (eg decor_post_pending) and requests from the decorator in _COMPIZ_DECOR_PENDING messages to communicate when decorations should be redrawn at new sizes

X11PixmapDeletor class (implements DecorPixmapDeletionInterface): a per-pixmap class which sends decor_post_delete_pixmap to the decorator when a pixmap is no longer in use by the decor plugin

DecorationListFindMatchingInterface: an interface implemented by DecorationList to find a decoration matching a specified criteria

DecorationInterface: an interface implemented by Decoration to get the receiver and properties about that decoration.

Tests included (Google Mock)

Resubmitted to cover a potential race condition in LP#996901

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2012-05-10:

Looks sensible - but so did the version that caused problems.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-05-10:

> Looks sensible - but so did the version that caused problems.

A race condition in the untested part of the code. we should look into getting gtk-window-decorator under test thought I fear there might be some larger architectural changes there that we just don't have time to do :(

Revision history for this message

triplesqaurednine (triplesquarednine) wrote on 2012-05-12:

@Sam,

I just tested this branch and it works.

if you like i can still test r3131 to see what the actual problem was, but it would appear your changes in this branch work.

cheerz

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-05-12:

> @Sam,
>
> I just tested this branch and it works.
>
> if you like i can still test r3131 to see what the actual problem was, but it
> would appear your changes in this branch work.
>
> cheerz

It's probably worth mentioning as it would be a needle in the haystack in the diff due to the speedy revert

635 if (!d->decorated)
636 return FALSE;
637

There was a race where a window could have its decoration resources freed by the time compiz got back to us about whether or not it was ok to re-update decorations.

As a side note, this particular part of the code has always suffered from such race conditions which can lead to random crashes. It is worth looking into a way that we can fix this, but like I said earlier, I fear that larger architectural changes may be required.

Revision history for this message

Alan Griffiths (alan-griffiths) on 2012-05-14:

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-05-15:

Unfortunately, I find gtk-window-decorator crashes with this branch whenever I minimize a window:

(gdb) bt
#0 max_window_name_width (win=0x243c0e0) at /home/dan/bzr/compiz-core/tmp.sync/gtk/window-decorator/decorator.c:420
#1 0x000000000041a377 in request_update_window_decoration_size (win=0x243c0e0) at /home/dan/bzr/compiz-core/tmp.sync/gtk/window-decorator/decorator.c:589
#2 0x00000000004173c7 in active_window_changed (screen=0x2366810) at /home/dan/bzr/compiz-core/tmp.sync/gtk/window-decorator/wnck.c:645
#3 0x00007fd8312ad354 in g_cclosure_marshal_VOID__OBJECTv () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#4 0x00007fd8312a9eca in ?? () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#5 0x00007fd8312c2741 in g_signal_emit_valist () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#6 0x00007fd8312c3242 in g_signal_emit () from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#7 0x00007fd8329404d0 in ?? () from /usr/lib/libwnck-1.so.22
#8 0x00007fd8329413f4 in ?? () from /usr/lib/libwnck-1.so.22
#9 0x00007fd830fecc9a in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#10 0x00007fd830fed060 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#11 0x00007fd830fed45a in g_main_loop_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#12 0x00007fd8324242f7 in gtk_main () from /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0
#13 0x000000000040af84 in main (argc=2, argv=0x7fff0347f138) at /home/dan/bzr/compiz-core/tmp.sync/gtk/window-decorator/gtk-window-decorator.c:460
(gdb)