[gjs] object: Refactor to use GjsMaybeOwned instead of keep-alive

[Philip Chimento <pchimento src gnome org>]

commit ea43a1c43f3905dac45fa4a34dc27813d81ce729
Author: Philip Chimento <philip endlessm com>
Date:   Thu Feb 2 16:19:21 2017 -0800

    object: Refactor to use GjsMaybeOwned instead of keep-alive
    
    In object.cpp, we previously kept a GObject's JS wrapper object alive by
    adding it to the keep-alive object. We can now use GjsMaybeOwned for this
    purpose if we add an accessor function to check whether the GjsMaybeOwned
    is in rooted mode.
    
    This is in preparation for an even deeper refactor where we replace the
    JS::Heap wrapper stored in the object's qdata with GjsMaybeOwned as well.
    
    This requires a small change to GjsContext's dispose handler so that we
    handle all pending toggle notifications before doing the first garbage
    collection.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776966

 gi/keep-alive.cpp         |  444 ---------------------------------------------
 gi/keep-alive.h           |   92 ----------
 gi/object.cpp             |   89 +++++-----
 gjs-srcs.mk               |    2 -
 gjs/context.cpp           |    5 +
 gjs/jsapi-util-root.h     |    2 +
 test/gjs-test-rooting.cpp |   24 +++
 7 files changed, 73 insertions(+), 585 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 776ca4d..707dae4 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -24,6 +24,7 @@
 #include <config.h>
 
 #include <memory>
+#include <set>
 #include <stack>
 #include <string.h>
 #include <vector>
@@ -39,9 +40,9 @@
 #include "proxyutils.h"
 #include "param.h"
 #include "value.h"
-#include "keep-alive.h"
 #include "closure.h"
 #include "gjs_gi_trace.h"
+#include "gjs/jsapi-util-root.h"
 #include "gjs/jsapi-wrapper.h"
 #include "gjs/context-private.h"
 #include "gjs/mem.h"
@@ -54,7 +55,7 @@
 struct ObjectInstance {
     GIObjectInfo *info;
     GObject *gobj; /* NULL if we are the prototype and not an instance */
-    JSObject *keep_alive; /* NULL if we are not added to it */
+    GjsMaybeOwned<JSObject *> keep_alive;
     GType gtype;
 
     /* a list of all signal connections, used when tracing */
@@ -95,6 +96,7 @@ static GHashTable *class_init_properties;
 extern struct JSClass gjs_object_instance_class;
 static GThread *gjs_eval_thread;
 static volatile gint pending_idle_toggles;
+static std::set<ObjectInstance *> dissociate_list;
 
 GJS_DEFINE_PRIV_FROM_JS(ObjectInstance, gjs_object_instance_class)
 
@@ -187,6 +189,21 @@ throw_priv_is_null_error(JSContext *context)
               " up to the parent _init properly?");
 }
 
+static void
+dissociate_list_add(ObjectInstance *priv)
+{
+    bool inserted;
+    std::tie(std::ignore, inserted) = dissociate_list.insert(priv);
+    g_assert(inserted);
+}
+
+static void
+dissociate_list_remove(ObjectInstance *priv)
+{
+    size_t erased = dissociate_list.erase(priv);
+    g_assert(erased > 0);
+}
+
 static ValueFromPropertyResult
 init_g_param_from_property(JSContext      *context,
                            const char     *js_prop_name,
@@ -900,8 +917,8 @@ wrapped_gobj_dispose_notify(gpointer      data,
 #endif
 
 static void
-gobj_no_longer_kept_alive_func(JSObject *obj,
-                               void     *data)
+gobj_no_longer_kept_alive_func(JS::HandleObject obj,
+                               void            *data)
 {
     ObjectInstance *priv;
 
@@ -909,9 +926,10 @@ gobj_no_longer_kept_alive_func(JSObject *obj,
 
     gjs_debug_lifecycle(GJS_DEBUG_GOBJECT,
                         "GObject wrapper %p will no longer be kept alive, eligible for collection",
-                        obj);
+                        obj.get());
 
-    priv->keep_alive = NULL;
+    priv->keep_alive.reset();
+    dissociate_list_remove(priv);
 }
 
 static GQuark
@@ -984,18 +1002,15 @@ handle_toggle_down(GObject *gobj)
 
     gjs_debug_lifecycle(GJS_DEBUG_GOBJECT,
                         "Toggle notify gobj %p obj %p is_last_ref true keep-alive %p",
-                        gobj, obj, priv->keep_alive);
+                        gobj, obj, priv->keep_alive.get());
 
     /* Change to weak ref so the wrapper-wrappee pair can be
      * collected by the GC
      */
-    if (priv->keep_alive != NULL) {
+    if (priv->keep_alive.rooted()) {
         gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "Removing object from keep alive");
-        gjs_keep_alive_remove_child(priv->keep_alive,
-                                    gobj_no_longer_kept_alive_func,
-                                    obj,
-                                    priv);
-        priv->keep_alive = NULL;
+        priv->keep_alive.reset();
+        dissociate_list_remove(priv);
     }
 }
 
