glib: stricter ref counting

Following the boost::instrusive_ptr example and making "add_ref =
true" the default in our CXX GLib and GObject wrappers led to some
memory leaks because it didn't enforce thinking about whether the
plain pointer is already owned by us or not.

It is better to use a mandatory enum value, ADD_REF and TRANSFER_REF,
and force explicit construction. Doing that revealed that the
assignment operator was implemented as constructing a CXX instance
with increased ref count and/or that in some places, a real leak was
caused by increasing the ref count unnecessarily.

Running under valgrind gave a false sense of security. Some of the
real leaks only showed up randomly in tests.

4 files changed, 27 insertions, 14 deletions

diff --git a/src/syncevo/GLibSupport.cpp b/src/syncevo/GLibSupport.cpp
index af8ecfaf..6dc45868 100644
--- a/src/syncevo/GLibSupport.cpp
+++ b/src/syncevo/GLibSupport.cpp
@@ -191,9 +191,9 @@ GLibNotify::GLibNotify(const char *file,
                        const callback_t &callback) :
     m_callback(callback)
 {
-    GFileCXX filecxx(g_file_new_for_path(file));
+    GFileCXX filecxx(g_file_new_for_path(file), TRANSFER_REF);
     GErrorCXX gerror;
-    GFileMonitorCXX monitor(g_file_monitor_file(filecxx.get(), G_FILE_MONITOR_NONE, NULL, gerror));
+    GFileMonitorCXX monitor(g_file_monitor_file(filecxx.get(), G_FILE_MONITOR_NONE, NULL, gerror), TRANSFER_REF);
     m_monitor.swap(monitor);
     if (!m_monitor) {
         gerror.throwError(std::string("monitoring ") + file);
@@ -240,7 +240,7 @@ class GLibTest : public CppUnit::TestFixture {
         list<Event> events;
         static const char *name = "GLibTest.out";
         unlink(name);
-        GMainLoopCXX loop(g_main_loop_new(NULL, FALSE), false);
+        GMainLoopCXX loop(g_main_loop_new(NULL, FALSE), TRANSFER_REF);
         if (!loop) {
             SE_THROW("could not allocate main loop");
         }
diff --git a/src/syncevo/GLibSupport.h b/src/syncevo/GLibSupport.h
index f6224192..36a431f5 100644
--- a/src/syncevo/GLibSupport.h
+++ b/src/syncevo/GLibSupport.h
@@ -172,6 +172,19 @@ template<class A1, class A2, class A3, class A4, class A5, class A6, class A7, c
     }
 };
 
+enum RefOwnership
+{
+    TRANSFER_REF = false, /**<
+                           * Create new smart pointer which steals an existing reference without
+                           * increasing the reference count of the object.
+                           */
+    ADD_REF = true        /**<
+                           * Create new smart pointer which increases the reference count when
+                           * storing the pointer to the object.
+                           */
+};
+
+
 template<class C> class TrackGObject : public boost::intrusive_ptr<C> {
     typedef boost::intrusive_ptr<C> Base_t;
 
@@ -186,14 +199,14 @@ template<class C> class TrackGObject : public boost::intrusive_ptr<C> {
     }
 
  public:
-    TrackGObject(C *ptr, bool add_ref = true) : Base_t(ptr, add_ref) {}
+    TrackGObject(C *ptr, RefOwnership ownership) : Base_t(ptr, (bool)ownership) {}
     TrackGObject() {}
     TrackGObject(const TrackGObject &other) : Base_t(other) {}
     operator C * () const { return Base_t::get(); }
     operator bool () const { return Base_t::get() != NULL; }
     C * ref() const { return static_cast<C *>(g_object_ref(Base_t::get())); }
 
-    static  TrackGObject steal(C *ptr) { return TrackGObject(ptr, false); }
+    static  TrackGObject steal(C *ptr) { return TrackGObject(ptr, TRANSFER_REF); }
 
     template<class S> guint connectSignal(const char *signal,
                                           const boost::function<S> &callback) {
@@ -211,7 +224,7 @@ template<class C> class TrackGObject : public boost::intrusive_ptr<C> {
 
 template<class C> class StealGObject : public TrackGObject<C> {
  public:
-    StealGObject(C *ptr) : TrackGObject<C>(ptr, false) {}
+    StealGObject(C *ptr) : TrackGObject<C>(ptr, TRANSFER_REF) {}
     StealGObject() {}
     StealGObject(const StealGObject &other) : TrackGObject<C>(other) {}
 };
@@ -220,19 +233,19 @@ template<class C> class TrackGLib : public boost::intrusive_ptr<C> {
     typedef boost::intrusive_ptr<C> Base_t;
 
  public:
-    TrackGLib(C *ptr, bool add_ref = true) : Base_t(ptr, add_ref) {}
+ TrackGLib(C *ptr, RefOwnership ownership) : Base_t(ptr, (bool)ownership) {}
     TrackGLib() {}
     TrackGLib(const TrackGLib &other) : Base_t(other) {}
     operator C * () const { return Base_t::get(); }
     operator bool () const { return Base_t::get() != NULL; }
     C * ref() const { return static_cast<C *>(g_object_ref(Base_t::get())); }
 
-    static  TrackGLib steal(C *ptr) { return TrackGLib(ptr, false); }
+    static  TrackGLib steal(C *ptr) { return TrackGLib(ptr, TRANSFER_REF); }
 };
 
 template<class C> class StealGLib : public TrackGLib<C> {
  public:
-    StealGLib(C *ptr) : TrackGLib<C>(ptr, false) {}
+    StealGLib(C *ptr) : TrackGLib<C>(ptr, TRANSFER_REF) {}
     StealGLib() {}
     StealGLib(const StealGLib &other) : TrackGLib<C>(other) {}
 };
diff --git a/src/syncevo/GeeSupport.h b/src/syncevo/GeeSupport.h
index 08cda36d..cfbe5cd3 100644
--- a/src/syncevo/GeeSupport.h
+++ b/src/syncevo/GeeSupport.h
@@ -89,7 +89,7 @@ template<class Entry> class GeeCollCXX
 
  public:
     template<class Collection> GeeCollCXX(Collection *collection) :
-        m_collection(GEE_ITERABLE(collection))
+        m_collection(GEE_ITERABLE(collection), ADD_REF)
     {}
 
     class Iterator
@@ -110,7 +110,7 @@ template<class Entry> class GeeCollCXX
          * Takes ownership of iterator, which may be NULL for the end Iterator.
          */
         Iterator(GeeIterator *iterator) :
-            m_it(iterator, false),
+            m_it(iterator, TRANSFER_REF),
             m_valid(false)
         {}
 
@@ -181,7 +181,7 @@ template<class Key, class Value> class GeeMapEntryWrapper  {
 
     /** take ownership of entry instance */
     GeeMapEntryWrapper(GeeMapEntry *entry = NULL) :
-        m_entry(entry, false)
+        m_entry(entry, TRANSFER_REF)
     {}
     GeeMapEntryWrapper(const GeeMapEntryWrapper &other):
         m_entry(other.m_entry)
diff --git a/src/syncevo/LocalTransportAgent.cpp b/src/syncevo/LocalTransportAgent.cpp
index 179c0ea9..8b56542c 100644
--- a/src/syncevo/LocalTransportAgent.cpp
+++ b/src/syncevo/LocalTransportAgent.cpp
@@ -56,8 +56,8 @@ LocalTransportAgent::LocalTransportAgent(SyncContext *server,
     m_clientContext(SyncConfig::normalizeConfigString(clientContext)),
     m_status(INACTIVE),
     m_loop(loop ?
-           GMainLoopCXX(static_cast<GMainLoop *>(loop)) /* increase reference */ :
-           GMainLoopCXX(g_main_loop_new(NULL, false), false) /* use reference handed to us by _new */)
+           GMainLoopCXX(static_cast<GMainLoop *>(loop), ADD_REF) :
+           GMainLoopCXX(g_main_loop_new(NULL, false), TRANSFER_REF))
 {
 }