Merge lp:~smspillaz/compiz-core/compiz-core.glib-mainloop-fixes into lp:compiz-core/0.9.5

[Sam Spilsbury]

Fix some of the GLib mainloop implementation in compiz

1. Don't use Glib::RefPtr for sources as they are broken (unref will cause a crash on a destroyed source)
2. Don't g_main_context_iteration in order to flush the event queue
3. Use XFlush instead
4. Handle signals using glib's signal sources so that they are safely dispatched back to the mainloop
5. Enabled the failing xig tests because of ^ (they will pass when https://code.launchpad.net/~smspillaz/xig/xig.disconnect-signals/+merge/89535 is merged)

[Alan Griffiths (alan-griffiths)]

> 1. Don't use Glib::RefPtr for sources as they are broken (unref will cause a crash on a destroyed source)

I'm slightly puzzled as it appears the prior code already uses raw pointers. (And if Glib::RefPtr is broken how do other uses work?)

review: Approve

[Sam Spilsbury (smspillaz)]

> > 1. Don't use Glib::RefPtr for sources as they are broken (unref will cause a
> crash on a destroyed source)
>
> I'm slightly puzzled as it appears the prior code already uses raw pointers.

Yes, only CompEventSource * did

> (And if Glib::RefPtr is broken how do other uses work?)

Its only broken for GSource * wrappers.

Eg, the underlying GSource * is free'd in the main loop, and glibmm doesn't catch the GDestroyNotify callback for when that happens so when the pointer goes out of scope, it attempts to free the underlying GSource * and this crashes

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2012-01-20 15:42:52 +0000
+++ src/CMakeLists.txt	2012-01-21 13:43:23 +0000
@@ -99,6 +99,7 @@
     ${CMAKE_CURRENT_SOURCE_DIR}/modifierhandler.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/propertywriter.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/eventsource.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/signalsource.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/stackdebugger.cpp
     
     ${_bcop_sources}

=== modified file 'src/eventsource.cpp'
--- src/eventsource.cpp	2011-08-19 14:25:11 +0000
+++ src/eventsource.cpp	2012-01-21 13:43:23 +0000
@@ -26,11 +26,9 @@
 
 #include "privatescreen.h"
 
-//Glib::RefPtr <CompEventSource>
-CompEventSource*
+CompEventSource *
 CompEventSource::create ()
 {
-  // return Glib::RefPtr <CompEventSource> (new CompEventSource ());
     return new CompEventSource ();
 }
 
@@ -62,13 +60,7 @@
 bool
 CompEventSource::callback ()
 {
-    if (restartSignal || shutDown)
-    {
-	screen->priv->mainloop->quit ();
-	return false;
-    }
-    else
-	screen->priv->processEvents ();
+    screen->priv->processEvents ();
     return true;
 }
 

=== modified file 'src/main.cpp'
--- src/main.cpp	2012-01-19 18:12:31 +0000
+++ src/main.cpp	2012-01-21 13:43:23 +0000
@@ -57,7 +57,7 @@
 }
 
 static void
-signalHandler (int sig)
+chldSignalHandler (int sig)
 {
     int status;
 
@@ -65,14 +65,6 @@
     case SIGCHLD:
 	waitpid (-1, &status, WNOHANG | WUNTRACED);
 	break;
-    case SIGHUP:
-	restartSignal = true;
-	break;
-    case SIGINT:
-    case SIGTERM:
-	shutDown = true;
-    default:
-	break;
     }
 }
 
@@ -248,10 +240,7 @@
     programArgc = argc;
     programArgv = argv;
 
-    signal (SIGHUP, signalHandler);
-    signal (SIGCHLD, signalHandler);
-    signal (SIGINT, signalHandler);
-    signal (SIGTERM, signalHandler);
+    signal (SIGCHLD, chldSignalHandler);
 
     if (!manager.parseArguments (argc, argv))
 	return 0;

