[gjs/wip/ptomato/785657: 2/6] Revert freeing closures in idle handler

[Philip Chimento <pchimento src gnome org>]

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

    Revert freeing closures in idle handler
    
    This turned out to cause a lot of problems; it has been responsible for
    almost all of the crashes in gnome-shell since 1.48. We revert back to
    the original code modulo a few improvements that have been made along the
    way.
    
    This state at least does not crash in the test suite, although it is
    definitely not correct since it breaks SpiderMonkey's garbage collector
    pre barrier verification mode (the reason the change was made in the
    first place.) That still must be fixed using a different approach.
    
    This partially reverts commits [41b78ae], [db3e387], [bace908],
    [9eb4a2b], [2593d3d], and [334ba96].

 gi/closure.cpp |   39 ++++++----------------------------
 gi/object.cpp  |   64 +++----------------------------------------------------
 2 files changed, 11 insertions(+), 92 deletions(-)
---
diff --git a/gi/closure.cpp b/gi/closure.cpp
index 5578335..8fd76ab 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -35,7 +35,6 @@
 struct Closure {
     JSContext *context;
     GjsMaybeOwned<JSObject *> obj;
-    unsigned idle_clear_id;
 };
 
 struct GjsClosure {
@@ -117,25 +116,6 @@ global_context_finalized(JS::HandleObject obj,
     }
 }
 
-/* Closures have to drop their references to their JS functions in an idle
- * handler, because otherwise the closure might stop tracing the function object
- * in the middle of garbage collection. That is not allowed with incremental GC.
- */
-static gboolean
-closure_clear_idle(void *data)
-{
-    auto closure = static_cast<GjsClosure *>(data);
-    gjs_debug_closure("Clearing closure %p which calls object %p",
-                      &closure->priv, closure->priv.obj.get());
-
-    closure->priv.obj.reset();
-    closure->priv.context = nullptr;
-    closure->priv.idle_clear_id = 0;
-
-    g_closure_unref(static_cast<GClosure *>(data));
-    return G_SOURCE_REMOVE;
-}
-
 /* Invalidation is like "dispose" - it is guaranteed to happen at
  * finalize, but may happen before finalize. Normally, g_closure_invalidate()
  * is called when the "target" of the closure becomes invalid, so that the
@@ -176,18 +156,20 @@ closure_invalidated(gpointer data,
                       "removing our destroy notifier on global object)",
                       closure);
 
-    c->idle_clear_id = g_idle_add(closure_clear_idle, closure);
-    g_closure_ref(closure);
+    c->obj.reset();
+    c->context = nullptr;
 }
 
 static void
 closure_set_invalid(gpointer  data,
                     GClosure *closure)
 {
+    Closure *self = &((GjsClosure*) closure)->priv;
+
+    self->obj.reset();
+    self->context = nullptr;
+
     GJS_DEC_COUNTER(closure);
-    Closure *c = &(reinterpret_cast<GjsClosure *>(closure))->priv;
-    c->idle_clear_id = g_idle_add(closure_clear_idle, closure);
-    g_closure_ref(closure);
 }
 
 static void
@@ -196,13 +178,6 @@ closure_finalize(gpointer  data,
 {
     Closure *self = &((GjsClosure*) closure)->priv;
 
-    if (self->idle_clear_id > 0) {
-        /* Remove any pending closure_clear_idle(), we are doing it
-         * immediately here. */
-        g_source_remove(self->idle_clear_id);
-        closure_clear_idle(closure);
-    }
-
     self->~Closure();
 }
 
diff --git a/gi/object.cpp b/gi/object.cpp
index a072242..8d82ac8 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -24,7 +24,6 @@
 #include <config.h>
 
 #include <deque>
-#include <map>
 #include <memory>
 #include <set>
 #include <stack>
@@ -66,7 +65,6 @@ struct ObjectInstance {
 
     /* a list of all signal connections, used when tracing */
     std::set<ConnectData *> signals;
-    std::map<ConnectData *, unsigned> pending_invalidations;
 
     /* the GObjectClass wrapped by this JS Object (only used for
        prototypes) */
@@ -1458,61 +1456,14 @@ object_instance_trace(JSTracer *tracer,
         vfunc->js_function.trace(tracer, "ObjectInstance::vfunc");
 }
 
-/* Removing the signal connection data from the list means that the object stops
- * tracing the JS function objects belonging to the closures. Incremental GC
- * does not allow that in the middle of a garbage collection. Therefore, we must
- * do it in an idle handler.
- */
-static gboolean
-signal_connection_invalidate_idle(void *user_data)
-{
-    auto cd = static_cast<ConnectData *>(user_data);
-    cd->obj->pending_invalidations.erase(cd);
-    cd->obj->signals.erase(cd);
-    g_slice_free(ConnectData, cd);
-    return G_SOURCE_REMOVE;
-}
-
 static void
 signal_connection_invalidated(void     *data,
                               GClosure *closure)
 {
     auto cd = static_cast<ConnectData *>(data);
-    std::map<ConnectData *, unsigned>& pending = cd->obj->pending_invalidations;
-    g_assert(pending.count(cd) == 0);
-    pending[cd] = g_idle_add(signal_connection_invalidate_idle, cd);
-}
-
-/* This is basically the same as invalidate_all_signals(), but does not defer
- * the invalidation to an idle handler. */
-static void
-invalidate_all_signals_now(ObjectInstance *priv)
-{
-    for (auto& iter : priv->pending_invalidations) {
-        ConnectData *cd = iter.first;
-        g_source_remove(iter.second);
-        g_slice_free(ConnectData, cd);
-        /* Erase element if not already erased */
-        priv->signals.erase(cd);
-    }
-    priv->pending_invalidations.clear();
 
-    /* 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()) {
-        ConnectData *cd = *priv->signals.begin();
-
-        /* We have to remove the invalidate notifier, which would
-         * otherwise schedule a new pending invalidation. */
-        g_closure_remove_invalidate_notifier(cd->closure, cd,
-                                             signal_connection_invalidated);
-        g_closure_invalidate(cd->closure);
-
-        g_slice_free(ConnectData, cd);
-        /* Erase element if not already erased */
-        priv->signals.erase(cd);
-    }
+    cd->obj->signals.erase(cd);
+    g_slice_free(ConnectData, cd);
 }
 
 static void
@@ -1534,16 +1485,9 @@ object_instance_finalize(JSFreeOp  *fop,
                                     priv->info ? g_base_info_get_namespace((GIBaseInfo*) priv->info) : 
"_gjs_private",
                                     priv->info ? g_base_info_get_name((GIBaseInfo*) priv->info) : 
g_type_name(priv->gtype)));
 
-    /* We must invalidate all signal connections now, instead of in an idle
-     * handler, because the object will not exist anymore when we get around to
-     * the idle function. We originally needed to defer these invalidations to
-     * an idle function since the object needs to continue tracing its signal
-     * connections while GC is going on. However, once the object is finalized,
-     * it will not be tracing them any longer anyway, so it's safe to do them
-     * now.
-     * This applies only to instances, not prototypes, but it's possible that
+    /* This applies only to instances, not prototypes, but it's possible that
      * an instance's GObject is already freed at this point. */
-    invalidate_all_signals_now(priv);
+    invalidate_all_signals(priv);
 
     /* Object is instance, not prototype, AND GObject is not already freed */
     if (priv->gobj) {