[gjs: 4/6] GjsMaybeOwned: Remove notifier support and move it into GjsPrivateContext

[Philip Chimento <pchimento src gnome org>]

commit 2fc0d36062c805aaa28a8e63763fafbe4d25cf75
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Sun May 16 18:02:10 2021 +0200

    GjsMaybeOwned: Remove notifier support and move it into GjsPrivateContext
    
    GjsMaybeOwned had a notification support that was used only by Closures
    (other than tests), this was causing adding an extra pointer to all
    the types using it (including Object) that was mostly unused.
    
    So, move this into private context, re-implementing it using our own
    notifier (instead of relying on GObject's that needs a specific order) and
    use it in closures.
    
    Also add an hook to handle gc events on private context that will be in
    later changes to cleanup more things, but for now is used to shrink the
    closures vector once we've done.

 gi/closure.cpp            | 35 ++++++++++++++------------
 gi/closure.h              | 10 ++++++--
 gi/function.cpp           |  2 ++
 gi/object.cpp             |  2 +-
 gjs/context-private.h     |  9 +++++++
 gjs/context.cpp           | 28 ++++++++++++++++++---
 gjs/jsapi-util-root.h     | 62 +++--------------------------------------------
 test/gjs-test-rooting.cpp | 11 ++++++---
 8 files changed, 75 insertions(+), 84 deletions(-)
---
diff --git a/gi/closure.cpp b/gi/closure.cpp
index 3f15d078..8647e956 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -32,12 +32,8 @@ Closure::Closure(JSContext* cx, JSFunction* callable, bool root,
         // Fully manage closure lifetime if so asked
         auto* gjs = GjsContextPrivate::from_cx(cx);
         g_assert(cx == gjs->context());
-        m_func.root(
-            cx, callable,
-            [](JS::HandleFunction, void* data) {
-                static_cast<Closure*>(data)->global_context_finalized();
-            },
-            this);
+        m_func.root(cx, callable);
+        gjs->register_notifier(global_context_notifier_cb, this);
         closure_notify = [](void*, GClosure* closure) {
             static_cast<Closure*>(closure)->closure_invalidated();
         };
@@ -90,16 +86,16 @@ Closure::Closure(JSContext* cx, JSFunction* callable, bool root,
  *
  */
 
-void Closure::invalidate_js_pointers() {
-    if (!m_func)
+void Closure::unset_context() {
+    if (!m_cx)
         return;
 
-    reset();
+    if (m_func && m_func.rooted()) {
+        auto* gjs = GjsContextPrivate::from_cx(m_cx);
+        gjs->unregister_notifier(global_context_notifier_cb, this);
+    }
 
-    /* Notify any closure reference holders they
-     * may want to drop references.
-     */
-    g_closure_invalidate(this);
+    m_cx = nullptr;
 }
 
 void Closure::global_context_finalized() {
@@ -108,15 +104,22 @@ void Closure::global_context_finalized() {
         "object %p",
         this, m_func.debug_addr());
 
-    if (m_func)
-        invalidate_js_pointers();
+    if (m_func) {
+        // Manually unset the context as we don't need to unregister the
+        // notifier here, or we'd end up touching a vector we're iterating
+        m_cx = nullptr;
+        reset();
+        // Notify any closure reference holders they
+        // may want to drop references.
+        g_closure_invalidate(this);
+    }
 }
 
 /* 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
  * source (the signal connection, say can be removed.) The usage above
- * in invalidate_js_pointers() is typical. Since the target of the closure
+ * in global_context_finalized() is typical. Since the target of the closure
  * is under our control, it's unlikely that g_closure_invalidate() will ever
  * be called by anyone else, but in case it ever does, it's slightly better
  * to remove the "keep alive" here rather than in the finalize notifier.
diff --git a/gi/closure.h b/gi/closure.h
index cfc546ee..a9110fc5 100644
--- a/gi/closure.h
+++ b/gi/closure.h
@@ -30,7 +30,7 @@ namespace Gjs {
 class Closure : public GClosure {
  protected:
     Closure(JSContext*, JSFunction*, bool root, const char* description);
-    ~Closure() = default;
+    ~Closure() { unset_context(); }
 
     // Need to call this if inheriting from Closure to call the dtor
     template <class C>
@@ -100,7 +100,10 @@ class Closure : public GClosure {
     }
 
  private:
+    void unset_context();
+
     void reset() {
+        unset_context();
         m_func.reset();
         m_cx = nullptr;
     }
@@ -110,9 +113,12 @@ class Closure : public GClosure {
         for_gclosure(closure)->marshal(ret, n_params, params, hint, data);
     }
 
+    static void global_context_notifier_cb(JSContext*, void* data) {
+        static_cast<Closure*>(data)->global_context_finalized();
+    }
+
     void closure_invalidated();
     void closure_set_invalid();
-    void invalidate_js_pointers();
     void global_context_finalized();
     void marshal(GValue* ret, unsigned n_parms, const GValue* params,
                  void* hint, void* data);
diff --git a/gi/function.cpp b/gi/function.cpp
index c4591144..672a4464 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -337,6 +337,8 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) {
             if (trampoline->m_scope == GI_SCOPE_TYPE_ASYNC) {
                 // We don't release the trampoline here as we've an extra ref
                 // that has been set in gjs_marshal_callback_in()
+                gjs_debug_closure("Saving async closure for gc cleanup %p",
+                                  trampoline);
                 completed_trampolines.emplace_back(trampoline);
             }
             gjs->schedule_gc_if_needed();
diff --git a/gi/object.cpp b/gi/object.cpp
index 2d5519a6..e0241717 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -74,7 +74,7 @@ class JSTracer;
 #if defined(__x86_64__) && defined(__clang__)
 /* This isn't meant to be comprehensive, but should trip on at least one CI job
  * if sizeof(ObjectInstance) is increased. */
-static_assert(sizeof(ObjectInstance) <= 88,
+static_assert(sizeof(ObjectInstance) <= 64,
               "Think very hard before increasing the size of ObjectInstance. "
               "There can be tens of thousands of them alive in a typical "
               "gnome-shell run.");
diff --git a/gjs/context-private.h b/gjs/context-private.h
index 4884f4a4..b472ef4f 100644
--- a/gjs/context-private.h
+++ b/gjs/context-private.h
@@ -14,6 +14,7 @@
 #include <string>
 #include <thread>
 #include <unordered_map>
+#include <utility>  // for pair
 #include <vector>
 
 #include <glib-object.h>
@@ -54,6 +55,10 @@ using GTypeTable =
                   js::SystemAllocPolicy>;
 
 class GjsContextPrivate : public JS::JobQueue {
+ public:
+    using DestroyNotify = void (*)(JSContext*, void* data);
+
+ private:
     GjsContext* m_public_context;
     JSContext* m_cx;
     JS::Heap<JSObject*> m_global;
@@ -74,6 +79,7 @@ class GjsContextPrivate : public JS::JobQueue {
     JobQueueStorage m_job_queue;
     unsigned m_idle_drain_handler;
 
+    std::vector<std::pair<DestroyNotify, void*>> m_destroy_notifications;
     std::unordered_map<uint64_t, GjsAutoChar> m_unhandled_rejection_stacks;
 
     GjsProfiler* m_profiler;
@@ -238,6 +244,9 @@ class GjsContextPrivate : public JS::JobQueue {
     void register_unhandled_promise_rejection(uint64_t id, GjsAutoChar&& stack);
     void unregister_unhandled_promise_rejection(uint64_t id);
 
+    void register_notifier(DestroyNotify notify_func, void* data);
+    void unregister_notifier(DestroyNotify notify_func, void* data);
+
     [[nodiscard]] bool register_module(const char* identifier,
                                        const char* filename, GError** error);
 
diff --git a/gjs/context.cpp b/gjs/context.cpp
index ce0692b1..e5a01fcf 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -15,6 +15,9 @@
 #    include <process.h>
 #endif
 
+#ifdef DEBUG
+#    include <algorithm>  // for find
+#endif
 #include <atomic>
 #include <new>
 #include <string>       // for u16string
@@ -57,10 +60,12 @@
 #include <jsfriendapi.h>  // for DumpHeap, IgnoreNurseryObjects
 #include <mozilla/UniquePtr.h>
 
+#include "gi/closure.h"  // for Closure::Ptr, Closure
 #include "gi/function.h"
 #include "gi/object.h"
 #include "gi/private.h"
 #include "gi/repo.h"
+#include "gi/utils-inl.h"
 #include "gjs/atoms.h"
 #include "gjs/byteArray.h"
 #include "gjs/context-private.h"
@@ -374,10 +379,8 @@ gjs_context_dispose(GObject *object)
     if (gjs->context())
         ObjectInstance::context_dispose_notify(nullptr, object);
 
-    /* Run dispose notifications next, so that anything releasing
-     * references in response to this can still get garbage collected */
     gjs_debug(GJS_DEBUG_CONTEXT,
-              "Notifying reference holders of GjsContext dispose");
+              "Notifying external reference holders of GjsContext dispose");
     G_OBJECT_CLASS(gjs_context_parent_class)->dispose(object);
 
     gjs->dispose();
@@ -389,8 +392,25 @@ void GjsContextPrivate::free_profiler(void) {
         g_clear_pointer(&m_profiler, _gjs_profiler_free);
 }
 
+void GjsContextPrivate::register_notifier(DestroyNotify notify_func,
+                                          void* data) {
+    m_destroy_notifications.push_back({notify_func, data});
+}
+
+void GjsContextPrivate::unregister_notifier(DestroyNotify notify_func,
+                                            void* data) {
+    auto target = std::make_pair(notify_func, data);
+    Gjs::remove_one_from_unsorted_vector(&m_destroy_notifications, target);
+}
+
 void GjsContextPrivate::dispose(void) {
     if (m_cx) {
+        gjs_debug(GJS_DEBUG_CONTEXT,
+                  "Notifying reference holders of GjsContext dispose");
+
+        for (auto const& destroy_notify : m_destroy_notifications)
+            destroy_notify.first(m_cx, destroy_notify.second);
+
         gjs_debug(GJS_DEBUG_CONTEXT,
                   "Checking unhandled promise rejections");
         warn_about_unhandled_promise_rejections();
@@ -784,6 +804,8 @@ void GjsContextPrivate::on_garbage_collection(JSGCStatus status, JS::GCReason re
             }
             m_gc_begin_time = 0;
             m_gc_reason = nullptr;
+
+            m_destroy_notifications.shrink_to_fit();
             gjs_debug_lifecycle(GJS_DEBUG_CONTEXT, "End garbage collection");
             break;
         default:
diff --git a/gjs/jsapi-util-root.h b/gjs/jsapi-util-root.h
index 5433a09b..bc58ba36 100644
--- a/gjs/jsapi-util-root.h
+++ b/gjs/jsapi-util-root.h
@@ -47,7 +47,7 @@
  *
  * If the thing is rooted, it will be unrooted either when the GjsMaybeOwned is
  * destroyed, or when the JSContext is destroyed. In the latter case, you can
- * get an optional notification by passing a callback to root().
+ * get an optional notification by registering a callback in the PrivateContext.
  *
  * To switch between one of the three modes, you must first call reset(). This
  * drops all references to any GC thing and leaves the GjsMaybeOwned in the
@@ -98,9 +98,6 @@ struct GjsHeapOperation<JSFunction*> {
  * any instances of classes that have it as a member on the stack either. */
 template<typename T>
 class GjsMaybeOwned {
- public:
-    typedef void (*DestroyNotify)(JS::Handle<T> thing, void *data);
-
  private:
     /* m_root value controls which of these members we can access. When switching
      * from one to the other, be careful to call the constructor and destructor
@@ -108,44 +105,6 @@ class GjsMaybeOwned {
     JS::Heap<T> m_heap;
     std::unique_ptr<JS::PersistentRooted<T>> m_root;
 
-    struct Notifier {
-        Notifier(GjsMaybeOwned<T> *parent, DestroyNotify func, void *data)
-            : m_parent(parent)
-            , m_func(func)
-            , m_data(data)
-        {
-            GjsContext* current = gjs_context_get_current();
-            g_assert(GJS_IS_CONTEXT(current));
-            g_object_weak_ref(G_OBJECT(current), on_context_destroy, this);
-        }
-
-        ~Notifier() { disconnect(); }
-
-        static void on_context_destroy(void* data,
-                                       GObject* ex_context [[maybe_unused]]) {
-            auto self = static_cast<Notifier*>(data);
-            auto *parent = self->m_parent;
-            self->m_parent = nullptr;
-            self->m_func(parent->handle(), self->m_data);
-        }
-
-        void disconnect() {
-            if (!m_parent)
-                return;
-
-            GjsContext* current = gjs_context_get_current();
-            g_assert(GJS_IS_CONTEXT(current));
-            g_object_weak_unref(G_OBJECT(current), on_context_destroy, this);
-            m_parent = nullptr;
-        }
-
-     private:
-        GjsMaybeOwned<T> *m_parent;
-        DestroyNotify m_func;
-        void *m_data;
-    };
-    std::unique_ptr<Notifier> m_notify;
-
     /* No-op unless GJS_VERBOSE_ENABLE_LIFECYCLE is defined to 1. */
     inline void debug(const char* what GJS_USED_VERBOSE_LIFECYCLE) {
         gjs_debug_lifecycle(GJS_DEBUG_KEEP_ALIVE, "GjsMaybeOwned %p %s", this,
@@ -159,7 +118,6 @@ class GjsMaybeOwned {
         g_assert(m_root);
 
         m_root.reset();
-        m_notify.reset();
 
         new (&m_heap) JS::Heap<T>();
     }
@@ -222,20 +180,12 @@ class GjsMaybeOwned {
 
     /* Roots the GC thing. You must not use this if you're already using the
      * wrapper to store a non-rooted GC thing. */
-    void
-    root(JSContext    *cx,
-         const T&      thing,
-         DestroyNotify notify = nullptr,
-         void         *data   = nullptr)
-    {
+    void root(JSContext* cx, const T& thing) {
         debug("root()");
         g_assert(!m_root);
         g_assert(m_heap.get() == JS::SafelyInitialized<T>());
         m_heap.~Heap();
         m_root = std::make_unique<JS::PersistentRooted<T>>(cx, thing);
-
-        if (notify)
-            m_notify = std::make_unique<Notifier>(this, notify, data);
     }
 
     /* You can only assign directly to the GjsMaybeOwned wrapper in the
@@ -266,11 +216,7 @@ class GjsMaybeOwned {
         teardown_rooting();
     }
 
-    void
-    switch_to_rooted(JSContext    *cx,
-                     DestroyNotify notify = nullptr,
-                     void         *data   = nullptr)
-    {
+    void switch_to_rooted(JSContext* cx) {
         debug("switch to rooted");
         g_assert(!m_root);
 
@@ -279,7 +225,7 @@ class GjsMaybeOwned {
         JS::Rooted<T> thing(cx, m_heap);
 
         reset();
-        root(cx, thing, notify, data);
+        root(cx, thing);
         g_assert(m_root);
     }
 
diff --git a/test/gjs-test-rooting.cpp b/test/gjs-test-rooting.cpp
index cb878554..c3c89eb3 100644
--- a/test/gjs-test-rooting.cpp
+++ b/test/gjs-test-rooting.cpp
@@ -3,7 +3,6 @@
 
 #include <config.h>
 
-#include <glib-object.h>  // for g_object_weak_unref, g_object_weak_ref
 #include <glib.h>
 
 #include <js/Class.h>
@@ -224,7 +223,7 @@ static void test_maybe_owned_switch_to_unrooted_allows_collection(
     delete obj;
 }
 
-static void context_destroyed(JS::HandleObject, void* data) {
+static void context_destroyed(JSContext*, void* data) {
     auto fx = static_cast<GjsRootingFixture *>(data);
     g_assert_false(fx->notify_called);
     g_assert_false(fx->finalized);
@@ -234,8 +233,10 @@ static void context_destroyed(JS::HandleObject, void* data) {
 
 static void test_maybe_owned_notify_callback_called_on_context_destroy(
     GjsRootingFixture* fx, const void*) {
+    auto* gjs = GjsContextPrivate::from_cx(PARENT(fx)->cx);
     fx->obj = new GjsMaybeOwned<JSObject *>();
-    fx->obj->root(PARENT(fx)->cx, test_obj_new(fx), context_destroyed, fx);
+    fx->obj->root(PARENT(fx)->cx, test_obj_new(fx));
+    gjs->register_notifier(context_destroyed, fx);
 
     gjs_unit_test_destroy_context(PARENT(fx));
     g_assert_true(fx->notify_called);
@@ -244,8 +245,10 @@ static void test_maybe_owned_notify_callback_called_on_context_destroy(
 
 static void test_maybe_owned_object_destroyed_after_notify(
     GjsRootingFixture* fx, const void*) {
+    auto* gjs = GjsContextPrivate::from_cx(PARENT(fx)->cx);
     fx->obj = new GjsMaybeOwned<JSObject *>();
-    fx->obj->root(PARENT(fx)->cx, test_obj_new(fx), context_destroyed, fx);
+    fx->obj->root(PARENT(fx)->cx, test_obj_new(fx));
+    gjs->register_notifier(context_destroyed, fx);
 
     gjs_unit_test_destroy_context(PARENT(fx));
     g_assert_true(fx->finalized);