=== modified file 'src/privateeventsource.h'
--- src/privateeventsource.h	2011-08-19 14:25:11 +0000
+++ src/privateeventsource.h	2012-01-21 13:43:23 +0000
@@ -31,9 +31,11 @@
 {
     public:
 
+	virtual ~CompEventSource ();
+
 	static
-        // Glib::RefPtr <CompEventSource> create ();
-	CompEventSource* create ();
+	CompEventSource *
+	create ();
 
 	sigc::connection connect (const sigc::slot <bool> &slot);
 
@@ -45,7 +47,6 @@
 	bool callback ();
 
 	CompEventSource ();
-	virtual ~CompEventSource ();
 
     private:
 

=== modified file 'src/privateiosource.h'
--- src/privateiosource.h	2011-07-04 16:25:20 +0000
+++ src/privateiosource.h	2012-01-21 13:43:23 +0000
@@ -31,10 +31,12 @@
 {
     public:
 
+	virtual ~CompWatchFd ();
+
 	static
-	Glib::RefPtr <CompWatchFd> create (int,
-					   Glib::IOCondition,
-					   FdWatchCallBack);
+	CompWatchFd * create (int,
+			      Glib::IOCondition,
+			      FdWatchCallBack);
 
     protected:
 

=== modified file 'src/privatescreen.h'
--- src/privatescreen.h	2012-01-19 18:12:31 +0000
+++ src/privatescreen.h	2012-01-21 13:43:23 +0000
@@ -41,6 +41,7 @@
 #include "privatetimeoutsource.h"
 #include "privateiosource.h"
 #include "privateeventsource.h"
+#include "privatesignalsource.h"
 
 #include "core_options.h"
 
@@ -382,21 +383,28 @@
 	
 	void setDefaultWindowAttributes (XWindowAttributes *);
 
+	void handleSignal (int signum);
+
     public:
 
 	PrivateScreen *priv;
 
 	Glib::RefPtr <Glib::MainLoop>  mainloop;
-	// See https://bugzilla.gnome.org/show_bug.cgi?id=561885
-	// Glib::RefPtr <CompEventSource> source;
-	CompEventSource* source;
-	Glib::RefPtr <CompTimeoutSource> timeout;
+
+	/* We cannot use RefPtrs. See
+	 * https://bugzilla.gnome.org/show_bug.cgi?id=561885
+	 */
+	CompEventSource * source;
+	CompTimeoutSource * timeout;
+	CompSignalSource * sighupSource;
+	CompSignalSource * sigtermSource;
+	CompSignalSource * sigintSource;
 	Glib::RefPtr <Glib::MainContext> ctx;
 
 	CompFileWatchList   fileWatch;
 	CompFileWatchHandle lastFileWatchHandle;
 
-	std::list<Glib::RefPtr <CompWatchFd> > watchFds;
+	std::list< CompWatchFd * > watchFds;
 	CompWatchFdHandle        lastWatchFdHandle;
 
 	std::map<CompString, CompPrivate> valueMap;

=== added file 'src/privatesignalsource.h'
--- src/privatesignalsource.h	1970-01-01 00:00:00 +0000
+++ src/privatesignalsource.h	2012-01-21 13:43:23 +0000
@@ -0,0 +1,55 @@
+/*
+ * Copyright © 2012 Canonical, Ltd.
+ *
+ * Permission to use, copy, modify, distribute, and sell this software
+ * and its documentation for any purpose is hereby granted without
+ * fee, provided that the above copyright notice appear in all copies
+ * and that both that copyright notice and this permission notice
+ * appear in supporting documentation, and that the name of
+ * Canonical, Ltd. not be used in advertising or publicity pertaining to
+ * distribution of the software without specific, written prior permission.
+ * Canonical, Ltd. makes no representations about the suitability of this
+ * software for any purpose. It is provided "as is" without express or
+ * implied warranty.
+ *
+ * CANONICAL LTD. DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
+ * NO EVENT SHALL Canonical, Ltd. BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+ * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ *
+ * Author: Sam Spilsbury <[e-mail]>
+ */
+
+#include <boost/function.hpp>
+
+#include <glib.h>
+
+class CompSignalSource
+{
+    public:
+
+	~CompSignalSource ();
+
+	typedef boost::function <void (int)> callbackFunc;
+
+	static
+	CompSignalSource *
+	create (int signum,
+		const callbackFunc &);
+
+    protected:
+
+	explicit CompSignalSource (int signum, const callbackFunc &);
+
+    private:
+
+	static gboolean callback (gpointer user_data);
+	static void     destroyed (gpointer user_data);
+
+	callbackFunc mFunc;
+	int          mSignal;
+	gint         mSource;
+};

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-01-20 12:59:05 +0000
+++ src/screen.cpp	2012-01-21 13:43:23 +0000
@@ -112,17 +112,34 @@
 }
 
 void
+PrivateScreen::handleSignal (int signum)
+{
+    switch (signum)
+    {
+	case SIGINT:
+	case SIGTERM:
+	    shutDown = true;
+	    break;
+	case SIGHUP:
+	    restartSignal = true;
+	    break;
+	default:
+	    break;
+    }
+
+    if (shutDown || restartSignal)
+	mainloop->quit ();
+}
+
+void
 CompScreen::eventLoop ()
 {
-    priv->ctx = Glib::MainContext::get_default ();
-    priv->mainloop = Glib::MainLoop::create (priv->ctx, false);
     priv->source = CompEventSource::create ();
     priv->timeout = CompTimeoutSource::create (priv->ctx);
 
     priv->source->attach (priv->ctx);
 
-    /* Kick the event loop */
-    priv->ctx->iteration (false);
+    XFlush (priv->dpy);
 
     priv->mainloop->run ();
 }
@@ -191,12 +208,16 @@
 	     (this, &CompWatchFd::internalCallback));
 }
 
