Index: gobject/pygsource.c
===================================================================
--- gobject/pygsource.c	(revision 755)
+++ gobject/pygsource.c	(working copy)
@@ -2,8 +2,10 @@
  * pygtk- Python bindings for the GTK toolkit.
  * Copyright (C) 1998-2003  James Henstridge
  * Copyright (C) 2005       Oracle
+ * Copyright (C) 2008       Bryan Silverthorn
  *
  * Author: Manish Singh <manish.singh@oracle.com>
+ * Author: Bryan Silverthorn <bcs26@cornell.edu>
  *
  *   pygsource.c: GSource wrapper
  *
@@ -23,6 +25,31 @@
  * USA
  */
 
+/* A few notes on GSource memory management:
+ *
+ * 1. On construction: incref wrapper unconditionally; it may
+ *    become attached in the future, not necessarily via the wrapper.
+ * 2. On destroy:
+ *    2.1 Decref the wrapper unconditionally.
+ *    2.2 Note that the wrapper is no longer refed by the wrapped GSource.
+ * 3. On visit:
+ *    3.1. If source refcount is one, no finalize method is present,
+ *       and we're refed by the wrapped GSource, visit self.
+ *    3.2. If finalize method is present, do nothing; it's uncollectable.
+ * 4. On clear: clear as expected, decref source.
+ * 5. On dealloc:
+ *    5.1. Clear weak refs. (Must be done _first_.)
+ *    5.2. Py_INCREF the wrapper to avoid double-decref in finalization.
+ *       (Incrementing the wrapper's refcount also breaks weak refs.)
+ *    5.3. Clear and free the wrapper as expected.
+ * 6. On finalize: if wrapper still exists, call finalize, if present.
+ *
+ * Be aware that these rules _still_ do not handle every case correctly.
+ * For example, a source callback might reference the context to which
+ * the source is attached; this cycle will permanently prevent both
+ * source and context from being collected.
+ */
+
 #ifdef HAVE_CONFIG_H
 #  include <config.h>
 #endif
@@ -31,21 +58,13 @@
 #include "pythread.h"
 #include <structmember.h>
 
-
-#define CHECK_DESTROYED(self, ret)			G_STMT_START {	\
-    if ((self)->source == NULL) {					\
-	PyErr_SetString(PyExc_RuntimeError, "source is destroyed");	\
-	return (ret);							\
-    }									\
-} G_STMT_END
-
-
 typedef struct {
     PyObject_HEAD
     GSource *source;
     PyObject *inst_dict;
     PyObject *weakreflist;
     gboolean python_source;
+    gboolean refed_by_gsource;
 } PyGSource;
 
 typedef struct
@@ -54,18 +73,39 @@
     PyObject *obj;
 } PyGRealSource;
 
