[gjs: 4/10] fundamental: Fix broken hash table

[Cosimo Cecchi <cosimoc src gnome org>]

commit 105a20a94b3d9bd76f577ca42836936399c281c7
Author: Philip Chimento <philip chimento gmail com>
Date:   Wed Jan 2 17:18:33 2019 -0700

    fundamental: Fix broken hash table
    
    There was a hash table (attached to the GjsContext using qdata) which
    stored a mapping of void* fundamental instance pointers to JSObject
    instances. This would have broken quite badly as soon as any of those
    JSObjects were moved by the garbage collector.
    
    Instead we use a JS::GCHashMap, and since it was previously attached to
    the GjsContext anyway we go ahead and make it a member of
    GjsContextPrivate. We put it inside a JS::WeakCache so that it will be
    automatically swept when the objects are garbage collected.
    
    The WeakCache must be destroyed before calling JS_DestroyContext() or we
    will fail an assertion in debug mode.

 gi/fundamental.cpp    | 87 +++++++++++----------------------------------------
 gi/fundamental.h      |  4 ++-
 gjs/context-private.h | 18 +++++++++++
 gjs/context.cpp       |  8 +++++
 4 files changed, 47 insertions(+), 70 deletions(-)
---
diff --git a/gi/fundamental.cpp b/gi/fundamental.cpp
index c7f165df..8bf25681 100644
--- a/gi/fundamental.cpp
+++ b/gi/fundamental.cpp
@@ -33,6 +33,7 @@
 #include "gi/object.h"
 #include "gi/repo.h"
 #include "gi/wrapperutils.h"
+#include "gjs/context-private.h"
 #include "gjs/jsapi-class.h"
 #include "gjs/jsapi-wrapper.h"
 #include "gjs/mem-private.h"
@@ -41,63 +42,6 @@
 #include <util/log.h>
 #include <girepository.h>
 
