[gjs/217-separate-closures-and-vfuncs: 2/2] object: Separate closures and vfuncs

[Philip Chimento <pchimento src gnome org>]

commit 15be587eaff66a0df92d953ded70d89cbf18b719
Author: Philip Chimento <philip chimento gmail com>
Date:   Fri Mar 6 22:09:13 2020 -0800

    object: Separate closures and vfuncs
    
    Previously, signal connections, pending notify callbacks, and vfuncs
    were stored in one list that was a member of ObjectBase. However, only
    ObjectPrototypes have vfuncs, and only ObjectInstances have signal
    connections and callbacks. This commit separates them, so that later we
    can move the signal connections and callbacks to expando objects as part
    of #217.

 gi/function.cpp |  2 +-
 gi/gobject.cpp  |  4 ++--
 gi/object.cpp   | 54 ++++++++++++++++++++++++++++++++++++------------------
 gi/object.h     | 44 ++++++++++++++++++++------------------------
 4 files changed, 59 insertions(+), 45 deletions(-)
---
diff --git a/gi/function.cpp b/gi/function.cpp
index 02735cfa..bd59c479 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -971,7 +971,7 @@ static bool gjs_invoke_c_function(JSContext* context, Function* function,
                         break;
                     }
                     if (scope == GI_SCOPE_TYPE_NOTIFIED && is_object_method) {
-                        ObjectBase* priv = ObjectBase::for_js(context, obj);
+                        auto* priv = ObjectInstance::for_js(context, obj);
                         if (!priv) {
                             gjs_throw(
                                 context,
diff --git a/gi/gobject.cpp b/gi/gobject.cpp
index 454bff15..5e16d77c 100644
--- a/gi/gobject.cpp
+++ b/gi/gobject.cpp
@@ -78,13 +78,13 @@ static bool jsobj_set_gproperty(JSContext* cx, JS::HandleObject object,
 static void gjs_object_base_init(void* klass) {
     auto* priv = ObjectPrototype::for_gtype(G_OBJECT_CLASS_TYPE(klass));
     if (priv)
-        priv->ref_closures();
+        priv->ref_vfuncs();
 }
 
 static void gjs_object_base_finalize(void* klass) {
     auto* priv = ObjectPrototype::for_gtype(G_OBJECT_CLASS_TYPE(klass));
     if (priv)
-        priv->unref_closures();
+        priv->unref_vfuncs();
 }
 
 static GObject* gjs_object_constructor(
diff --git a/gi/object.cpp b/gi/object.cpp
index d92441ba..925f9e38 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1448,16 +1448,18 @@ ObjectInstance::ensure_uses_toggle_ref(JSContext *cx)
     g_object_unref(m_ptr);
 }
 
-void ObjectBase::invalidate_all_closures(void) {
-    /* Can't loop directly through the items, since invalidating an item's
-     * closure might have the effect of removing the item from the set in the
-     * invalidate notifier */
-    while (!m_closures.empty()) {
-        /* This will also free cd, through the closure invalidation mechanism */
-        GClosure *closure = *m_closures.begin();
+static void invalidate_closure_list(std::forward_list<GClosure*>* closures) {
+    g_assert(closures);
+    // Can't loop directly through the items, since invalidating an item's
+    // closure might have the effect of removing the item from the list in the
+    // invalidate notifier
+    while (!closures->empty()) {
+        // This will also free the closure data, through the closure
+        // invalidation mechanism
+        GClosure* closure = *closures->begin();
         g_closure_invalidate(closure);
         /* Erase element if not already erased */
-        m_closures.remove(closure);
+        closures->remove(closure);
     }
 }
 
@@ -1484,7 +1486,7 @@ ObjectInstance::disassociate_js_gobject(void)
     unset_object_qdata();
 
     /* Now release all the resources the current wrapper has */
-    invalidate_all_closures();
+    invalidate_closure_list(&m_closures);
     release_native_object();
 
     /* Mark that a JS object once existed, but it doesn't any more */
@@ -1612,7 +1614,7 @@ bool ObjectInstance::constructor_impl(JSContext* context,
            gjs->call_function(object, initer, argv, argv.rval());
 }
 
-void ObjectBase::trace_impl(JSTracer* tracer) {
+void ObjectInstance::trace_impl(JSTracer* tracer) {
     for (GClosure *closure : m_closures)
         gjs_closure_trace(closure, tracer);
 }
@@ -1621,6 +1623,8 @@ void ObjectPrototype::trace_impl(JSTracer* tracer) {
     m_property_cache.trace(tracer);
     m_field_cache.trace(tracer);
     m_unresolvable_cache.trace(tracer);
+    for (GClosure* closure : m_vfuncs)
+        gjs_closure_trace(closure, tracer);
 }
 
 void ObjectInstance::finalize_impl(JSFreeOp* fop, JSObject* obj) {
@@ -1636,7 +1640,7 @@ void ObjectInstance::finalize_impl(JSFreeOp* fop, JSObject* obj) {
 ObjectInstance::~ObjectInstance() {
     TRACE(GJS_OBJECT_WRAPPER_FINALIZE(this, m_ptr, ns(), name()));
 
-    invalidate_all_closures();
+    invalidate_closure_list(&m_closures);
 
     /* GObject is not already freed */
     if (m_ptr) {
@@ -1683,7 +1687,7 @@ ObjectInstance::~ObjectInstance() {
 }
 
 ObjectPrototype::~ObjectPrototype() {
-    invalidate_all_closures();
+    invalidate_closure_list(&m_vfuncs);
 
     g_clear_pointer(&m_info, g_base_info_unref);
     g_type_class_unref(g_type_class_peek(m_gtype));
@@ -1802,7 +1806,7 @@ GIFieldInfo* ObjectPrototype::lookup_cached_field_info(JSContext* cx,
     return parent->lookup_cached_field_info(cx, key);
 }
 
-void ObjectBase::associate_closure(JSContext* cx, GClosure* closure) {
+void ObjectInstance::associate_closure(JSContext* cx, GClosure* closure) {
     if (!is_prototype())
         to_instance()->ensure_uses_toggle_ref(cx);
 
@@ -1812,12 +1816,12 @@ void ObjectBase::associate_closure(JSContext* cx, GClosure* closure) {
     g_assert(already_has == m_closures.end() &&
              "This closure was already associated with this object");
     m_closures.push_front(closure);
-    g_closure_add_invalidate_notifier(closure, this,
-                                      &ObjectBase::closure_invalidated_notify);
+    g_closure_add_invalidate_notifier(
+        closure, this, &ObjectInstance::closure_invalidated_notify);
 }
 
-void ObjectBase::closure_invalidated_notify(void* data, GClosure* closure) {
-    auto* priv = static_cast<ObjectBase*>(data);
+void ObjectInstance::closure_invalidated_notify(void* data, GClosure* closure) {
+    auto* priv = static_cast<ObjectInstance*>(data);
     priv->m_closures.remove(closure);
 }
 
@@ -2589,13 +2593,27 @@ bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
         if (!trampoline)
             return false;
 
-        associate_closure(cx, trampoline->js_function);
+        // This is traced, and will be cleared from the list when the closure is
+        // invalidated
+        g_assert(std::find(m_vfuncs.begin(), m_vfuncs.end(),
+                           trampoline->js_function) == m_vfuncs.end() &&
+                 "This vfunc was already associated with this class");
+        m_vfuncs.push_front(trampoline->js_function);
+        g_closure_add_invalidate_notifier(
+            trampoline->js_function, this,
+            &ObjectPrototype::vfunc_invalidated_notify);
+
         *((ffi_closure **)method_ptr) = trampoline->closure;
     }
 
     return true;
 }
 
+void ObjectPrototype::vfunc_invalidated_notify(void* data, GClosure* closure) {
+    auto* priv = static_cast<ObjectPrototype*>(data);
+    priv->m_vfuncs.remove(closure);
+}
+
 bool
 gjs_lookup_object_constructor(JSContext             *context,
                               GType                  gtype,
diff --git a/gi/object.h b/gi/object.h
index 5b8cd258..ea985ca3 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -98,11 +98,6 @@ class ObjectBase
     friend class GIWrapperBase<ObjectBase, ObjectPrototype, ObjectInstance>;
 
  protected:
-    /* a list of all GClosures installed on this object (from
-     * signals, trampolines, explicit GClosures, and vfuncs on prototypes),
-     * used when tracing */
-    std::forward_list<GClosure*> m_closures;
-
     explicit ObjectBase(ObjectPrototype* proto = nullptr)
         : GIWrapperBase(proto) {}
 
@@ -153,23 +148,11 @@ class ObjectBase
                                         no_throw);
     }
 
-    /* Methods to manipulate the list of closures */
-
- protected:
-    void invalidate_all_closures(void);
-
- public:
-    void associate_closure(JSContext* cx, GClosure* closure);
-    static void closure_invalidated_notify(void* data, GClosure* closure);
-
     /* JSClass operations */
 
     static bool add_property(JSContext* cx, JS::HandleObject obj,
                              JS::HandleId id, JS::HandleValue value);
 
- private:
-    void trace_impl(JSTracer* tracer);
-
     /* JS property getters/setters */
 
  public:
@@ -243,6 +226,8 @@ class ObjectPrototype
     PropertyCache m_property_cache;
     FieldCache m_field_cache;
     NegativeLookupCache m_unresolvable_cache;
+    // a list of vfunc GClosures installed on this prototype, used when tracing
+    std::forward_list<GClosure*> m_vfuncs;
 
     ObjectPrototype(GIObjectInfo* info, GType gtype);
     ~ObjectPrototype();
@@ -260,6 +245,8 @@ class ObjectPrototype
 
     GJS_USE
     bool is_vfunc_unchanged(GIVFuncInfo* info);
+    static void vfunc_invalidated_notify(void* data, GClosure* closure);
+
     GJS_JSAPI_RETURN_CONVENTION
     bool lazy_define_gobject_property(JSContext* cx, JS::HandleObject obj,
                                       JS::HandleId id, bool* resolved,
@@ -291,14 +278,12 @@ class ObjectPrototype
                              JS::MutableHandleObject constructor,
                              JS::MutableHandleObject prototype);
 
-    /* These are currently only needed in the GObject base init and finalize
-     * functions, for prototypes, even though m_closures is in ObjectBase. */
-    void ref_closures(void) {
-        for (GClosure* closure : m_closures)
+    void ref_vfuncs(void) {
+        for (GClosure* closure : m_vfuncs)
             g_closure_ref(closure);
     }
-    void unref_closures(void) {
-        for (GClosure* closure : m_closures)
+    void unref_vfuncs(void) {
+        for (GClosure* closure : m_vfuncs)
             g_closure_unref(closure);
     }
 
@@ -330,7 +315,9 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     // GIWrapperInstance::m_ptr may be null in ObjectInstance.
 
     GjsMaybeOwned<JSObject*> m_wrapper;
-
+    // a list of all GClosures installed on this object (from signal connections
+    // and scope-notify callbacks passed to methods), used when tracing
+    std::forward_list<GClosure*> m_closures;
     GjsListLink m_instance_link;
 
     bool m_wrapper_finalized : 1;
@@ -396,6 +383,14 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     GJS_JSAPI_RETURN_CONVENTION
     static JSObject* wrapper_from_gobject(JSContext* cx, GObject* ptr);
 
+    /* Methods to manipulate the list of closures */
+
+ private:
+    static void closure_invalidated_notify(void* data, GClosure* closure);
+
+ public:
+    void associate_closure(JSContext* cx, GClosure* closure);
+
     /* Helper methods */
 
  private:
@@ -453,6 +448,7 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     bool add_property_impl(JSContext* cx, JS::HandleObject obj, JS::HandleId id,
                            JS::HandleValue value);
     void finalize_impl(JSFreeOp* fop, JSObject* obj);
+    void trace_impl(JSTracer* trc);
 
     /* JS property getters/setters */