+static gboolean
+set_error_if_destroyed(PyGSource *self)
+{
+    if (!self->source)
+    {
+        PyErr_SetString(PyExc_ValueError, "operation on deallocated source");
+
+        return TRUE;
+    }
+    else if (g_source_is_destroyed(self->source))
+    {
+        PyErr_SetString(PyExc_ValueError, "operation on destroyed source");
+
+        return TRUE;
+    }
+    else
+        return FALSE;
+}
+
 static PyObject *
 source_repr(PyGSource *self, const char *type)
 {
     gchar buf[256], *desc;
- 
-    if (self->source) {
-	if (g_source_get_context(self->source))
-	    desc = "attached";
-	else
-	    desc = "unattached";
+
+    if (!self) {
+        desc = "deallocated";
+    } else if (g_source_is_destroyed(self->source)) {
+        desc = "destroyed";
     } else {
-	desc = "destroyed";
+        if (g_source_get_context(self->source))
+            desc = "attached";
+        else
+            desc = "unattached";
     }
 
     if (type)
@@ -94,30 +134,27 @@
     if (py_context)
 	context = py_context->context;
 
-    CHECK_DESTROYED(self, NULL);
+    if (set_error_if_destroyed(self))
+        return NULL;
 
-    if (self->python_source) {
-	PyGRealSource *pysource = (PyGRealSource *)self->source;
-	Py_INCREF(pysource->obj);
-    }
+    id = g_source_attach(self->source, context);
 
-    id = g_source_attach(self->source, context);
     return PyInt_FromLong(id);
 }
 
 static PyObject *
 pyg_source_destroy(PyGSource *self)
 {
-    CHECK_DESTROYED(self, NULL);
+    if (set_error_if_destroyed(self))
+        return NULL;
 
-    if (self->python_source && self->source->context) {
-	PyGRealSource *pysource = (PyGRealSource *)self->source;
-	Py_DECREF(pysource->obj);
-    }
-
     g_source_destroy(self->source);
-    self->source = NULL;
 
+    // we need to know whether the wrapper is refed by the wrapped
+    self->refed_by_gsource = FALSE;
+
+    Py_DECREF(self);
+
     Py_INCREF(Py_None);
     return Py_None;
 }
@@ -128,7 +165,8 @@
     PyObject *first, *callback, *cbargs = NULL, *data;
     gint len;
 
-    CHECK_DESTROYED(self, NULL);
+    if (set_error_if_destroyed(self))
+        return NULL;
 
     len = PyTuple_Size (args);
     if (len < 1) {
@@ -169,7 +207,8 @@
 {
     GMainContext *context;
 
-    CHECK_DESTROYED(self, NULL);
+    if (set_error_if_destroyed(self))
+        return NULL;
 
     context = g_source_get_context(self->source);
 
@@ -199,7 +238,8 @@
 				     &PyGPollFD_Type, &fd))
 	return NULL;
 
-    CHECK_DESTROYED(self, NULL);
+    if(set_error_if_destroyed(self))
+        return NULL;
 
     g_source_add_poll(self->source, &fd->pollfd);
 
@@ -225,7 +265,8 @@
 				     &PyGPollFD_Type, &fd))
 	return NULL;
 
-    CHECK_DESTROYED(self, NULL);
+    if (set_error_if_destroyed(self))
+        return NULL;
 
     g_source_remove_poll(self->source, &fd->pollfd);
 
@@ -239,7 +280,8 @@
     GTimeVal timeval;
     double   ret;
 
-    CHECK_DESTROYED(self, NULL);
+    if (set_error_if_destroyed(self))
+        return NULL;
 
     g_source_get_current_time(self->source, &timeval);
     ret = (double)timeval.tv_sec + (double)timeval.tv_usec * 0.000001;
@@ -273,7 +315,8 @@
 static PyObject *
 pyg_source_get_priority(PyGSource *self, void *closure)
 {
-    CHECK_DESTROYED(self, NULL);
+    if (set_error_if_destroyed(self))
+        return NULL;
 
     return PyInt_FromLong(g_source_get_priority(self->source));
 }
