[gjs] Trace signal closures from the object instead of the context keep-alive

[Giovanni Campagna <gcampagna src gnome org>]

commit 06aa616a8c9b6d356aefc95a6b0c1c317b86c46a
Author: Giovanni Campagna <gcampagna src gnome org>
Date:   Wed Jun 20 22:30:52 2012 +0200

    Trace signal closures from the object instead of the context keep-alive
    
    Implement tracing for GObjects and use it so that signal handlers
    are rooted to the objects they're connected, and not the global
    object. This means that if the object goes away and the only thing
    referring to it is the handler function, it is recognized as a
    cycle by the GC and collected, reducing memory leaks.
    Other closures, as well as callback trampolines and vfuncs, are still
    rooted in the usual way.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=678504

 gi/closure.c       |   48 +++++++++++++++++++++++++++-----
 gi/closure.h       |    6 +++-
 gi/object.c        |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 gi/value.c         |    4 +-
 modules/dbus.c     |   13 +++++----
 modules/mainloop.c |    6 ++--
 6 files changed, 132 insertions(+), 22 deletions(-)
---
diff --git a/gi/closure.c b/gi/closure.c
index 873c28c..96a7477 100644
--- a/gi/closure.c
+++ b/gi/closure.c
@@ -230,6 +230,17 @@ closure_invalidated(gpointer data,
 }
 
 static void
+closure_set_invalid(gpointer  data,
+                    GClosure *closure)
+{
+    Closure *self = (Closure*) closure;
+
+    self->obj = NULL;
+    self->context = NULL;
+    self->runtime = NULL;
+}
+
+static void
 closure_finalized(gpointer data,
                   GClosure *closure)
 {
@@ -317,10 +328,25 @@ gjs_closure_get_callable(GClosure *closure)
     return c->obj;
 }
 
