[gjs/wip/ptomato/mozjs38: 1/26] js: Refactor dual use of JS::Heap wrapper

[Philip Chimento <pchimento src gnome org>]

commit b36c2fffea212d337bdd02e6c95b6fb79014ab13
Author: Philip Chimento <philip chimento gmail com>
Date:   Wed Jan 11 23:34:40 2017 -0800

    js: Refactor dual use of JS::Heap wrapper
    
    The previous situation was that a JS::Heap wrapper was used to root a GC
    thing under some conditions using JS::AddFooRoot() or the keep-alive
    object, and trace or maintain a weak pointer otherwise.
    
    This will not be possible anymore in SpiderMonkey 38. JS::AddFooRoot()
    and JS::RemoveFooRoot() are going away, in favour of
    JS::PersistentRootedFoo. The keep-alive object has its own problems,
    because the SpiderMonkey 38 garbage collector will move GC things around,
    so anything referring to a GC thing on the keep-alive object will have to
    know when to update its JS::Heap wrapper when the GC thing moves.
    
    This previous situation existed in two places.
    
      (1) The JS::Value holding a trampoline's function. If the function was
      owned by the trampoline (the normal case), then it was rooted. If the
      function was a vfunc, in which case it was owned by the GObject class
      prototype and the trampoline was essentially leaked, then it remained a
      weak pointer.
    
      (2) The JSObject associated with a closure. In the normal case the
      object and the closure had the same lifetime and the object was rooted.
      Similar to above, if the closure was a signal it was owned by the
      GObject class, and traced.
    
    For both of these places we now use GjsMaybeOwned, a wrapper that sticks
    its GC thing in either a JS::PersistentRootedFoo, if the thing is
    intended to be rooted, or a JS::Heap<Foo>, if it is not. If rooted, the
    GjsMaybeOwned holds a weak reference to the GjsContext, and therefore
    can send out a notification when the context's dispose function is run,
    similar to existing functionality of the keep-alive object.
    
    This will still need to change further after the switch to SpiderMonkey
    38, since weak pointers must be updated when they are moved by the GC.
    (This is technically already the case in SpiderMonkey 31, but the API
    makes it difficult to do correctly, and in practice it isn't necessary.)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776966

 Makefile.am           |    1 +
 gi/closure.cpp        |   39 ++++-----
 gi/function.cpp       |   17 +---
 gi/function.h         |    5 +-
 gjs/context.cpp       |    6 +-
 gjs/jsapi-util-root.h |  215 +++++++++++++++++++++++++++++++++++++++++++++++++
 util/log.cpp          |    3 +
 util/log.h            |    1 +
 8 files changed, 249 insertions(+), 38 deletions(-)
---
diff --git a/Makefile.am b/Makefile.am
index cfe2089..0524bba 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -91,6 +91,7 @@ libgjs_la_SOURCES =           \
        gjs/jsapi-dynamic-class.cpp \
        gjs/jsapi-util-args.h           \
        gjs/jsapi-util-error.cpp        \
+       gjs/jsapi-util-root.h           \
        gjs/jsapi-util-string.cpp       \
        gjs/mem.cpp             \
        gjs/native.cpp          \
diff --git a/gi/closure.cpp b/gi/closure.cpp
index 3747296..f495189 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -28,14 +28,14 @@
 #include <util/log.h>
 
 #include "closure.h"
+#include "gjs/jsapi-util-root.h"
 #include "gjs/jsapi-wrapper.h"
 #include "gjs/mem.h"
-#include "keep-alive.h"
 
 struct Closure {
     JSRuntime *runtime;
     JSContext *context;
-    JS::Heap<JSObject *> obj;
+    GjsMaybeOwned<JSObject *> obj;
     guint unref_on_global_object_finalized : 1;
 };
 
@@ -91,7 +91,7 @@ invalidate_js_pointers(GjsClosure *gc)
     if (c->obj == NULL)
         return;
 
-    c->obj = NULL;
+    c->obj.reset();
     c->context = NULL;
     c->runtime = NULL;
 
@@ -102,8 +102,8 @@ invalidate_js_pointers(GjsClosure *gc)
 }
 
 static void