@@ -281,7 +324,8 @@
 static int
 pyg_source_set_priority(PyGSource *self, PyObject *value, void *closure)
 {
-    CHECK_DESTROYED(self, -1);
+    if(set_error_if_destroyed(self))
+        return -1;
 
     if (value == NULL) {
 	PyErr_SetString(PyExc_TypeError, "cannot delete priority");
@@ -301,7 +345,8 @@
 static PyObject *
 pyg_source_get_can_recurse(PyGSource *self, void *closure)
 {
-    CHECK_DESTROYED(self, NULL);
+    if(set_error_if_destroyed(self))
+        return NULL;
 
     return PyBool_FromLong(g_source_get_can_recurse(self->source));
 }
@@ -309,7 +354,8 @@
 static int
 pyg_source_set_can_recurse(PyGSource *self, PyObject *value, void *closure)
 {
-    CHECK_DESTROYED(self, -1);
+    if(set_error_if_destroyed(self))
+        return -1;
 
     if (value == NULL) {
 	PyErr_SetString(PyExc_TypeError, "cannot delete can_recurse");
@@ -324,7 +370,8 @@
 static PyObject *
 pyg_source_get_id(PyGSource *self, void *closure)
 {
-    CHECK_DESTROYED(self, NULL);
+    if(set_error_if_destroyed(self))
+        return NULL;
 
     if (g_source_get_context(self->source) == NULL) {
 	PyErr_SetString(PyExc_RuntimeError, "source is not attached");
@@ -348,13 +395,20 @@
     return source_repr(self, NULL);
 }
 
+/* We can (and should) self-collect if:
+ *  - wrapper is referenced only by source, and
+ *  - we don't have a finalize method.
+ */
 static int
 pyg_source_traverse(PyGSource *self, visitproc visit, void *arg)
 {
-    int ret = 0;
+    Py_VISIT(self->inst_dict);
 
-    if (self->inst_dict) ret = visit(self->inst_dict, arg);
-    if (ret != 0) return ret;
+    if (self->source
+            && self->refed_by_gsource
+            && self->source->ref_count == 1
+            && !PyObject_HasAttrString((PyObject *)self, "finalize"))
+        Py_VISIT(self);
 
     return 0;
 }
@@ -362,32 +416,21 @@
 static int
 pyg_source_clear(PyGSource *self)
 {
-    PyObject *tmp;
+    Py_CLEAR(self->inst_dict);
 
-    tmp = self->inst_dict;
-    self->inst_dict = NULL;
-    Py_XDECREF(tmp);
+    if(self->source) {
+        /* make sure we can't touch the source again, then unref
+         */
+        GSource* source = self->source;
 
-    if (self->source) {
-	g_source_unref(self->source);
-	self->source = NULL;
+        self->source = NULL;
+
+        g_source_unref(source);
     }
 
     return 0;
 }
 
-static void
-pyg_source_dealloc(PyGSource *self)
-{
-    PyObject_ClearWeakRefs((PyObject *)self);
-
-    PyObject_GC_UnTrack((PyObject *)self);
-
-    pyg_source_clear(self);
-
-    PyObject_GC_Del(self);
-}
-
 static gboolean
 pyg_source_prepare(GSource *source, gint *timeout)
 {
@@ -520,6 +563,8 @@
 	    Py_DECREF(t);
 	}
     }
+    else
+        PyErr_Clear();
 
     pyg_gil_state_release(state);
 }
@@ -546,7 +591,10 @@
     self->weakreflist = NULL;
 
     self->python_source = TRUE;
+    self->refed_by_gsource = TRUE;
 
+    Py_INCREF(self);
+
     return 0;
 }
 
@@ -556,6 +604,29 @@
     PyObject_GC_Del(op);
 }
 
+static void
+pyg_source_dealloc(PyGSource *self)
+{
+    if (self->weakreflist)
+        PyObject_ClearWeakRefs((PyObject *)self);
+
+    /* Keep the object alive through (potential) finalization ---
+     * we eventually free it with a positive refcount; also note
+     * that we assume finalization cannot occur through the GC.
+     */
+    Py_INCREF(self);
+
+    PyObject_GC_UnTrack(self);
+
+    pyg_source_clear(self);
+
+    if (self->ob_refcnt <= 1) {
+        /* The object was not resurrected by the finalizer.
+         */
+        pyg_source_free((PyObject *)self);
+    }
+}
+
 PyTypeObject PyGSource_Type = {
     PyObject_HEAD_INIT(NULL)
     0,					/* ob_size */
@@ -628,7 +699,10 @@
     self->weakreflist = NULL;
 
     self->python_source = FALSE;
+    self->refed_by_gsource = TRUE;
 
+    Py_INCREF(self);
+
     return 0;
 }
 
@@ -653,8 +727,7 @@
     (getattrofunc)0,			/* tp_getattro */
     (setattrofunc)0,			/* tp_setattro */
     0,					/* tp_as_buffer */
-    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE |
-      Py_TPFLAGS_HAVE_GC,		/* tp_flags */
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */
     NULL,				/* tp_doc */
     (traverseproc)0,			/* tp_traverse */
     (inquiry)0,				/* tp_clear */
@@ -705,7 +778,10 @@
     self->weakreflist = NULL;
 
     self->python_source = FALSE;
+    self->refed_by_gsource = TRUE;
 
+    Py_INCREF(self);
+
     return 0;
 }
 
@@ -730,8 +806,7 @@
     (getattrofunc)0,			/* tp_getattro */
     (setattrofunc)0,			/* tp_setattro */
     0,					/* tp_as_buffer */
-    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE |
-      Py_TPFLAGS_HAVE_GC,		/* tp_flags */
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */
     NULL,				/* tp_doc */
     (traverseproc)0,			/* tp_traverse */
     (inquiry)0,				/* tp_clear */