+void
+gjs_closure_trace(GClosure *closure,
+                  JSTracer *tracer)
+{
+    Closure *c;
+
+    c = (Closure*) closure;
+
+    if (c->obj == NULL)
+        return;
+
+    JS_CALL_OBJECT_TRACER(tracer, c->obj, "signal connection");
+}
+
 GClosure*
 gjs_closure_new(JSContext  *context,
-                   JSObject   *callable,
-                   const char *description)
+                JSObject   *callable,
+                const char *description,
+                gboolean    root_function)
 {
     Closure *c;
 
@@ -343,12 +369,19 @@ gjs_closure_new(JSContext  *context,
      */
     g_closure_add_finalize_notifier(&c->base, NULL, closure_finalized);
 
-    gjs_keep_alive_add_global_child(context,
-                                    global_context_finalized,
-                                    c->obj,
-                                    c);
+    if (root_function) {
+        /* Fully manage closure lifetime if so asked */
+        gjs_keep_alive_add_global_child(context,
+                                        global_context_finalized,
+                                        c->obj,
+                                        c);
 
-    g_closure_add_invalidate_notifier(&c->base, NULL, closure_invalidated);
+        g_closure_add_invalidate_notifier(&c->base, NULL, closure_invalidated);
+    } else {
+        /* Only mark the closure as invalid if memory is managed
+           outside (i.e. by object.c for signals) */
+        g_closure_add_invalidate_notifier(&c->base, NULL, closure_set_invalid);
+    }
 
     gjs_debug_closure("Create closure %p which calls object %p '%s'",
                       c, c->obj, description);
@@ -357,4 +390,3 @@ gjs_closure_new(JSContext  *context,
 
     return &c->base;
 }
-
diff --git a/gi/closure.h b/gi/closure.h
index c8a821b..f894805 100644
--- a/gi/closure.h
+++ b/gi/closure.h
@@ -32,7 +32,8 @@ G_BEGIN_DECLS
 
 GClosure*  gjs_closure_new           (JSContext    *context,
                                       JSObject     *callable,
-                                      const char   *description);
+                                      const char   *description,
+                                      gboolean      root_function);
 void       gjs_closure_invoke        (GClosure     *closure,
                                       int           argc,
                                       jsval        *argv,
@@ -41,6 +42,9 @@ JSRuntime* gjs_closure_get_runtime   (GClosure     *closure);
 gboolean   gjs_closure_is_valid      (GClosure     *closure);
 JSObject*  gjs_closure_get_callable  (GClosure     *closure);
 
+void       gjs_closure_trace         (GClosure     *closure,
+                                      JSTracer     *tracer);
+
 G_END_DECLS
 
 #endif  /* __GJS_CLOSURE_H__ */
diff --git a/gi/object.c b/gi/object.c
index 95109db..fead7cd 100644
--- a/gi/object.c
+++ b/gi/object.c
@@ -34,6 +34,7 @@
 #include "param.h"
 #include "value.h"
 #include "keep-alive.h"
+#include "closure.h"
 #include "gjs_gi_trace.h"
 
 #include <gjs/gjs-module.h>
@@ -50,8 +51,17 @@ typedef struct {
     GObject *gobj; /* NULL if we are the prototype and not an instance */
     JSObject *keep_alive; /* NULL if we are not added to it */
     GType gtype;
+
+    /* a list of all signal connections, used when tracing */
+    GList *signals;
 } ObjectInstance;
 
+typedef struct {
+    ObjectInstance *obj;
+    GList *link;
+    GClosure *closure;
+} ConnectData;
+
 enum {
     PROP_0,
     PROP_JS_CONTEXT,
@@ -1011,6 +1021,39 @@ GJS_NATIVE_CONSTRUCTOR_DECLARE(object_instance)
 }
 
 static void
+invalidate_all_signals(ObjectInstance *priv)
+{
+    GList *iter, *next;
+
+    for (iter = priv->signals; iter; ) {
+        ConnectData *cd = iter->data;
+        next = iter->next;
+
+        /* This will also free cd and iter, through
+           the closure invalidation mechanism */
+        g_closure_invalidate(cd->closure);
+
+        iter = next;
+    }
+}
+
+static void
+object_instance_trace(JSTracer *tracer,
+                      JSObject *obj)
+{
+    ObjectInstance *priv;
+    GList *iter;
+
+    priv = priv_from_js(tracer->context, obj);
+
+    for (iter = priv->signals; iter; iter = iter->next) {
+        ConnectData *cd = iter->data;
+
+        gjs_closure_trace(cd->closure, tracer);
+    }
+}
+
+static void
 object_instance_finalize(JSContext *context,
                          JSObject  *obj)
 {
@@ -1031,6 +1074,8 @@ object_instance_finalize(JSContext *context,
                                     priv->info ? g_base_info_get_name ( (GIBaseInfo*) priv->info) : g_type_name(priv->gtype)));
 
     if (priv->gobj) {
+        invalidate_all_signals (priv);
+
         if (G_UNLIKELY (priv->gobj->ref_count <= 0)) {
             g_error("Finalizing proxy for an already freed object of type: %s.%s\n",
                     priv->info ? g_base_info_get_namespace((GIBaseInfo*) priv->info) : "",
@@ -1084,6 +1129,17 @@ gjs_lookup_object_prototype(JSContext    *context,
     return proto;
 }
 
+static void
+signal_connection_invalidated (gpointer  user_data,
+                               GClosure *closure)
+{
+    ConnectData *connect_data = user_data;
+
+    connect_data->obj->signals = g_list_delete_link(connect_data->obj->signals,
+                                                    connect_data->link);
+    g_slice_free(ConnectData, connect_data);
+}
+
 static JSBool
 real_connect_func(JSContext *context,
                   uintN      argc,
@@ -1099,6 +1155,7 @@ real_connect_func(JSContext *context,
     char *signal_name;
     GQuark signal_detail;
     jsval retval;
+    ConnectData *connect_data;
     JSBool ret = JS_FALSE;
 
     priv = priv_from_js(context, obj);
@@ -1148,6 +1205,14 @@ real_connect_func(JSContext *context,
     if (closure == NULL)
         goto out;
 
+    connect_data = g_slice_new(ConnectData);
+    priv->signals = g_list_prepend(priv->signals, connect_data);
+    connect_data->obj = priv;
+    connect_data->link = priv->signals;
+    /* This is a weak reference, and will be cleared when the closure is invalidated */
+    connect_data->closure = closure;
+    g_closure_add_invalidate_notifier(closure, connect_data, signal_connection_invalidated);
+
     id = g_signal_connect_closure(priv->gobj,
                                   signal_name,
                                   closure,
@@ -1392,7 +1457,8 @@ static struct JSClass gjs_object_instance_class = {
     NULL, /* We copy this class struct with multiple names */
     JSCLASS_HAS_PRIVATE |
     JSCLASS_NEW_RESOLVE |
-    JSCLASS_NEW_RESOLVE_GETS_START,
+    JSCLASS_NEW_RESOLVE_GETS_START |
+    JSCLASS_MARK_IS_TRACE,
     JS_PropertyStub,
     JS_PropertyStub,
     object_instance_get_prop,
@@ -1401,7 +1467,14 @@ static struct JSClass gjs_object_instance_class = {
     (JSResolveOp) object_instance_new_resolve, /* needs cast since it's the new resolve signature */
     JS_ConvertStub,
     object_instance_finalize,
-    JSCLASS_NO_OPTIONAL_MEMBERS
+    NULL,
+    NULL,
+    NULL,
+    NULL,
+    NULL,
+    NULL,
+    JS_CLASS_TRACE(object_instance_trace),
+    NULL,
 };
 
 static JSBool
diff --git a/gi/value.c b/gi/value.c
index 4d02827..5d8fe16 100644
--- a/gi/value.c
+++ b/gi/value.c
@@ -154,7 +154,7 @@ gjs_closure_new_for_signal(JSContext  *context,
 {
     GClosure *closure;
 
-    closure = gjs_closure_new(context, callable, description);
+    closure = gjs_closure_new(context, callable, description, FALSE);
 
     g_closure_set_meta_marshal(closure, GUINT_TO_POINTER(signal_id), closure_marshal);
 
@@ -168,7 +168,7 @@ gjs_closure_new_marshaled (JSContext    *context,
 {
     GClosure *closure;
 
-    closure = gjs_closure_new(context, callable, description);
+    closure = gjs_closure_new(context, callable, description, TRUE);
 
     g_closure_set_marshal(closure, closure_marshal);
 
diff --git a/modules/dbus.c b/modules/dbus.c
index 92bfd2e..3a62025 100644
--- a/modules/dbus.c
+++ b/modules/dbus.c
@@ -429,7 +429,7 @@ gjs_js_dbus_call_async(JSContext  *context,
      * and deal with the GC root and other issues, even though we
      * won't ever marshal via GValue
      */
-    closure = gjs_closure_new(context, JSVAL_TO_OBJECT(argv[9]), "async call");
+    closure = gjs_closure_new(context, JSVAL_TO_OBJECT(argv[9]), "async call", TRUE);
     if (closure == NULL) {
         dbus_pending_call_unref(pending);
         return JS_FALSE;
@@ -505,7 +505,8 @@ signal_handler_new(JSContext *context,
      */
     handler->closure = gjs_closure_new(context,
                                        JSVAL_TO_OBJECT(callable),
-                                       "signal watch");
+                                       "signal watch",
+                                       TRUE);
     if (handler->closure == NULL) {
         g_free(handler);
         return NULL;
@@ -1208,13 +1209,13 @@ gjs_js_dbus_acquire_name(JSContext  *context,
     owner->bus_type = bus_type;
 
     owner->acquired_closure =
-        gjs_closure_new(context, acquire_func, "acquired bus name");
+        gjs_closure_new(context, acquire_func, "acquired bus name", TRUE);
 
     g_closure_ref(owner->acquired_closure);
     g_closure_sink(owner->acquired_closure);
 
     owner->lost_closure =
-        gjs_closure_new(context, lost_func, "lost bus name");
+        gjs_closure_new(context, lost_func, "lost bus name", TRUE);
 
     g_closure_ref(owner->lost_closure);
     g_closure_sink(owner->lost_closure);
@@ -1440,13 +1441,13 @@ gjs_js_dbus_watch_name(JSContext  *context,
     watcher = g_slice_new0(GjsJSDBusNameWatcher);
 
     watcher->appeared_closure =
-        gjs_closure_new(context, appeared_func, "service appeared");
+        gjs_closure_new(context, appeared_func, "service appeared", TRUE);
 
     g_closure_ref(watcher->appeared_closure);
     g_closure_sink(watcher->appeared_closure);
 
     watcher->vanished_closure =
-        gjs_closure_new(context, vanished_func, "service vanished");
+        gjs_closure_new(context, vanished_func, "service vanished", TRUE);
 
     g_closure_ref(watcher->vanished_closure);
     g_closure_sink(watcher->vanished_closure);
diff --git a/modules/mainloop.c b/modules/mainloop.c
index 02b53c7..67912b2 100644
--- a/modules/mainloop.c
+++ b/modules/mainloop.c
@@ -198,7 +198,7 @@ gjs_timeout_add(JSContext *context,
                         "interval", &interval, "callback", &callback))
       return JS_FALSE;
 
-    closure = gjs_closure_new(context, callback, "timeout");
+    closure = gjs_closure_new(context, callback, "timeout", TRUE);
     if (closure == NULL)
         return JS_FALSE;
 
@@ -241,7 +241,7 @@ gjs_timeout_add_seconds(JSContext *context,
                         "interval", &interval, "callback", &callback))
       return JS_FALSE;
 
-    closure = gjs_closure_new(context, callback, "timeout_seconds");
+    closure = gjs_closure_new(context, callback, "timeout_seconds", TRUE);
     if (closure == NULL)
         return JS_FALSE;
 
@@ -288,7 +288,7 @@ gjs_idle_add(JSContext *context,
                         "callback", &callback, "priority", &priority))
       return JS_FALSE;
 
-    closure = gjs_closure_new(context, callback, "idle");
+    closure = gjs_closure_new(context, callback, "idle", TRUE);
     if (closure == NULL)
         return JS_FALSE;