-GJS_USE
-static GQuark
-gjs_fundamental_table_quark (void)
-{
-    static GQuark val = 0;
-    if (!val)
-        val = g_quark_from_static_string("gjs::fundamental-table");
-
-    return val;
-}
-
-GJS_USE
-static GHashTable *
-_ensure_mapping_table(GjsContext *context)
-{
-    GHashTable *table =
-        (GHashTable *) g_object_get_qdata ((GObject *) context,
-                                           gjs_fundamental_table_quark());
-
-    if (G_UNLIKELY(table == NULL)) {
-        table = g_hash_table_new(NULL, NULL);
-        g_object_set_qdata_full((GObject *) context,
-                                gjs_fundamental_table_quark(),
-                                table,
-                                (GDestroyNotify) g_hash_table_unref);
-    }
-
-    return table;
-}
-
-static void
-_fundamental_add_object(void *native_object, JSObject *js_object)
-{
-    GHashTable *table = _ensure_mapping_table(gjs_context_get_current());
-
-    g_hash_table_insert(table, native_object, js_object);
-}
-
-static void
-_fundamental_remove_object(void *native_object)
-{
-    GHashTable *table = _ensure_mapping_table(gjs_context_get_current());
-
-    g_hash_table_remove(table, native_object);
-}
-
-GJS_USE
-static JSObject *
-_fundamental_lookup_object(void *native_object)
-{
-    GHashTable *table = _ensure_mapping_table(gjs_context_get_current());
-
-    return (JSObject *) g_hash_table_lookup(table, native_object);
-}
-
-/**/
-
 FundamentalInstance::FundamentalInstance(JSContext* cx, JS::HandleObject obj)
     : GIWrapperInstance(cx, obj) {
     GJS_INC_COUNTER(fundamental_instance);
@@ -110,16 +54,20 @@ FundamentalInstance::FundamentalInstance(JSContext* cx, JS::HandleObject obj)
  * future if you have a pointer to @gfundamental. (Assuming @object has not been
  * garbage collected in the meantime.)
  */
-void FundamentalInstance::associate_js_instance(JSObject* object,
+bool FundamentalInstance::associate_js_instance(JSContext* cx, JSObject* object,
                                                 void* gfundamental) {
     m_ptr = gfundamental;
 
-    g_assert(_fundamental_lookup_object(gfundamental) == NULL);
-    _fundamental_add_object(gfundamental, object);
+    GjsContextPrivate* gjs = GjsContextPrivate::from_cx(cx);
+    if (!gjs->fundamental_table().putNew(gfundamental, object)) {
+        JS_ReportOutOfMemory(cx);
+        return false;
+    }
 
     debug_lifecycle(object, "associated JSObject with fundamental");
 
     ref();
+    return true;
 }
 
 /**/
@@ -275,11 +223,10 @@ bool FundamentalInstance::constructor_impl(JSContext* cx,
     GArgument ret_value;
     GITypeInfo return_info;
 
-    if (!invoke_constructor(cx, object, argv, &ret_value))
+    if (!invoke_constructor(cx, object, argv, &ret_value) ||
+        !associate_js_instance(cx, object, ret_value.v_pointer))
         return false;
 
-    associate_js_instance(object, ret_value.v_pointer);
-
     GICallableInfo* constructor_info = get_prototype()->constructor_info();
     g_callable_info_load_return_type(constructor_info, &return_info);
 
@@ -290,7 +237,6 @@ bool FundamentalInstance::constructor_impl(JSContext* cx,
 
 FundamentalInstance::~FundamentalInstance(void) {
     if (m_ptr) {
-        _fundamental_remove_object(m_ptr);
         unref();
         m_ptr = nullptr;
     }
@@ -480,9 +426,10 @@ gjs_object_from_g_fundamental(JSContext    *context,
     if (gfundamental == NULL)
         return NULL;
 
-    JS::RootedObject object(context, _fundamental_lookup_object(gfundamental));
-    if (object)
-        return object;
+    GjsContextPrivate* gjs = GjsContextPrivate::from_cx(context);
+    auto p = gjs->fundamental_table().lookup(gfundamental);
+    if (p)
+        return p->value();
 
     gjs_debug_marshal(GJS_DEBUG_GFUNDAMENTAL,
                       "Wrapping fundamental %s.%s %p with JSObject",
@@ -496,14 +443,16 @@ gjs_object_from_g_fundamental(JSContext    *context,
     if (!proto)
         return NULL;
 
-    object = JS_NewObjectWithGivenProto(context, JS_GetClass(proto), proto);
+    JS::RootedObject object(context, JS_NewObjectWithGivenProto(
+                                         context, JS_GetClass(proto), proto));
 
     if (!object)
         return nullptr;
 
     auto* priv = FundamentalInstance::new_for_js_object(context, object);
 
-    priv->associate_js_instance(object, gfundamental);
+    if (!priv->associate_js_instance(context, object, gfundamental))
+        return nullptr;
 
     return object;
 }
diff --git a/gi/fundamental.h b/gi/fundamental.h
index 83a5147c..44d9850b 100644
--- a/gi/fundamental.h
+++ b/gi/fundamental.h
@@ -145,7 +145,9 @@ class FundamentalInstance
                           const JS::CallArgs& args);
 
  public:
-    void associate_js_instance(JSObject* object, void* gfundamental);
+    GJS_JSAPI_RETURN_CONVENTION
+    bool associate_js_instance(JSContext* cx, JSObject* object,
+                               void* gfundamental);
 };
 
 G_BEGIN_DECLS
diff --git a/gjs/context-private.h b/gjs/context-private.h
index 3b499c90..566be990 100644
--- a/gjs/context-private.h
+++ b/gjs/context-private.h
@@ -33,7 +33,19 @@
 #include "jsapi-util.h"
 #include "jsapi-wrapper.h"
 
+#include "js/GCHashTable.h"
+#include "js/SweepingAPI.h"
+
 using JobQueue = JS::GCVector<JSObject*, 0, js::SystemAllocPolicy>;
+using FundamentalTable =
+    JS::GCHashMap<void*, JS::Heap<JSObject*>, js::DefaultHasher<void*>,
+                  js::SystemAllocPolicy>;
+
+// FundamentalTable's key type is void*, the GC sweep method should ignore it
+namespace JS {
+template <>
+struct GCPolicy<void*> : public IgnoreGCPolicy<void*> {};
+}  // namespace JS
 
 class GjsContextPrivate {
     GjsContext* m_public_context;
@@ -69,6 +81,9 @@ class GjsContextPrivate {
     };
     EnvironmentPreparer m_environment_preparer;
 
+    // Weak pointer mapping from fundamental native pointer to JSObject
+    JS::WeakCache<FundamentalTable>* m_fundamental_table;
+
     uint8_t m_exit_code;
 
     /* flags */
@@ -122,6 +137,9 @@ class GjsContextPrivate {
     GJS_USE bool is_owner_thread(void) const {
         return m_owner_thread == g_thread_self();
     }
+    GJS_USE JS::WeakCache<FundamentalTable>& fundamental_table(void) {
+        return *m_fundamental_table;
+    }
     GJS_USE
     static const GjsAtoms& atoms(JSContext* cx) { return from_cx(cx)->m_atoms; }
 
diff --git a/gjs/context.cpp b/gjs/context.cpp
index 7ef0378c..922cd7bb 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -342,6 +342,8 @@ void GjsContextPrivate::dispose(void) {
 
         JS_BeginRequest(m_cx);
 
+        gjs_debug(GJS_DEBUG_CONTEXT, "Releasing cached JS wrappers");
+        m_fundamental_table->clear();
         /* Do a full GC here before tearing down, since once we do
          * that we may not have the JS_GetPrivate() to access the
          * context
@@ -372,6 +374,7 @@ void GjsContextPrivate::dispose(void) {
 
         gjs_debug(GJS_DEBUG_CONTEXT, "Freeing allocated resources");
         delete m_job_queue;
+        delete m_fundamental_table;
 
         /* Tear down JS */
         JS_DestroyContext(m_cx);
@@ -451,6 +454,11 @@ GjsContextPrivate::GjsContextPrivate(JSContext* cx, GjsContext* public_context)
     if (!m_job_queue)
         g_error("Failed to initialize promise job queue");
 
+    JSRuntime* rt = JS_GetRuntime(m_cx);
+    m_fundamental_table = new JS::WeakCache<FundamentalTable>(rt);
+    if (!m_fundamental_table->init())
+        g_error("Failed to initialize fundamental objects table");
+
     JS_BeginRequest(m_cx);
 
     JS::RootedObject global(m_cx, gjs_create_global_object(m_cx));