I think Ian just fixed this. Is this still a problem with the latest unstable release?

No I didn't touch anything related to this at all.

IIRC, Ubuntu people downstream (I can find the bug if you like) say that it's because Metacity's compositor grabs a selection which it fails to release; I thought I'd seen something of the sort in the changelog.

Yeah, i know why it happens, i just havent done anything about it.
In reality surely metacity needs to quit so that compiz can run, rather than just release the compositor selection?

How does it work if metacity is running without the compositor and compiz is started? Does metacity quit? If so, surely compiz should check for the presence of an already running window manager, rather than an already running compositor?

If Metacity is running and compiz is started, the behaviour should be (and AFAIK is) identical to what happens when any WM replaces any other WM.  The old WM owns the selection WM_Sn where n is the screen number.  The new WM will request this selection; the old WM is required at this point to give up the selection, put its house in order, and die.  Of course it could refuse to give up the selection, but this would be ungentlemanly and nobody does it.  There is a convention that, when a WM finds that it's the new WM in this situation, it will require its invoker to pass it a --replace flag to do this magic; otherwise it will quit.  I am reasonably sure that Compiz honours this convention.

That part works, compositor or no.

The problem here is alleged in https://bugs.launchpad.net/metacity/+bug/178953 to be that Metacity has some kind of difficulty giving up the _NEW_WM_CM_Sn selection, which is compositor-specific, rather than the generalised WM_Sn selection.  Some links are given on that page which explain it better than I really understand.

Yes, I had already read and understand the links, but my point is why compiz does not just try to replace metacity like it normally does? Surely it would make more sense for compiz to claim WM_Sn before checking for the CM selection, which would fix all these problems as the chances are that a compositor is also going to be the window manager.

I'm sorry, I didn't mean to be patronising.

I haven't tested it myself, but I got the idea, from what people on the launchpad bug were saying, that Metacity was refusing to give up _NEW_WM_CM_Sn.  Is that your reading of the launchpad bug, and is it in accord with the code?

yes, it is.
but i'm wondering why compiz doesnt just do what it normally does and request that metacity shuts down.

This seems to crash metacity in trunk (possibly not because of compositor issues at all); escalating.  Expect patch by tonight.

Created attachment 112182 [details] [review]
Check for compositor data being null

There seem to be two problems here.

== Problem one ==

There was one piece of code in the compositor which, when unmanaging windows, reassigned the active window, and updated the screen's compositor data to do so; but when the compositor had been shut down because it was being replaced by another compositor, this was null and so Metacity segfaulted (hence this morning's escalation).  While I was at it, I added some other defensive programming checks around the place; if any of them are excessive, Ian, feel free to remove or change them.

== Problem two ==

When you run Compiz to replace a compositing Metacity, it claims not to be able to gain the compositing manager selection; Compiz hangs at this point and Metacity carries on.  This is important, but not as important as a segfault, so this bug will be de-escalated.

De-escalated to High and fix for problem one checked in:
http://svn.gnome.org/viewvc/metacity?rev=3751&view=rev

Will probably need to go into stable when Ian's had a look.

they probably are excessive, but cant hurt although surely the braces should be missing from the 1 line if statements? Should be good for stable.

My name isn't Ian btw :)

I'm never really sure about braces around one-line if statements.  I mean, I know what they do, I'm never quite sure whether they're good style.  We should have a coding standards document or something.

I'll backport this soon, then.

I do apologise, I did know your name.  I blame lack of sleep this week!

And now to investigate problem two...

Well, I think braces look stupid around 1 liners we're using the if()\n{\n approach like Metacity is, they're fine in the if () { \n approach.

I just assumed it was metacity coding style anyway :)

Looks like this bug has been forgotten... This paradoxically prevents people from using composited Metacity, since when enabling this feature you can't switch to compiz without reverting the option. At least, no distribution is going to enable compositing by default because of that...

From reading through the metacity and compiz code, I think the problem is the use of CurrentTime in meta_screen_unset_cm_selection()

The sequence of events I think is:

 - Compiz acquires ownership of the window manager selection (using a timestamp it gets from a PropertyNotify event - like meta_display_get_current_time_roundtrip)

 - Metacity sees it losing the WM selection and sets the CM selection to None with a timestamp of CurrentTime, which is converted to a server timestamp

 - Compiz tries to acquire the CM selection with the same timestamp it used for the window manager timestamp. That timestamp is older than the current selection timestamp (from the unset), so setting the selection fails.

Section 2.3.1 of the ICCCM requires that when giving up ownership of a selection you use the timestamp you used to acquire the selection initially.

So one solution is:

 - Use meta_display_get_current_time_roundtrip() in 
   meta_screen_set_cm_selection() to get a real timestamp
 - Store that in the screen
 - Use it in meta_screen_unset_cm_selection()

(I haven't tested this, conceivable something else is going on.)

Created attachment 141537 [details] [review]
store timestamp for _NET_WM_CM_SX selection

This patch implements Owen's suggestion of storing a timestamp in MetaScreen and using it to get and release the selection. With this patch I can enable the metacity compositor and switch between metacity and compiz without any issues.

Sounds like a great patch to a problem of metacity not implementing protocol specification correctly - was this applied to some GNOME version that I can test on my system (where I have this problem)?

Review of attachment 141537 [details] [review]:

Patch looks good, applies cleanly, doesn't appear to break anything in testing, and seems to fix a problem in our implementation of the protocol at least.  I can't test whether it actually stops Compiz from running because Compiz won't start up for me on this computer, but I've checked it in because, even if it doesn't solve the presenting problem, it's clearly the right thing to do.

Great, at last! ;-)

This now works for me; closing as cleanup now I've noticed it.