@@ -1018,21 +1033,20 @@ handle_toggle_up(GObject   *gobj)
 
     gjs_debug_lifecycle(GJS_DEBUG_GOBJECT,
                         "Toggle notify gobj %p obj %p is_last_ref false keep-alive %p",
-                        gobj, obj, priv->keep_alive);
+                        gobj, obj, priv->keep_alive.get());
 
     /* Change to strong ref so the wrappee keeps the wrapper alive
      * in case the wrapper has data in it that the app cares about
      */
-    if (priv->keep_alive == NULL) {
+    if (!priv->keep_alive.rooted()) {
+        priv->keep_alive.reset();
         /* FIXME: thread the context through somehow. Maybe by looking up
          * the compartment that obj belongs to. */
         GjsContext *context = gjs_context_get_current();
         gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "Adding object to keep alive");
-        priv->keep_alive = gjs_keep_alive_get_global((JSContext*) gjs_context_get_native_context(context));
-        gjs_keep_alive_add_child(priv->keep_alive,
-                                 gobj_no_longer_kept_alive_func,
-                                 obj,
-                                 priv);
+        auto cx = static_cast<JSContext *>(gjs_context_get_native_context(context));
+        priv->keep_alive.root(cx, obj, gobj_no_longer_kept_alive_func, priv);
+        dissociate_list_add(priv);
     }
 }
 
@@ -1251,14 +1265,6 @@ gjs_object_clear_toggles(void)
 void
 gjs_object_prepare_shutdown (JSContext *context)
 {
-    JSObject *keep_alive = gjs_keep_alive_get_global_if_exists (context);
-    GjsKeepAliveIter kiter;
-    JSObject *child;
-    void *data;
-
-    if (!keep_alive)
-        return;
-
     /* First, get rid of anything left over on the main context */
     gjs_object_clear_toggles();
 
@@ -1267,14 +1273,9 @@ gjs_object_prepare_shutdown (JSContext *context)
      *   toggle ref removal -> gobj dispose -> toggle ref notify
      * by simply ignoring toggle ref notifications during this process.
      */
-    gjs_keep_alive_iterator_init(&kiter, keep_alive);
-    while (gjs_keep_alive_iterator_next(&kiter,
-                                        gobj_no_longer_kept_alive_func,
-                                        &child, &data)) {
-        ObjectInstance *priv = (ObjectInstance*)data;
-
-        release_native_object(priv);
-    }
+    for (auto iter : dissociate_list)
+        release_native_object(iter);
+    dissociate_list.clear();
 }
 
 static ObjectInstance *
@@ -1338,12 +1339,8 @@ associate_js_gobject (JSContext       *context,
      * the wrapper to be garbage collected (and thus unref the
      * wrappee).
      */
-    priv->keep_alive = gjs_keep_alive_get_global(context);
-    gjs_keep_alive_add_child(priv->keep_alive,
-                             gobj_no_longer_kept_alive_func,
-                             object,
-                             priv);
-
+    priv->keep_alive.root(context, object, gobj_no_longer_kept_alive_func, priv);
+    dissociate_list_add(priv);
     g_object_add_toggle_ref(gobj, wrapped_gobj_toggle_notify, NULL);
 }
 
@@ -1565,7 +1562,7 @@ object_instance_finalize(JSFreeOp  *fop,
         release_native_object(priv);
     }
 
