[gjs] js: Update weak pointers after GC

[Philip Chimento <pchimento src gnome org>]

commit bc1910cf93bd66dfc104e61f3553156f4235cb95
Author: Philip Chimento <philip endlessm com>
Date:   Thu Jan 26 16:56:29 2017 -0800

    js: Update weak pointers after GC
    
    There are two places where we maintain weak pointers to GC things: one is
    in object.cpp, where a GObject is associated with its JS wrapper object.
    The other is in gtype.cpp, where each GType is associated with its
    wrapper object.
    
    In SpiderMonkey 38, we have to call JS_UpdateWeakPointerAfterGC() on weak
    pointers, every garbage collection cycle, in case the garbage collector
    moves them.
    
    We considered adding a separate callback for each object with
    JS_AddWeakPointerCallback() instead of one callback per file that
    processes all the weak pointers it knows about, but that is prohibitive
    since SpiderMonkey doesn't take the user data into account when calling
    JS_RemoveWeakPointerCallback(), so you can't add and remove multiple
    instances of the same callback.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=777962

 gi/gtype.cpp          |   52 ++++++++++++++++++++++++++------
 gi/object.cpp         |   78 ++++++++++++++++++++++++++++++-------------------
 gjs/jsapi-util-root.h |    3 +-
 3 files changed, 92 insertions(+), 41 deletions(-)
---
diff --git a/gi/gtype.cpp b/gi/gtype.cpp
index 10ba932..61708e6 100644
--- a/gi/gtype.cpp
+++ b/gi/gtype.cpp
@@ -24,11 +24,16 @@
 
 #include <config.h>
 
+#include <set>
+
 #include "gtype.h"
 #include "gjs/jsapi-wrapper.h"
 #include <util/log.h>
 #include <girepository.h>
 
+static bool weak_pointer_callback = false;
+static std::set<GType> weak_pointer_list;
+
 static JS::Value
 gjs_gtype_create_proto(JSContext       *context,
                        JS::HandleObject module,
@@ -53,6 +58,30 @@ gjs_get_gtype_wrapper_quark(void)
 }
 
 static void
