Merge lp:~compiz-team/compiz/compiz.fix_1085591 into lp:compiz/0.9.9
| Status: | Superseded |
|---|---|
| Proposed branch: | lp:~compiz-team/compiz/compiz.fix_1085591 |
| Merge into: | lp:compiz/0.9.9 |
| Diff against target: |
1794 lines (+1206/-185) 21 files modified
compizconfig/libcompizconfig/tests/compizconfig_test_ccs_util.cpp (+1/-29) include/core/screen.h (+1/-0) src/global.cpp (+1/-0) src/main.cpp (+4/-0) src/screen.cpp (+165/-98) tests/CMakeLists.txt (+2/-0) tests/acceptance-tests/CMakeLists.txt (+1/-0) tests/acceptance-tests/xorg-gtest/CMakeLists.txt (+11/-0) tests/acceptance-tests/xorg-gtest/tests/CMakeLists.txt (+21/-0) tests/acceptance-tests/xorg-gtest/tests/README (+3/-0) tests/acceptance-tests/xorg-gtest/tests/compiz_acceptance_replace_current_wm.cpp (+255/-0) tests/shared/CMakeLists.txt (+2/-0) tests/shared/gtest_shared_asynctask.h (+53/-0) tests/shared/gtest_shared_tmpenv.h (+56/-0) tests/shared/src/CMakeLists.txt (+7/-0) tests/shared/src/gtest_shared_asynctask.cpp (+138/-0) tests/system/xorg-gtest/include/compiz-xorg-gtest.h (+40/-3) tests/system/xorg-gtest/src/compiz-xorg-gtest.cpp (+199/-20) tests/system/xorg-gtest/tests/CMakeLists.txt (+23/-9) tests/system/xorg-gtest/tests/compiz_xorg_gtest_icccm.cpp (+136/-0) tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp (+87/-26) |
| To merge this branch: | bzr merge lp:~compiz-team/compiz/compiz.fix_1085591 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Daniel van Vugt | 2012-12-03 | Needs Fixing on 2012-12-04 | |
| PS Jenkins bot | continuous-integration | Approve on 2012-12-03 | |
|
Review via email:
|
|||
This proposal has been superseded by a proposal from 2012-12-05.
Commit Message
Fix race condition causing --replace to fail occasionally.
Clear XErrors right before taking SubstructureRed
This changes the startup order a fair bit. The new order is now:
1. Do non-critical initialization that does not require a server grab
2. Do the ICCCM check to shut down the other window manager
3. Take server grab
4. Create edge windows
5. Clear error buffer
6. Attempt to take SubstructureRed
7. Clear error buffer -> if fail, exit
8. Create handles based on XQueryTree
9. Release server grab
(LP: #1085591)
System test added to demonstrate that compiz should exit when another client has SubstructureRed
In order to make this work properly, some changes had to be made to compiz. This adds a new runtime switch --send-
Also fixed race condition caused by destroying the WMSnSelectionWindow before shutdown.
Because we haven't changed our active event mask
to remove SubstructureRed
compliant window managers may receive a DestroyNotify
(eg, because we're blocked on XSync) before we get
a chance to close our display connection and remove
our SubstructureRed
to fail to start.
The selection window is destroyed anyways when we
close our connection, and that is a very accurate
indicator to other WM's that we are well and truly
gone because the protocol requires the implementation
to remove all client event masks before destroying
windows.
Added acceptance-tests for testing that behaviour. They are not added to the default test target for obvious reasons.
Description of the Change
Fix race condition causing --replace to fail occasionally.
Clear XErrors right before taking SubstructureRed
This changes the startup order a fair bit. The new order is now:
1. Do non-critical initialization that does not require a server grab
2. Do the ICCCM check to shut down the other window manager
3. Take server grab
4. Create edge windows
5. Clear error buffer
6. Attempt to take SubstructureRed
7. Clear error buffer -> if fail, exit
8. Create handles based on XQueryTree
9. Release server grab
System test added to demonstrate that compiz should exit when another client has SubstructureRed
In order to make this work properly, some changes had to be made to compiz. This adds a new runtime switch --send-
(LP: #1085591)
Also fixed race condition caused by destroying the WMSnSelectionWindow before shutdown.
Because we haven't changed our active event mask
to remove SubstructureRed
compliant window managers may receive a DestroyNotify
(eg, because we're blocked on XSync) before we get
a chance to close our display connection and remove
our SubstructureRed
to fail to start.
The selection window is destroyed anyways when we
close our connection, and that is a very accurate
indicator to other WM's that we are well and truly
gone because the protocol requires the implementation
to remove all client event masks before destroying
windows.
Added acceptance-tests for testing that behaviour. They are not added to the default test target for obvious reasons.
Please throw this against the wall to see if any oddities have been introduced - they sometimes happen when you change the startup order as there is coupling between side effects.
- 3501. By Sam Spilsbury on 2012-12-03
-
Merge lp:compiz
- 3502. By Sam Spilsbury on 2012-12-03
-
Fix redundant thing
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:3502
http://
Executed test runs:
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Daniel van Vugt (vanvugt) wrote : | # |
I still get bug 1085591 even with this branch. Seems like nothing is fixed.
1. Start compiz
2. Start another compiz --replace
Expected: Compiz is replaced
Observed: The old compiz is killed but the new one fails to start with:
compiz (core) - Error: Another window manager is already running on screen: 0
| Sam Spilsbury (smspillaz) wrote : | # |
Hmm.
Well in any case this does fix the problematic nature of the code. We are
meant to check for errors upon taking SubstructureRed
don't do that properly.
I'm suprised its still not working for you, it definitely fixed the problem
here.
On Dec 4, 2012 11:44 AM, "Daniel van Vugt" <email address hidden>
wrote:
> Review: Needs Fixing
>
> I still get bug 1085591 even with this branch. Seems like nothing is fixed.
>
> 1. Start compiz
> 2. Start another compiz --replace
> Expected: Compiz is replaced
> Observed: The old compiz is killed but the new one fails to start with:
> compiz (core) - Error: Another window manager is already running on
> screen: 0
> --
>
> https:/
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>
| Sam Spilsbury (smspillaz) wrote : | # |
Oh, could you get some more output for me?
Change the DEBUG #define to be 1 and run compiz --debug.
That will print out X errors as they occurr.
On Dec 4, 2012 12:32 PM, "Sam Spilsbury" <email address hidden> wrote:
> Hmm.
>
> Well in any case this does fix the problematic nature of the code. We are
> meant to check for errors upon taking SubstructureRed
> don't do that properly.
>
> I'm suprised its still not working for you, it definitely fixed the
> problem here.
> On Dec 4, 2012 11:44 AM, "Daniel van Vugt" <email address hidden>
> wrote:
>
>> Review: Needs Fixing
>>
>> I still get bug 1085591 even with this branch. Seems like nothing is
>> fixed.
>>
>> 1. Start compiz
>> 2. Start another compiz --replace
>> Expected: Compiz is replaced
>> Observed: The old compiz is killed but the new one fails to start with:
>> compiz (core) - Error: Another window manager is already running on
>> screen: 0
>> --
>>
>> https:/
>> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>>
>
| Daniel van Vugt (vanvugt) wrote : | # |
Debug mode says...
./bin/compiz (core) - Info: Loading plugin: core
./bin/compiz (core) - Debug: Trying to load core from: /home/dan/
./bin/compiz (core) - Debug: dlopen failed: /home/dan/
./bin/compiz (core) - Debug: Trying to load core from: /home/dan/
./bin/compiz (core) - Debug: dlopen failed: /home/dan/
./bin/compiz (core) - Info: Starting plugin: core
./bin/compiz (core) - Debug: Started plugin: core
X Error of failed request: BadWindow (invalid Window parameter)
Major opcode of failed request: 34 (X_UngrabKey)
Minor opcode of failed request: 0
Resource id in failed request: 0x0
X Error of failed request: BadAccess (attempt to access private resource denied)
Major opcode of failed request: 2 (X_ChangeWindow
Minor opcode of failed request: 0
Resource id in failed request: 0xe1
X Error of failed request: BadAccess (attempt to access private resource denied)
Major opcode of failed request: 33 (X_GrabKey)
Minor opcode of failed request: 0
Resource id in failed request: 0xe1
... above error repeats several times ...
X Error of failed request: BadAccess (attempt to access private resource denied)
Major opcode of failed request: 2 (X_ChangeWindow
Minor opcode of failed request: 0
Resource id in failed request: 0xe1
./bin/compiz (core) - Error: Another window manager is already running on screen: 0
./bin/compiz (core) - Info: Stopping plugin: core
./bin/compiz (core) - Debug: Stopped plugin: core
./bin/compiz (core) - Info: Unloading plugin: core
- 3503. By Sam Spilsbury on 2012-12-04
-
Refactor the process class a bit - split it out
- 3504. By Sam Spilsbury on 2012-12-04
-
Make the message checks more generic
- 3505. By Sam Spilsbury on 2012-12-04
-
Do entry through a boost::function
- 3506. By Sam Spilsbury on 2012-12-05
-
Refactored the threading logic into AsyncThread
- 3507. By Sam Spilsbury on 2012-12-05
-
Move AsyncTask into its own separate static library
- 3508. By Sam Spilsbury on 2012-12-05
-
Added acceptance tests for --replace behaviour.
SIGINTClosesDown just tests that sending SIGINT closes
the running instanceReplaceOtherWMFast is a test for --replace behaviour to make
sure that the old one dies and the new one comes upReplaceOtherWMSlow slows down the old instance by continually
stopping and starting it, and might trigger race conditions
that prevent the new one from starting up properly.These tests are not run automatically as they are inherently
flakey and timing-based - 3509. By Sam Spilsbury on 2012-12-05
-
Keep the old compiz stopped for longer periods
- 3510. By Sam Spilsbury on 2012-12-05
-
Whitespace
- 3511. By Sam Spilsbury on 2012-12-05
-
Added README
- 3512. By Sam Spilsbury on 2012-12-05
-
Don't destroy wmSnSelectionWindow on shutdown.
Because we haven't changed our active event mask
to remove SubstructureRedirectMask, other ICCCM
compliant window managers may receive a DestroyNotify
(eg, because we're blocked on XSync) before we get
a chance to close our display connection and remove
our SubstructureRedirectMask. That will cause them
to fail to start.The selection window is destroyed anyways when we
close our connection, and that is a very accurate
indicator to other WM's that we are well and truly
gone because the protocol requires the implementation
to remove all client event masks before destroying
windows.Now ReplaceOtherWMSlow doesn't seem to fail at all
because "another WM was running"
| Sam Spilsbury (smspillaz) wrote : | # |
OK, thanks for the output.
I've fixed another race condition and added some acceptance tests to verify it (they are quite flakey due to being time-based so they aren't added to the test target by default)
- 3513. By Sam Spilsbury on 2012-12-05
-
Remove redundant function


PASSED: Continuous integration, rev:3501 jenkins. qa.ubuntu. com/job/ compiz- ci/287/ jenkins. qa.ubuntu. com/job/ compiz- ci/./build= pbuilder, distribution= quantal, flavor= amd64/287/ console
http://
Executed test runs:
SUCCESS: http://
Click here to trigger a rebuild: jenkins. qa.ubuntu. com/job/ compiz- ci/287/ /rebuild/?
http://