[Merge] ~kondor-dani/compiz:dbus-fix into compiz:master

[Daniel Kondor]

> I am looking at:
> http://www.cplusplus.com/reference/list/list/erase/
> http://www.cplusplus.com/reference/list/list/clear/
> 
> What exactly "which are destroyed" means? Sounds like it should be destroyed
> already!? Sorry, I don't know C++...

For list, it means that this will call the destructor of the object removed from the list, which is a Glib::RefPtr<CompWatchFd> in this case. The destructor should then free any memory used by the object. For a Glib::RefPtr, it will call g_object_unref I think:
https://developer.gnome.org/glibmm/stable/classGlib_1_1RefPtr.html#a54f4e0a8d9e9c43f24401b2f6b86a603

In my understanding, the problem is that calling the destructor for this object is not sufficient in this case; probably since the MainContext keeps a reference as well which explicitly needs to be removed with the destroy() call.

> 
> Check ~EventManager, it calls g_source_destroy for each fd. For consistently
> maybe g_source_destroy (w->gobj ()) is better?

As far as I can tell g_source_destroy(w->gobj()) and w->destroy() should be equivalent; the former is the C interface, the latter is the C++ interface from glibmm. I think both should work the same, it is a matter of preference.

> 
> Also did you see FIXME in CompWatchFd::internalCallback? Is that unrelated to
> your findings?

I was wondering about that. I think the problem there was how to remove from the list, but that is already done in EventManager::removeWatchFd(), but I'm not sure about this.

On the one hand, it should not be safe to destroy w by removing it from the list if it might be referenced in another thread at this time (or through a callback). On the other hand, since it is a reference counted object, if a reference is kept somewhere else, that might be OK. I'm unsure, but I assume that the MainContext is keeping a reference of all sources added to it and the destroy() call is removing these.

In this case, my fix could introduce a bug dependent on the race condition that a source is destroyed while it is executing. This is protected by the fact that if w->mExecuting is true, w->destroy() will not be called, but this source will be removed when the dispatch function returns false. I believe it is still OK to remove to source from the list here, since the main loop context keeps a reference to it.

Also, is it possible that EventManager::removeWatchFd() and CompWatchFd::internalCallback() run in different threads? I'm assuming not, since then mExecuting would need to be an atomic variable to be protected.



-- 
https://code.launchpad.net/~kondor-dani/compiz/+git/compiz/+merge/384366
Your team Compiz Maintainers is requested to review the proposed merge of ~kondor-dani/compiz:dbus-fix into compiz:master.