+update_gtype_weak_pointers(JSRuntime *rt,
+                           void      *data)
+{
+    for (auto iter = weak_pointer_list.begin(); iter != weak_pointer_list.end(); ) {
+        auto heap_wrapper = static_cast<JS::Heap<JSObject *> *>(g_type_get_qdata(*iter, 
gjs_get_gtype_wrapper_quark()));
+        JS_UpdateWeakPointerAfterGC(heap_wrapper);
+        if (*heap_wrapper == nullptr)
+            iter = weak_pointer_list.erase(iter);
+        else
+            iter++;
+    }
+}
+
+static void
+ensure_weak_pointer_callback(JSContext *cx)
+{
+    if (!weak_pointer_callback) {
+        JS_AddWeakPointerCallback(JS_GetRuntime(cx), update_gtype_weak_pointers,
+                                  nullptr);
+        weak_pointer_callback = true;
+    }
+}
+
+static void
 gjs_gtype_finalize(JSFreeOp *fop,
                    JSObject *obj)
 {
@@ -62,6 +91,7 @@ gjs_gtype_finalize(JSFreeOp *fop,
     if (G_UNLIKELY(gtype == 0))
         return;
 
+    weak_pointer_list.erase(gtype);
     g_type_set_qdata(gtype, gjs_get_gtype_wrapper_quark(), NULL);
 }
 
@@ -120,8 +150,6 @@ JSObject *
 gjs_gtype_create_gtype_wrapper (JSContext *context,
                                 GType      gtype)
 {
-    JSObject *object;
-
     JS_BeginRequest(context);
 
     /* put constructor for GIRepositoryGType() in the global namespace */
@@ -129,21 +157,25 @@ gjs_gtype_create_gtype_wrapper (JSContext *context,
     JS::RootedObject proto(context,
         gjs_gtype_create_proto(context, global, "GIRepositoryGType", JS::NullPtr()).toObjectOrNull());
 
-    object = (JSObject*) g_type_get_qdata(gtype, gjs_get_gtype_wrapper_quark());
-    if (object != NULL)
+    auto heap_wrapper =
+        static_cast<JS::Heap<JSObject *> *>(g_type_get_qdata(gtype, gjs_get_gtype_wrapper_quark()));
+    if (heap_wrapper != nullptr)
         goto out;
 
-    object = JS_NewObjectWithGivenProto(context, &gjs_gtype_class, proto,
-                                        JS::NullPtr());
-    if (object == NULL)
+    heap_wrapper = new JS::Heap<JSObject *>();
+    *heap_wrapper = JS_NewObjectWithGivenProto(context, &gjs_gtype_class, proto,
+                                               JS::NullPtr());
+    if (*heap_wrapper == nullptr)
         goto out;
 
-    JS_SetPrivate(object, GSIZE_TO_POINTER(gtype));
-    g_type_set_qdata(gtype, gjs_get_gtype_wrapper_quark(), object);
+    JS_SetPrivate(*heap_wrapper, GSIZE_TO_POINTER(gtype));
+    ensure_weak_pointer_callback(context);
+    g_type_set_qdata(gtype, gjs_get_gtype_wrapper_quark(), heap_wrapper);
+    weak_pointer_list.insert(gtype);
 
  out:
     JS_EndRequest(context);
-    return object;
+    return *heap_wrapper;
 }
 
 static GType
diff --git a/gi/object.cpp b/gi/object.cpp
index a3b72da..585b813 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -99,6 +99,9 @@ enum {
 static std::stack<JS::PersistentRootedObject> object_init_list;
 static GHashTable *class_init_properties;
 
+static bool weak_pointer_callback = false;
+static std::set<ObjectInstance *> weak_pointer_list;
+
 extern struct JSClass gjs_object_instance_class;
 static GThread *gjs_eval_thread;
 static volatile gint pending_idle_toggles;
@@ -922,14 +925,15 @@ object_instance_props_to_g_parameters(JSContext                  *context,
 }
 
 #define DEBUG_DISPOSE 0
-#if DEBUG_DISPOSE
 static void
 wrapped_gobj_dispose_notify(gpointer      data,
                             GObject      *where_the_object_was)
 {
-    gjs_debug(GJS_DEBUG_GOBJECT, "JSObject %p GObject %p disposed", data, where_the_object_was);
-}
+    weak_pointer_list.erase(static_cast<ObjectInstance *>(data));
+#if DEBUG_DISPOSE
+    gjs_debug(GJS_DEBUG_GOBJECT, "Wrapped GObject %p disposed", where_the_object_was);
 #endif
+}
 
 static void
 gobj_no_longer_kept_alive_func(JS::HandleObject obj,
@@ -945,6 +949,7 @@ gobj_no_longer_kept_alive_func(JS::HandleObject obj,
 
     priv->keep_alive.reset();
     dissociate_list_remove(priv);
+    weak_pointer_list.erase(priv);
 }
 
 static GQuark
@@ -1146,10 +1151,9 @@ wrapped_gobj_toggle_notify(gpointer      data,
                            GObject      *gobj,
                            gboolean      is_last_ref)
 {
-    bool is_main_thread, is_sweeping;
+    bool is_main_thread;
     bool toggle_up_queued, toggle_down_queued;
     GjsContext *context;
-    JSContext *js_context;
 
     context = gjs_context_get_current();
     if (_gjs_context_destroying(context)) {
@@ -1192,12 +1196,6 @@ wrapped_gobj_toggle_notify(gpointer      data,
      * not a big deal.
      */
     is_main_thread = (gjs_eval_thread == g_thread_self());
-    if (is_main_thread) {
-        js_context = (JSContext*) gjs_context_get_native_context(context);
-        is_sweeping = gjs_runtime_is_sweeping(JS_GetRuntime(js_context));
-    } else {
-        is_sweeping = false;
-    }
 
     toggle_up_queued = toggle_idle_source_is_queued(gobj, TOGGLE_UP);
     toggle_down_queued = toggle_idle_source_is_queued(gobj, TOGGLE_DOWN);
@@ -1230,17 +1228,7 @@ wrapped_gobj_toggle_notify(gpointer      data,
                 g_error("toggling up object %s that's already queued to toggle up\n",
                         G_OBJECT_TYPE_NAME(gobj));
             }
-            if (is_sweeping) {
-                ObjectInstance *priv = get_object_qdata(gobj);
-                if (priv->keep_alive.update_after_gc()) {
-                    /* Ouch, the JS object is dead already. Disassociate the GObject
-                     * and hope the GObject dies too.
-                     */
-                    disassociate_js_gobject(gobj);
-                }
-            } else {
-                handle_toggle_up(gobj);
-            }
+            handle_toggle_up(gobj);
         } else {
             queue_toggle_idle(gobj, TOGGLE_UP);
         }
@@ -1317,6 +1305,37 @@ init_object_private (JSContext       *context,
 }
 
 static void
+update_heap_wrapper_weak_pointers(JSRuntime *rt,
+                                  gpointer   data)
+{
+    for (auto iter = weak_pointer_list.begin(); iter != weak_pointer_list.end(); ) {
+        ObjectInstance *priv = *iter;
+        if (priv->keep_alive.rooted() || priv->keep_alive == nullptr ||
+            !priv->keep_alive.update_after_gc()) {
+            iter++;
+        } else {
+            /* Ouch, the JS object is dead already. Disassociate the
+             * GObject and hope the GObject dies too. (Remove it from
+             * the weak pointer list first, since the disassociation
+             * may also cause it to be erased.)
+             */
+            iter = weak_pointer_list.erase(iter);
+            disassociate_js_gobject(priv->gobj);
+        }
+    }
+}
+
+static void
+ensure_weak_pointer_callback(JSContext *cx)
+{
+    if (!weak_pointer_callback) {
+        JS_AddWeakPointerCallback(JS_GetRuntime(cx),
+                                  update_heap_wrapper_weak_pointers, NULL);
+        weak_pointer_callback = true;
+    }
+}
+
+static void
 associate_js_gobject (JSContext       *context,
                       JS::HandleObject object,
                       GObject         *gobj)
@@ -1330,9 +1349,10 @@ associate_js_gobject (JSContext       *context,
 
     set_object_qdata(gobj, priv);
 
-#if DEBUG_DISPOSE
-    g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, object);
-#endif
+    ensure_weak_pointer_callback(context);
+    weak_pointer_list.insert(priv);
+
+    g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, priv);
 
     /* OK, here is where things get complicated. We want the
      * wrapped gobj to keep the JSObject* wrapper alive, because
@@ -1351,10 +1371,12 @@ associate_js_gobject (JSContext       *context,
 }
 
 static void
-disassociate_js_gobject (GObject   *gobj)
+disassociate_js_gobject(GObject *gobj)
 {
     ObjectInstance *priv = get_object_qdata(gobj);
 
+    g_object_weak_unref(priv->gobj, wrapped_gobj_dispose_notify, priv);
+
     /* Idles are already checked in the only caller of this
        function, the toggle ref notify, but let's check again...
     */
@@ -1366,10 +1388,6 @@ disassociate_js_gobject (GObject   *gobj)
 
     /* Mark that a JS object once existed, but it doesn't any more */
     priv->js_object_finalized = true;
-
-#if DEBUG_DISPOSE
-    g_object_weak_unref(gobj, wrapped_gobj_dispose_notify, object);
-#endif
 }
 
 static bool
diff --git a/gjs/jsapi-util-root.h b/gjs/jsapi-util-root.h
index 756e255..74acbfa 100644
--- a/gjs/jsapi-util-root.h
+++ b/gjs/jsapi-util-root.h
@@ -86,7 +86,8 @@ struct GjsHeapOperation<JSObject *> {
     static bool
     update_after_gc(JS::Heap<JSObject *> *location)
     {
-        return JS_IsAboutToBeFinalized(location);
+        JS_UpdateWeakPointerAfterGC(location);
+        return (*location == nullptr);
     }
 };