[gjs/wip/ptomato/785657: 8/8] WIP - object: invalidate signal connection later

[Philip Chimento <pchimento src gnome org>]

commit be03c93d7b61635c7ce477131eac8c11b424e5db
Author: Philip Chimento <philip chimento gmail com>
Date:   Sun Aug 20 22:07:27 2017 -0700

    WIP - object: invalidate signal connection later
    
    This defers the call of g_closure_invalidate() until the end of a garbage
    collection. We can't stop tracing signal connections in the middle of
    garbage collection, and we can't defer the actual freeing of the
    closures because that caused too many race conditions with the garbage
    collector. Instead, defer the invalidation.
    
    This probably means that more JS objects will survive a garbage
    collection when they didn't have to, but they should get collected in the
    following garbage collection.
    
    FIXME: Crashes when tearing down objects when the context is destroyed

 gi/object.cpp  |   42 +++++++++++++++++++++++++++++++++---------
 gi/object.h    |    2 ++
 gjs/engine.cpp |    5 +++++
 3 files changed, 40 insertions(+), 9 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index ba28391..2ffaf7c 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -84,6 +84,8 @@ extern struct JSClass gjs_object_instance_class;
 
 static std::set<ObjectInstance *> dissociate_list;
 
+static std::set<GClosure *> closures_to_invalidate;
+
 GJS_DEFINE_PRIV_FROM_JS(ObjectInstance, gjs_object_instance_class)
 
 static void            disassociate_js_gobject (GObject *gobj);
@@ -1268,18 +1270,33 @@ associate_js_gobject (JSContext       *context,
     g_object_add_toggle_ref(gobj, wrapped_gobj_toggle_notify, NULL);
 }
 
+/**
+ * gjs_object_invalidate_closures:
+ *
+ * Should be called periodically to clean up all closures that need to be
+ * invalidated as a result of running a garbage collection. Currently we call
+ * it at the end of every GC, in engine.cpp.
+ */
+void
+gjs_object_invalidate_closures(void)
+{
+    for (GClosure *closure : closures_to_invalidate) {
+        g_closure_invalidate(closure);
+        g_closure_unref(closure);
+    }
+    closures_to_invalidate.clear();
+}
+
+/* Called at the end of an object's lifetime, when all signals get
+ * disconnected. */
 static void
 invalidate_all_signals(ObjectInstance *priv)
 {
-    /* Can't loop directly through the items, since invalidating an item's
-     * closure might have the effect of removing the item from the set in the
-     * invalidate notifier */
-    while (!priv->signals.empty()) {
-        /* This will also free cd, through the closure invalidation mechanism */
-        GClosure *closure = *priv->signals.begin();
-        g_closure_invalidate(closure);
-        /* Erase element if not already erased */
-        priv->signals.erase(closure);
+    for (GClosure *closure : priv->signals) {
+        auto result = closures_to_invalidate.insert(closure);
+        /* result.second is whether the element was inserted or not */
+        if (result.second)
+            g_closure_ref(closure);
     }
 }
 
@@ -1480,6 +1497,13 @@ object_instance_finalize(JSFreeOp  *fop,
      * an instance's GObject is already freed at this point. */
     invalidate_all_signals(priv);
 
+    /* The invalidate notifier will want to use this object, so remove it */
+    for (GClosure *closure : priv->signals) {
+        g_closure_remove_invalidate_notifier(closure, priv,
+                                             signal_connection_invalidated);
+    }
+    priv->signals.clear();
+
     /* Object is instance, not prototype, AND GObject is not already freed */
     if (priv->gobj) {
         bool had_toggle_up;
diff --git a/gi/object.h b/gi/object.h
index 69017ef..722fea1 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -60,6 +60,8 @@ void      gjs_object_prepare_shutdown   (JSContext     *context);
 
 void gjs_object_clear_toggles(void);
 
+void gjs_object_invalidate_closures(void);
+
 void gjs_object_define_static_methods(JSContext       *context,
                                       JS::HandleObject constructor,
                                       GType            gtype,
diff --git a/gjs/engine.cpp b/gjs/engine.cpp
index a6d5f48..1da82c6 100644
--- a/gjs/engine.cpp
+++ b/gjs/engine.cpp
@@ -207,6 +207,11 @@ on_garbage_collect(JSContext *cx,
      * garbage collected. */
     if (status == JSGC_BEGIN)
         gjs_object_clear_toggles();
+    /* After garbage collection, invalidate any closures that have been
+     * disconnected. We can't do this during garbage collection because it's
+     * illegal to stop tracing a closure in the middle of GC. */
+    else if (status == JSGC_END)
+        gjs_object_invalidate_closures();
 }
 
 static bool