-global_context_finalized(JSObject *obj,
-                         void     *data)
+global_context_finalized(GjsMaybeOwned<JSObject *> *obj,
+                         void                      *data)
 {
     GjsClosure *gc = (GjsClosure*) data;
     Closure *c;
@@ -113,7 +113,7 @@ global_context_finalized(JSObject *obj,
 
     gjs_debug_closure("Context global object destroy notifier on closure %p "
                       "which calls object %p",
-                      c, c->obj);
+                      c, c->obj.get());
 
     /* invalidate_js_pointers() could free us so check flag now to avoid
      * invalid memory access
@@ -122,7 +122,7 @@ global_context_finalized(JSObject *obj,
     c->unref_on_global_object_finalized = false;
 
     if (c->obj != NULL) {
-        g_assert(c->obj == obj);
+        g_assert(c->obj == *obj);
 
         invalidate_js_pointers(gc);
     }
@@ -152,7 +152,7 @@ check_context_valid(GjsClosure *gc)
 
     gjs_debug_closure("Context %p no longer exists, invalidating "
                       "closure %p which calls object %p",
-                      c->context, c, c->obj);
+                      c->context, c, c->obj.get());
 
     /* Did not find the context. */
     invalidate_js_pointers(gc);
@@ -179,7 +179,7 @@ closure_invalidated(gpointer data,
 
     GJS_DEC_COUNTER(closure);
     gjs_debug_closure("Invalidating closure %p which calls object %p",
-                      closure, c->obj);
+                      closure, c->obj.get());
 
     if (c->obj == NULL) {
         gjs_debug_closure("   (closure %p already dead, nothing to do)",
@@ -234,12 +234,8 @@ closure_invalidated(gpointer data,
         gjs_debug_closure("   (closure %p's context was alive, "
                           "removing our destroy notifier on global object)",
                           closure);
-        gjs_keep_alive_remove_global_child(c->context,
-                                           global_context_finalized,
-                                           c->obj,
-                                           closure);
 
-        c->obj = NULL;
+        c->obj.reset();
         c->context = NULL;
         c->runtime = NULL;
     }
@@ -251,7 +247,7 @@ closure_set_invalid(gpointer  data,
 {
     Closure *self = &((GjsClosure*) closure)->priv;
 
-    self->obj = NULL;
+    self->obj.reset();
     self->context = NULL;
     self->runtime = NULL;
 
@@ -303,7 +299,7 @@ gjs_closure_invoke(GClosure                   *closure,
         /* Exception thrown... */
         gjs_debug_closure("Closure invocation failed (exception should "
                           "have been thrown) closure %p callable %p",
-                          closure, c->obj);
+                          closure, c->obj.get());
         if (!gjs_log_exception(context))
             gjs_debug_closure("Closure invocation failed but no exception was set?"
                               "closure %p", closure);
@@ -360,7 +356,7 @@ gjs_closure_trace(GClosure *closure,
     if (c->obj == NULL)
         return;
 
-    JS_CallHeapObjectTracer(tracer, &c->obj, "signal connection");
+    c->obj.trace(tracer, "signal connection");
 }
 
 GClosure*
@@ -384,20 +380,17 @@ gjs_closure_new(JSContext  *context,
     c->context = context;
     JS_BeginRequest(context);
 
-    c->obj = callable;
     c->unref_on_global_object_finalized = false;
 
     GJS_INC_COUNTER(closure);
 
     if (root_function) {
         /* Fully manage closure lifetime if so asked */
-        gjs_keep_alive_add_global_child(context,
-                                        global_context_finalized,
-                                        c->obj,
-                                        gc);
+        c->obj.root(context, callable, global_context_finalized, gc);
 
         g_closure_add_invalidate_notifier(&gc->base, NULL, closure_invalidated);
     } else {
+        c->obj = callable;
         /* Only mark the closure as invalid if memory is managed
            outside (i.e. by object.c for signals) */
         g_closure_add_invalidate_notifier(&gc->base, NULL, closure_set_invalid);
@@ -406,7 +399,7 @@ gjs_closure_new(JSContext  *context,
     g_closure_add_finalize_notifier(&gc->base, NULL, closure_finalize);
 
     gjs_debug_closure("Create closure %p which calls object %p '%s'",
-                      gc, c->obj, description);
+                      gc, c->obj.get(), description);
 
     JS_EndRequest(context);
 
diff --git a/gi/function.cpp b/gi/function.cpp
index dc14281..38d31a8 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -82,14 +82,6 @@ gjs_callback_trampoline_unref(GjsCallbackTrampoline *trampoline)
 
     trampoline->ref_count--;
     if (trampoline->ref_count == 0) {
-        JSContext *context = trampoline->context;
-
-        if (!trampoline->is_vfunc) {
-            JS_BeginRequest(context);
-            JS::RemoveValueRoot(context, &trampoline->js_function);
-            JS_EndRequest(context);
-        }
-
         g_callable_info_free_closure(trampoline->info, trampoline->closure);
         g_base_info_unref( (GIBaseInfo*) trampoline->info);
         g_free (trampoline->param_types);
@@ -208,7 +200,7 @@ gjs_callback_closure(ffi_cif *cif,
     }
 
     JS_BeginRequest(context);
-    func_obj = &trampoline->js_function.toObject();
+    func_obj = &trampoline->js_function.get().toObject();
     JSAutoCompartment ac(context, func_obj);
 
     n_args = g_callable_info_get_n_args(trampoline->info);
@@ -455,9 +447,10 @@ gjs_callback_trampoline_new(JSContext      *context,
     trampoline->context = context;
     trampoline->info = callable_info;
     g_base_info_ref((GIBaseInfo*)trampoline->info);
-    trampoline->js_function = function;
-    if (!is_vfunc)
-        JS::AddValueRoot(context, &trampoline->js_function);
+    if (is_vfunc)
+        trampoline->js_function = function;
+    else
+        trampoline->js_function.root(context, function);
 
     /* Analyze param types and directions, similarly to init_cached_function_data */
     n_args = g_callable_info_get_n_args(trampoline->info);
diff --git a/gi/function.h b/gi/function.h
index 8bc9cb1..4c0df03 100644
--- a/gi/function.h
+++ b/gi/function.h
@@ -28,6 +28,7 @@
 #include <glib.h>
 
 #include "gjs/jsapi-util.h"
+#include "gjs/jsapi-util-root.h"
 
 #include <girepository.h>
 #include <girffi.h>
@@ -45,7 +46,9 @@ struct GjsCallbackTrampoline {
     gint ref_count;
     JSContext *context;
     GICallableInfo *info;
-    JS::Heap<JS::Value> js_function;
+
+    GjsMaybeOwned<JS::Value> js_function;
+
     ffi_cif cif;
     ffi_closure *closure;
     GIScopeType scope;
diff --git a/gjs/context.cpp b/gjs/context.cpp
index 35693d8..cafa4a4 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -378,6 +378,10 @@ gjs_context_dispose(GObject *object)
 
     js_context = GJS_CONTEXT(object);
 
+    /* Run dispose notifications first, so that anything releasing
+     * references in response to this can still get garbage collected */
+    G_OBJECT_CLASS(gjs_context_parent_class)->dispose(object);
+
     if (js_context->context != NULL) {
 
         gjs_debug(GJS_DEBUG_CONTEXT,
@@ -413,8 +417,6 @@ gjs_context_dispose(GObject *object)
         js_context->context = NULL;
         g_clear_pointer(&js_context->runtime, gjs_runtime_unref);
     }
-
-    G_OBJECT_CLASS(gjs_context_parent_class)->dispose(object);
 }
 
 static void
diff --git a/gjs/jsapi-util-root.h b/gjs/jsapi-util-root.h
new file mode 100644
index 0000000..1ee1297
--- /dev/null
+++ b/gjs/jsapi-util-root.h
@@ -0,0 +1,215 @@
+/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (c) 2017 Endless Mobile, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef GJS_JSAPI_UTIL_ROOT_H
+#define GJS_JSAPI_UTIL_ROOT_H
+
+#include <glib.h>
+#include <glib-object.h>
+
+#include "gjs/context.h"
+#include "gjs/jsapi-wrapper.h"
+#include "util/log.h"
+
+template<typename T>
+struct GjsTraceHeap {
+    static void trace(JSTracer    *tracer,
+                      JS::Heap<T> *thing,
+                      const char  *name);
+};
+
+template<>
+struct GjsTraceHeap<JSObject *> {
+    static void
+    trace(JSTracer             *tracer,
+          JS::Heap<JSObject *> *thing,
+          const char           *name)
+    {
+        JS_CallHeapObjectTracer(tracer, thing, name);
+    }
+};
+
+template<typename T>
+class GjsMaybeOwned {
+public:
+    typedef void (*GjsOwnedNotify)(GjsMaybeOwned<T> *thing, void *data);
+
+private:
+    bool m_rooted;
+
+    JSContext *m_cx;
+    JS::Heap<T> m_heap;
+    JS::PersistentRooted<T> *m_root;
+
+    GjsOwnedNotify m_notify;
+    void *m_data;
+
+    enum ShouldRemoveWeakref {
+        NO,
+        YES,
+    };
+
+    static void
+    on_context_destroy(void    *data,
+                       GObject *ex_context)
+    {
+        auto self = static_cast<GjsMaybeOwned<T> *>(data);
+        self->invalidate();
+    }
+
+    void
+    teardown_rooting(ShouldRemoveWeakref unref)
+    {
+        gjs_debug_lifecycle(GJS_DEBUG_ROOT, "%p.teardown_rooting()", this);
+        g_assert(m_rooted);
+
+        // Preserve an unrooted ref to the GC thing, in case the notify
+        // callback wants to use it
+        m_heap = *m_root;
+        delete m_root;
+        m_root = nullptr;
+        m_rooted = false;
+
+        if (unref == NO)
+            return;
+
+        auto gjs_cx = static_cast<GjsContext *>(JS_GetContextPrivate(m_cx));
+        g_object_weak_unref(G_OBJECT(gjs_cx), on_context_destroy, this);
+    }
+
+    void
+    invalidate(void)
+    {
+        gjs_debug_lifecycle(GJS_DEBUG_ROOT, "%p.invalidate()", this);
+        g_assert(m_rooted);
+
+        // The weak ref is already gone because the context is dead
+        teardown_rooting(ShouldRemoveWeakref::NO);
+
+        // Safe for re-entry into the destructor from the callback
+        if (m_notify)
+            m_notify(this, m_data);
+
+        m_cx = nullptr;
+        m_notify = nullptr;
+        m_data = nullptr;
+    }
+
+public:
+    GjsMaybeOwned(void) :
+        m_rooted(false),
+        m_cx(nullptr),
+        m_root(nullptr),
+        m_notify(nullptr),
+        m_data(nullptr)
+    {
+        gjs_debug(GJS_DEBUG_ROOT, "%p.constructor()", this);
+    }
+
+    ~GjsMaybeOwned(void)
+    {
+        gjs_debug(GJS_DEBUG_ROOT, "%p.destructor()", this);
+        if (!m_rooted)
+            return;
+
+        teardown_rooting(ShouldRemoveWeakref::YES);
+    }
+
+    const T
+    get(void) const
+    {
+        return m_rooted ? m_root->get() : m_heap.get();
+    }
+    operator const T(void) const { return get(); }
+
+    bool
+    operator==(const T& other) const
+    {
+        if (m_rooted)
+            return m_root->get() == other;
+        return m_heap == other;
+    }
+    inline bool operator!=(const T& other) const { return !(*this == other); }
+
+    JS::Handle<T>
+    handle(void)
+    {
+        g_assert(m_rooted);
+        return *m_root;
+    }
+
+    void
+    root(JSContext     *cx,
+         const T&       thing,
+         GjsOwnedNotify notify = nullptr,
+         void           *data  = nullptr)
+    {
+        gjs_debug_lifecycle(GJS_DEBUG_ROOT, "%p.root()", this);
+        g_assert(!m_rooted);
+        m_rooted = true;
+        m_cx = cx;
+        m_notify = notify;
+        m_data = data;
+        m_root = new JS::PersistentRooted<T>(m_cx, thing);
+
+        auto gjs_cx = static_cast<GjsContext *>(JS_GetContextPrivate(m_cx));
+        g_assert(GJS_IS_CONTEXT(gjs_cx));
+        g_object_weak_ref(G_OBJECT(gjs_cx), on_context_destroy, this);
+    }
+
+    void
+    operator=(const T& thing)
+    {
+        if (m_rooted)
+            *m_root = thing;
+        else
+            m_heap = thing;
+    }
+
+    void
+    reset(void)
+    {
+        gjs_debug_lifecycle(GJS_DEBUG_ROOT, "%p.reset()", this);
+        if (!m_rooted) {
+            m_heap.set(js::GCMethods<T>::initial());
+            return;
+        }
+
+        teardown_rooting(ShouldRemoveWeakref::YES);
+
+        m_cx = nullptr;
+        m_notify = nullptr;
+        m_data = nullptr;
+    }
+
+    void
+    trace(JSTracer   *tracer,
+          const char *name)
+    {
+        gjs_debug_lifecycle(GJS_DEBUG_ROOT, "%p.trace()", this);
+        g_assert(!m_rooted);
+        GjsTraceHeap<T>::trace(tracer, &m_heap, name);
+    }
+};
+
+#endif /* GJS_JSAPI_UTIL_ROOT_H */
diff --git a/util/log.cpp b/util/log.cpp
index bacb12b..04bf098 100644
--- a/util/log.cpp
+++ b/util/log.cpp
@@ -260,6 +260,9 @@ _Pragma("GCC diagnostic pop")
     case GJS_DEBUG_PROXY:
         prefix = "JS CPROXY";
         break;
+    case GJS_DEBUG_ROOT:
+        prefix = "JS ROOT";
+        break;
     default:
         prefix = "???";
         break;
diff --git a/util/log.h b/util/log.h
index 98a790c..f4fea27 100644
--- a/util/log.h
+++ b/util/log.h
@@ -60,6 +60,7 @@ typedef enum {
     GJS_DEBUG_GERROR,
     GJS_DEBUG_GFUNDAMENTAL,
     GJS_DEBUG_PROXY,
+    GJS_DEBUG_ROOT,
 } GjsDebugTopic;
 
 /* These defines are because we have some pretty expensive and