-Glib::RefPtr <CompWatchFd>
+CompWatchFd::~CompWatchFd ()
+{
+}
+
+CompWatchFd *
 CompWatchFd::create (int               fd,
 		     Glib::IOCondition events,
 		     FdWatchCallBack   callback)
 {
-    return Glib::RefPtr <CompWatchFd> (new CompWatchFd (fd, events, callback));
+    return new CompWatchFd (fd, events, callback);
 }
 
 CompWatchFdHandle
@@ -219,7 +240,7 @@
     if (events & POLLHUP)
 	gEvents |= Glib::IO_HUP;
 
-    Glib::RefPtr <CompWatchFd> watchFd = CompWatchFd::create (fd, gEvents, callBack);
+    CompWatchFd *watchFd = CompWatchFd::create (fd, gEvents, callBack);
 
     watchFd->attach (priv->ctx);
 
@@ -238,8 +259,8 @@
 void
 CompScreen::removeWatchFd (CompWatchFdHandle handle)
 {
-    std::list<Glib::RefPtr <CompWatchFd> >::iterator it;
-    Glib::RefPtr <CompWatchFd>	       w;
+    std::list<CompWatchFd * >::iterator it;
+    CompWatchFd *			w;
 
     for (it = priv->watchFds.begin();
 	 it != priv->watchFds.end (); it++)
@@ -259,7 +280,7 @@
 	return;
     }
 
-    w.reset ();
+    delete w;
     priv->watchFds.erase (it);
 }
 
@@ -4363,6 +4384,12 @@
     int                  nvisinfo;
     XSetWindowAttributes attrib;
 
+    priv->ctx = Glib::MainContext::get_default ();
+    priv->mainloop = Glib::MainLoop::create (priv->ctx, false);
+    priv->sighupSource = CompSignalSource::create (SIGHUP, boost::bind (&PrivateScreen::handleSignal, priv, _1));
+    priv->sigintSource = CompSignalSource::create (SIGINT, boost::bind (&PrivateScreen::handleSignal, priv, _1));
+    priv->sigtermSource = CompSignalSource::create (SIGTERM, boost::bind (&PrivateScreen::handleSignal, priv, _1));
+
     dpy = priv->dpy = XOpenDisplay (name);
     if (!priv->dpy)
     {
@@ -4371,6 +4398,8 @@
 	return false;
     }
 
