Merge lp:~compiz-team/compiz/compiz.fix_1085590.1 into lp:compiz/0.9.9

[Sam Spilsbury]

In order to safely destroy a Glib::Source you need to g_source_destroy its
underlying gobj which will invoke the destructor for the Glib::Source.
Added an integration test to prove it.
(LP: #1085590)

In order to safely destroy a Glib::Source you need to g_source_destroy its underlying gobj
which will invoke the destructor for the Glib::Source.

Added an integration test to prove it

(LP: #1085590)

[lp:~compiz-team/compiz/compiz.fix_1085590.1]

3496. By Sam Spilsbury on 2012-12-02

Add missing files

3497. By Sam Spilsbury on 2012-12-02

Revert unneeded change

3498. By Sam Spilsbury on 2012-12-02

Fix omission

3499. By Sam Spilsbury on 2012-12-02

We already have the connnection number use the provided fd

3500. By Sam Spilsbury on 2012-12-02

Remove commented code

3501. By Sam Spilsbury on 2012-12-02

Check if the source exists before calling g_source_destroy

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3500
http://jenkins.qa.ubuntu.com/job/compiz-ci/283/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/283/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/283//rebuild/?

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3501
http://jenkins.qa.ubuntu.com/job/compiz-ci/285/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/285/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/285//rebuild/?

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

review: Approve

=== modified file 'src/eventsource.cpp'
--- src/eventsource.cpp	2012-05-10 12:58:01 +0000
+++ src/eventsource.cpp	2012-12-02 18:31:23 +0000
@@ -30,7 +30,8 @@
 CompEventSource *
 CompEventSource::create ()
 {
-    return new CompEventSource ();
+    return new CompEventSource (screen->dpy (),
+				ConnectionNumber (screen->dpy ()));
 }
 
 sigc::connection
@@ -39,10 +40,10 @@
     return connect_generic (slot);
 }
 
-CompEventSource::CompEventSource () :
+CompEventSource::CompEventSource (Display *dpy, int fd) :
     Glib::Source (),
-    mDpy (screen->dpy ()),
-    mConnectionFD (ConnectionNumber (screen->dpy ()))
+    mDpy (dpy),
+    mConnectionFD (fd)
 {
     mPollFD.set_fd (mConnectionFD);
     mPollFD.set_events (Glib::IO_IN);

=== modified file 'src/privateeventsource.h'
--- src/privateeventsource.h	2012-05-10 12:58:01 +0000
+++ src/privateeventsource.h	2012-12-02 18:31:23 +0000
@@ -50,7 +50,7 @@
 	bool dispatch (sigc::slot_base *slot);
 	bool callback ();
 
-	CompEventSource ();
+	CompEventSource (Display *dpy, int fd);
 
     private:
 

=== modified file 'src/privateiosource.h'
--- src/privateiosource.h	2012-03-12 12:47:04 +0000
+++ src/privateiosource.h	2012-12-02 18:31:23 +0000
@@ -26,6 +26,19 @@
 #ifndef _COMPIZ_PRIVATEIOSOURCE_H
 #define _COMPIZ_PRIVATEIOSOURCE_H
 
+#include <glibmm/main.h>
+
+typedef boost::function<void (short int)> FdWatchCallBack;
+typedef int CompWatchFdHandle;
+
+namespace compiz
+{
+    namespace private_screen
+    {
+	class EventManager;
+    }
+}
+
 class CompWatchFd :
     public Glib::IOSource
 {

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-11-13 07:56:49 +0000
+++ src/screen.cpp	2012-12-02 18:31:23 +0000
@@ -5221,14 +5221,27 @@
 
 cps::EventManager::~EventManager ()
 {
-    delete timeout;
+    /* Not guaranteed to be created by EventManager's constructor */
+    if (timeout)
+    {
+	/* This will implicitly call ~CompTimeoutSource
+	 * See LP: #1085590 */
+	g_source_destroy (timeout->gobj ());
+    }
     delete sigintSource;
     delete sigtermSource;
     delete sighupSource;
-    delete source;
-
+
+    /* Not guaranteed to be created by EventManager's constructor */
+    if (source)
+    {
+	/* This will implicitly call ~CompEventSource */
+	g_source_destroy (source->gobj ());
+    }
+
+    /* This will implicitly call ~CompWatchFd */
     foreach (CompWatchFd *fd, watchFds)
-	delete fd;
+	g_source_destroy (fd->gobj ());
 
     watchFds.clear ();
 }

=== modified file 'src/signalsource.cpp'
--- src/signalsource.cpp	2012-01-25 07:33:33 +0000
+++ src/signalsource.cpp	2012-12-02 18:31:23 +0000
@@ -65,6 +65,7 @@
 
 CompSignalSource::~CompSignalSource ()
 {
+    /* Do not remove, see LP: #1085590 */
     if (mSource)
 	g_source_remove (mSource);
 }

=== modified file 'src/timer/src/privatetimeoutsource.h'
--- src/timer/src/privatetimeoutsource.h	2012-01-20 17:50:31 +0000
+++ src/timer/src/privatetimeoutsource.h	2012-12-02 18:31:23 +0000
@@ -23,6 +23,7 @@
  * Authored by: Sam Spilsbury <[e-mail]>
  */
 
+#include <glibmm/main.h>
 #include <core/timer.h>
 #include <core/timeouthandler.h>
 

=== modified file 'tests/integration/CMakeLists.txt'
--- tests/integration/CMakeLists.txt	2012-01-20 16:29:27 +0000
+++ tests/integration/CMakeLists.txt	2012-12-02 18:31:23 +0000
@@ -1,1 +1,2 @@
 add_subdirectory (xig)
+add_subdirectory (glib)

=== added directory 'tests/integration/glib'
=== added file 'tests/integration/glib/CMakeLists.txt'
--- tests/integration/glib/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ tests/integration/glib/CMakeLists.txt	2012-12-02 18:31:23 +0000
@@ -0,0 +1,30 @@
+include (FindPkgConfig)
+
+pkg_check_modules (COMPIZ_GLIB_INTEGRATION_TEST glibmm-2.4)
+
+if (COMPIZ_GLIB_INTEGRATION_TEST_FOUND)
+
+    include_directories (${compiz_SOURCE_DIR}/src/)
+    include_directories (${compiz_SOURCE_DIR}/src/timer/include)
+    include_directories (${compiz_SOURCE_DIR}/src/timer/src)
+    include_directories (${compiz_SOURCE_DIR}/include)
+    include_directories (${COMPIZ_GLIB_INTEGRATION_TEST_INCLUDE_DIRS})
+
+    link_directories (${compiz_BINARY_DIR}/src)
+    link_directories (${COMPIZ_GLIB_INTEGRATION_TEST_LIBRARY_DIRS})
+
+    add_executable (compiz_glib_integration_test
+		    ${CMAKE_CURRENT_SOURCE_DIR}/glib_integration_test.cpp)
+
+    target_link_libraries (compiz_glib_integration_test
+		           compiz_core
+			   ${COMPIZ_GLIB_INTEGRATION_TEST_LIBRARIES}
+			   ${GTEST_BOTH_LIBRARIES}
+			   ${GMOCK_LIBRARY}
+			   ${GMOCK_MAIN_LIBRARY}
+			   ${CMAKE_THREAD_LIBS_INIT} # Link in pthread.
+			   )
+
+    compiz_discover_tests (compiz_glib_integration_test COVERAGE compiz_core)
+
+endif (COMPIZ_GLIB_INTEGRATION_TEST_FOUND)

=== added file 'tests/integration/glib/glib_integration_test.cpp'
--- tests/integration/glib/glib_integration_test.cpp	1970-01-01 00:00:00 +0000
+++ tests/integration/glib/glib_integration_test.cpp	2012-12-02 18:31:23 +0000
@@ -0,0 +1,152 @@
+/*
+ * Compiz GLib integration test
+ *
+ * Copyright (C) 2012 Sam Spilsbury <[e-mail]>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * Authored By:
+ * Sam Spilsbury <[e-mail]>
+ */
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include "privateeventsource.h"
+#include "privatetimeoutsource.h"
+#include "privateiosource.h"
+
+class DieVerifier
+{
+    public:
+
+	MOCK_METHOD0 (Die, void ());
+};
+
+class GLibSourceDestroyIntegration :
+    public ::testing::Test
+{
+    public:
+
+	GLibSourceDestroyIntegration () :
+	    context (Glib::MainContext::get_default ()),
+	    mainloop (Glib::MainLoop::create ())
+	{
+	}
+
+	DieVerifier                  die;
+	Glib::RefPtr <Glib::MainContext> context;
+	Glib::RefPtr <Glib::MainLoop>    mainloop;
+};
+
+class StubTimeoutSource :
+    public CompTimeoutSource
+{
+    public:
+
+	StubTimeoutSource (DieVerifier *dieVerifier,
+			   Glib::RefPtr <Glib::MainContext> &context) :
+	    CompTimeoutSource (context),
+	    mDie (dieVerifier)
+	{
+	}
+
+	virtual ~StubTimeoutSource ()
+	{
+	    mDie->Die ();
+	}
+
+    private:
+
+	DieVerifier *mDie;
+};
+
+class StubEventSource :
+    public CompEventSource
+{
+    public:
+
+	StubEventSource (DieVerifier *dieVerifier) :
+	    CompEventSource (NULL, 0),
+	    mDie (dieVerifier)
+	{
+	}
+
+	virtual ~StubEventSource ()
+	{
+	    mDie->Die ();
+	}
+
+    private:
+
+	DieVerifier *mDie;
+};
+
+class StubWatchFd :
+    public CompWatchFd
+{
+    public:
+
+	StubWatchFd (DieVerifier *dieVerifier,
+		     int fd,
+		     Glib::IOCondition iocond,
+		     const FdWatchCallBack &cb) :
+	    CompWatchFd (fd, iocond, cb),
+	    mDie (dieVerifier)
+	{
+	}
+
+	virtual ~StubWatchFd ()
+	{
+	    mDie->Die ();
+	}
+
+    private:
+
+	DieVerifier *mDie;
+};
+
+TEST_F (GLibSourceDestroyIntegration, TimeoutSourceGSourceDestroy)
+{
+    StubTimeoutSource *sts = new StubTimeoutSource (&die, context);
+
+    EXPECT_CALL (die, Die ());
+
+    g_source_destroy (sts->gobj ());
+}
+
+TEST_F (GLibSourceDestroyIntegration, EventSourceGSourceDestroy)
+{
+    StubEventSource *sts = new StubEventSource (&die);
+
+    sts->attach (context);
+
+    EXPECT_CALL (die, Die ());
+
+    g_source_destroy (sts->gobj ());
+}
+
+TEST_F (GLibSourceDestroyIntegration, FdSourceGSourceDestroy)
+{
+    Glib::IOCondition iocond = Glib::IO_IN;
+    int fd = 0;
+    FdWatchCallBack cb;
+    StubWatchFd *sts = new StubWatchFd (&die, fd, iocond, cb);
+
+    sts->attach (context);
+
+    EXPECT_CALL (die, Die ());
+
+    g_source_destroy (sts->gobj ());
+}