-    if (priv->keep_alive != NULL) {
+    if (priv->keep_alive.rooted()) {
         /* This happens when the refcount on the object is still >1,
          * for example with global objects GDK never frees like GdkDisplay,
          * when we close down the JS runtime.
@@ -1576,10 +1573,8 @@ object_instance_finalize(JSFreeOp  *fop,
         gjs_debug_lifecycle(GJS_DEBUG_GOBJECT,
                             "Removing from keep alive");
 
-        gjs_keep_alive_remove_child(priv->keep_alive,
-                                    gobj_no_longer_kept_alive_func,
-                                    obj,
-                                    priv);
+        priv->keep_alive.reset();
+        dissociate_list_remove(priv);
     }
 
     if (priv->info) {
diff --git a/gjs-srcs.mk b/gjs-srcs.mk
index 7c6dc81..2295bf8 100644
--- a/gjs-srcs.mk
+++ b/gjs-srcs.mk
@@ -32,8 +32,6 @@ gjs_srcs =                            \
        gi/gtype.h                      \
        gi/interface.cpp                \
        gi/interface.h                  \
-       gi/keep-alive.cpp               \
-       gi/keep-alive.h                 \
        gi/ns.cpp                       \
        gi/ns.h                         \
        gi/object.cpp                   \
diff --git a/gjs/context.cpp b/gjs/context.cpp
index a75c938..1b276e6 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -400,6 +400,11 @@ gjs_context_dispose(GObject *object)
         gjs_debug(GJS_DEBUG_CONTEXT,
                   "Destroying JS context");
 
+        /* Finalize any pending toggle refs left over on the main context,
+         * before doing any garbage collection, so that we can collect the JS
+         * wrapper objects */
+        gjs_object_clear_toggles();
+
         JS_BeginRequest(js_context->context);
 
         /* Do a full GC here before tearing down, since once we do
diff --git a/gjs/jsapi-util-root.h b/gjs/jsapi-util-root.h
index 6220975..eea0462 100644
--- a/gjs/jsapi-util-root.h
+++ b/gjs/jsapi-util-root.h
@@ -263,6 +263,8 @@ public:
         g_assert(!m_rooted);
         GjsHeapOperation<T>::trace(tracer, &m_heap, name);
     }
+
+    bool rooted(void) { return m_rooted; }
 };
 
 #endif /* GJS_JSAPI_UTIL_ROOT_H */
diff --git a/test/gjs-test-rooting.cpp b/test/gjs-test-rooting.cpp
index af68ce0..99f160f 100644
--- a/test/gjs-test-rooting.cpp
+++ b/test/gjs-test-rooting.cpp
@@ -90,6 +90,26 @@ wait_for_gc(GjsRootingFixture *fx)
 }
 
 static void
+test_maybe_owned_rooted_flag_set_when_rooted(GjsRootingFixture *fx,
+                                             gconstpointer      unused)
+{
+    auto obj = new GjsMaybeOwned<JS::Value>();
+    obj->root(PARENT(fx)->cx, JS::TrueValue());
+    g_assert_true(obj->rooted());
+    delete obj;
+}
+
+static void
+test_maybe_owned_rooted_flag_not_set_when_not_rooted(GjsRootingFixture *fx,
+                                                     gconstpointer      unused)
+{
+    auto obj = new GjsMaybeOwned<JS::Value>();
+    *obj = JS::TrueValue();
+    g_assert_false(obj->rooted());
+    delete obj;
+}
+
+static void
 test_maybe_owned_rooted_keeps_alive_across_gc(GjsRootingFixture *fx,
                                               gconstpointer      unused)
 {
@@ -191,6 +211,10 @@ gjs_test_add_tests_for_rooting(void)
 #define ADD_ROOTING_TEST(path, f) \
     g_test_add("/rooting/" path, GjsRootingFixture, NULL, setup, f,  teardown);
 
+    ADD_ROOTING_TEST("maybe-owned/rooted-flag-set-when-rooted",
+                     test_maybe_owned_rooted_flag_set_when_rooted);
+    ADD_ROOTING_TEST("maybe-owned/rooted-flag-not-set-when-not-rooted",
+                     test_maybe_owned_rooted_flag_not_set_when_not_rooted);
     ADD_ROOTING_TEST("maybe-owned/rooted-keeps-alive-across-gc",
                      test_maybe_owned_rooted_keeps_alive_across_gc);
     ADD_ROOTING_TEST("maybe-owned/rooted-is-collected-after-reset",