+    XSynchronize (dpy, TRUE);
+
 //    priv->connection = XGetXCBConnection (priv->dpy);
 
     snprintf (priv->displayString, 255, "DISPLAY=%s",
@@ -4932,4 +4961,11 @@
 
 PrivateScreen::~PrivateScreen ()
 {
+    delete timeout;
+    delete source;
+
+    foreach (CompWatchFd *fd, watchFds)
+	delete fd;
+
+    watchFds.clear ();
 }

=== added file 'src/signalsource.cpp'
--- src/signalsource.cpp	1970-01-01 00:00:00 +0000
+++ src/signalsource.cpp	2012-01-21 13:43:23 +0000
@@ -0,0 +1,65 @@
+/*
+ * Copyright © 2012 Canonical, Ltd.
+ *
+ * Permission to use, copy, modify, distribute, and sell this software
+ * and its documentation for any purpose is hereby granted without
+ * fee, provided that the above copyright notice appear in all copies
+ * and that both that copyright notice and this permission notice
+ * appear in supporting documentation, and that the name of
+ * Canonical, Ltd. not be used in advertising or publicity pertaining to
+ * distribution of the software without specific, written prior permission.
+ * Canonical, Ltd. makes no representations about the suitability of this
+ * software for any purpose. It is provided "as is" without express or
+ * implied warranty.
+ *
+ * CANONICAL LTD. DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
+ * NO EVENT SHALL Canonical, Ltd. BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+ * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ *
+ * Author: Sam Spilsbury <[e-mail]>
+ */
+
+#include "privatesignalsource.h"
+#include <glib-unix.h>
+
+CompSignalSource *
+CompSignalSource::create (int signum, const callbackFunc &f)
+{
+    return new CompSignalSource (signum, f);
+}
+
+CompSignalSource::CompSignalSource (int signum, const callbackFunc &f) :
+    mFunc (f),
+    mSignal (signum),
+    mSource (g_unix_signal_add_full (G_PRIORITY_HIGH,
+				     signum,
+				     CompSignalSource::callback,
+				     this,
+				     CompSignalSource::destroyed))
+{
+}
+
+gboolean
+CompSignalSource::callback (gpointer user_data)
+{
+    CompSignalSource *self = static_cast <CompSignalSource *> (user_data);
+    self->mFunc (self->mSignal);
+    return TRUE;
+}
+
+void
+CompSignalSource::destroyed (gpointer user_data)
+{
+    CompSignalSource *self = static_cast <CompSignalSource *> (user_data);
+    self->mSource = 0;
+}
+
+CompSignalSource::~CompSignalSource ()
+{
+    if (mSource)
+	g_source_remove (mSource);
+}

=== modified file 'src/timer/src/privatetimeoutsource.h'
--- src/timer/src/privatetimeoutsource.h	2012-01-12 06:48:58 +0000
+++ src/timer/src/privatetimeoutsource.h	2012-01-21 13:43:23 +0000
@@ -34,7 +34,9 @@
 {
     public:
 
-	static Glib::RefPtr <CompTimeoutSource> create  (Glib::RefPtr <Glib::MainContext> &ctx);
+	virtual ~CompTimeoutSource ();
+
+	static CompTimeoutSource * create  (Glib::RefPtr <Glib::MainContext> &ctx);
 	sigc::connection connect (const sigc::slot <bool> &slot);
 
     protected:
@@ -45,7 +47,6 @@
 	bool callback ();
 
 	explicit CompTimeoutSource (Glib::RefPtr <Glib::MainContext> &ctx);
-	virtual ~CompTimeoutSource ();
 
     friend class CompTimer;
 };

=== modified file 'src/timer/src/timer.cpp'
--- src/timer/src/timer.cpp	2012-01-19 18:12:31 +0000
+++ src/timer/src/timer.cpp	2012-01-21 13:43:23 +0000
@@ -69,10 +69,10 @@
     return connect_generic (slot);
 }
 
-Glib::RefPtr <CompTimeoutSource>
+CompTimeoutSource *
 CompTimeoutSource::create (Glib::RefPtr <Glib::MainContext> &ctx)
 {
-    return Glib::RefPtr <CompTimeoutSource> (new CompTimeoutSource (ctx));
+    return new CompTimeoutSource (ctx);
 }
 
 #define COMPIZ_TIMEOUT_WAIT 15

=== modified file 'tests/integration/xig/CMakeLists.txt'
--- tests/integration/xig/CMakeLists.txt	2012-01-20 16:29:27 +0000
+++ tests/integration/xig/CMakeLists.txt	2012-01-21 13:43:23 +0000
@@ -10,21 +10,20 @@
 
 	add_subdirectory (src)
 
-	#add_test (compiz-xig-test-startup
-	#	  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner startup)
-
-	# Disabled as compiz is crashing on SIGHUP
-	#add_test (compiz-xig-test-restart
-	#	  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner restart)
-
-	#add_test (compiz-xig-test-xserver-quit
-	#	  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner xserver-quit)
-
-	#add_test (compiz-xig-test-new-window
-	#	  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner new-window)
-
-	#add_test (compiz-xig-test-existing-window
-	#	  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner existing-window)
+	add_test (compiz-xig-test-startup
+		  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner startup)
+
+	add_test (compiz-xig-test-restart
+		  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner restart)
+
+	add_test (compiz-xig-test-xserver-quit
+		  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner xserver-quit)
+
+	add_test (compiz-xig-test-new-window
+		  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner new-window)
+
+	add_test (compiz-xig-test-existing-window
+		  ${CMAKE_CURRENT_BINARY_DIR}/src/compiz-xig-test-runner existing-window)
 
 else (COMPIZ_XIG_TEST_FOUND)
 

=== modified file 'tests/integration/xig/scripts/restart.conf'
--- tests/integration/xig/scripts/restart.conf	2012-01-20 16:29:27 +0000
+++ tests/integration/xig/scripts/restart.conf	2012-01-21 13:43:23 +0000
@@ -10,9 +10,9 @@
 
 # Restart
 #?*RESTART-COMPIZ
-#?COMPIZ EXIT STATUS=1
 #?X CLIENT-DISCONNECTED
 #?X CLIENT-CONNECTED
 
 #?*STOP-COMPIZ
+#?X CLIENT-DISCONNECTED
 #?COMPIZ EXIT STATUS=0