[gjs] object: Invalidate closures while iterating, avoiding recursion

[Philip Chimento <pchimento src gnome org>]

commit b0e83df3cba495135b9e1ff3b97a1195a201ff2d
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Wed May 12 05:01:17 2021 +0200

    object: Invalidate closures while iterating, avoiding recursion
    
    Clean the closures callbacks while going through it, de-registering the
    related callbacks that may cause searching and removing elements from
    the closures list.
    
    This is not only safer, but also faster.

 gi/object.cpp | 31 +++++++++++++++++++++----------
 gi/object.h   |  1 +
 2 files changed, 22 insertions(+), 10 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 12fe6e8d..03cfd7bc 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1556,21 +1556,26 @@ bool ObjectInstance::ensure_uses_toggle_ref(JSContext* cx) {
     return true;
 }
 
-static void invalidate_closure_list(std::forward_list<GClosure*>* closures) {
+static void invalidate_closure_list(std::forward_list<GClosure*>* closures,
+                                    void* data, GClosureNotify notify_func) {
     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()) {
+    g_assert(notify_func);
+
+    auto before = closures->before_begin();
+    for (auto it = closures->begin(); it != closures->end();) {
         // This will also free the closure data, through the closure
         // invalidation mechanism, but adding a temporary reference to
         // ensure that the closure is still valid when calling invalidation
         // notify callbacks
         GjsAutoGClosure closure(closures->front(), GjsAutoTakeOwnership());
+        it = closures->erase_after(before);
+
+        // Only call the invalidate notifiers that won't touch this vector
+        g_closure_remove_invalidate_notifier(closure, data, notify_func);
         g_closure_invalidate(closure);
-        /* Erase element if not already erased */
-        closures->remove(closure);
     }
+
+    g_assert(closures->empty());
 }
 
 // Note: m_wrapper (the JS object) may already be null when this is called, if
@@ -1598,7 +1603,7 @@ ObjectInstance::disassociate_js_gobject(void)
     }
 
     /* Now release all the resources the current wrapper has */
-    invalidate_closure_list(&m_closures);
+    invalidate_closures();
     release_native_object();
 
     /* Mark that a JS object once existed, but it doesn't any more */
@@ -1764,7 +1769,7 @@ void ObjectInstance::finalize_impl(JSFreeOp* fop, JSObject* obj) {
 ObjectInstance::~ObjectInstance() {
     TRACE(GJS_OBJECT_WRAPPER_FINALIZE(this, m_ptr, ns(), name()));
 
-    invalidate_closure_list(&m_closures);
+    invalidate_closures();
 
     // Do not keep the queue locked here, as we may want to leave the other
     // threads to queue toggle events till we're owning the GObject so that
@@ -1818,7 +1823,7 @@ ObjectInstance::~ObjectInstance() {
 }
 
 ObjectPrototype::~ObjectPrototype() {
-    invalidate_closure_list(&m_vfuncs);
+    invalidate_closure_list(&m_vfuncs, this, &vfunc_invalidated_notify);
 
     g_type_class_unref(g_type_class_peek(m_gtype));
 
@@ -1963,10 +1968,15 @@ bool ObjectInstance::associate_closure(JSContext* cx, GClosure* closure) {
 }
 
 void ObjectInstance::closure_invalidated_notify(void* data, GClosure* closure) {
+    // This callback should *only* touch m_closures
     auto* priv = static_cast<ObjectInstance*>(data);
     priv->m_closures.remove(closure);
 }
 
+void ObjectInstance::invalidate_closures() {
+    invalidate_closure_list(&m_closures, this, &closure_invalidated_notify);
+}
+
 bool ObjectBase::connect(JSContext* cx, unsigned argc, JS::Value* vp) {
     GJS_CHECK_WRAPPER_PRIV(cx, argc, vp, args, obj, ObjectBase, priv);
     if (!priv->check_is_instance(cx, "connect to signals"))
@@ -2799,6 +2809,7 @@ bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
 }
 
 void ObjectPrototype::vfunc_invalidated_notify(void* data, GClosure* closure) {
+    // This callback should *only* touch m_vfuncs
     auto* priv = static_cast<ObjectPrototype*>(data);
     priv->m_vfuncs.remove(closure);
 }
diff --git a/gi/object.h b/gi/object.h
index 1d5403bd..6003bd5e 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -369,6 +369,7 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     /* Methods to manipulate the list of closures */
 
  private:
+    void invalidate_closures();
     static void closure_invalidated_notify(void* data, GClosure* closure);
 
  public: