[gjs/306-too-much-recursion: 2/4] object: Fix signal match mechanism

[Philip Chimento <pchimento src gnome org>]

commit 9ac8e2507aaf2ccc3931cd4c95c6e0cb1ea34f15
Author: Philip Chimento <philip chimento gmail com>
Date:   Mon Mar 16 22:06:04 2020 -0700

    object: Fix signal match mechanism
    
    The previous mechanism, of stuffing the JSFunction pointer into the
    closure data and matching it with G_SIGNAL_MATCH_DATA, did not work in
    some cases. Apparently the garbage collector can move functions around
    even outside of us getting a notification in the trace handler. This
    caused occasional test failures where the wrong number of signals would
    get disconnected or blocked.
    
    Instead, match with G_SIGNAL_MATCH_CLOSURE. But since signal connections
    to the same JS function still have different GClosure structs, we need
    to search the object's connected signals and match any of them that have
    the JS function as their callable object.
    
    In the case of disconnect, we need to store all the candidate GClosures
    in a separate vector and then potentially disconnect them, since
    otherwise they might get removed from the m_closures list while it is
    being iterated.
    
    Keep the old way, with just one call to the GObject API, as fallback in
    case the API user is not matching by function.

 gi/closure.cpp | 12 +-----------
 gi/object.cpp  | 56 +++++++++++++++++++++++++++++++++++++++++---------------
 gi/object.h    |  9 ++++++---
 3 files changed, 48 insertions(+), 29 deletions(-)
---
diff --git a/gi/closure.cpp b/gi/closure.cpp
index 0528062a..f0793c6b 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -281,8 +281,6 @@ gjs_closure_trace(GClosure *closure,
         return;
 
     c->func.trace(tracer, "signal connection");
-    // update the saved address (for comparison only) in case GC moved it
-    closure->data = const_cast<void*>(c->func.debug_addr());
 }
 
 GClosure* gjs_closure_new(JSContext* context, JSFunction* callable,
@@ -290,16 +288,8 @@ GClosure* gjs_closure_new(JSContext* context, JSFunction* callable,
                           bool root_function) {
     Closure *c;
 
-    // We store a bare pointer to the JSFunction in the GClosure's data field
-    // so that g_signal_handlers_block_matched() and friends can work. We are
-    // not supposed to store bare pointers to GC things, but in this particular
-    // case it will work because:
-    //   - we update the data field in gjs_closure_trace() so if the pointer is
-    //     moved or finalized, the data field will still be accurate
-    //   - signal handlers are always in trace mode, so it's not the case that
-    //     JS::PersistentRooted will move the pointer out from under us.
     auto* gc = reinterpret_cast<GjsClosure*>(
-        g_closure_new_simple(sizeof(GjsClosure), callable));
+        g_closure_new_simple(sizeof(GjsClosure), nullptr));
     c = new (&gc->priv) Closure();
 
     /* The saved context is used for lifetime management, so that the closure will
diff --git a/gi/object.cpp b/gi/object.cpp
index 7bc942b9..b5a638b8 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1970,9 +1970,9 @@ ObjectInstance::emit_impl(JSContext          *context,
 
 bool ObjectInstance::signal_match_arguments_from_object(
     JSContext* cx, JS::HandleObject match_obj, GSignalMatchType* mask_out,
-    unsigned* signal_id_out, GQuark* detail_out, void** func_ptr_out) {
-    g_assert(mask_out && signal_id_out && detail_out && func_ptr_out &&
-             "forgot out parameter");
+    unsigned* signal_id_out, GQuark* detail_out,
+    JS::MutableHandleFunction func_out) {
+    g_assert(mask_out && signal_id_out && detail_out && "forgot out parameter");
 
     int mask = 0;
     const GjsAtoms& atoms = GjsContextPrivate::atoms(cx);
@@ -2014,11 +2014,11 @@ bool ObjectInstance::signal_match_arguments_from_object(
     }
 
     bool has_func;
-    void* func_ptr = nullptr;
+    JS::RootedFunction func(cx);
     if (!JS_HasOwnPropertyById(cx, match_obj, atoms.func(), &has_func))
         return false;
     if (has_func) {
-        mask |= G_SIGNAL_MATCH_DATA;
+        mask |= G_SIGNAL_MATCH_CLOSURE;
 
         JS::RootedValue value(cx);
         if (!JS_GetPropertyById(cx, match_obj, atoms.func(), &value))
@@ -2029,7 +2029,7 @@ bool ObjectInstance::signal_match_arguments_from_object(
             return false;
         }
 
-        func_ptr = JS_GetObjectFunction(&value.toObject());
+        func = JS_GetObjectFunction(&value.toObject());
     }
 
     if (!has_id && !has_detail && !has_func) {
@@ -2043,7 +2043,7 @@ bool ObjectInstance::signal_match_arguments_from_object(
     if (has_detail)
         *detail_out = detail;
     if (has_func)
-        *func_ptr_out = func_ptr;
+        func_out.set(func);
     return true;
 }
 
@@ -2070,13 +2070,25 @@ bool ObjectInstance::signal_find_impl(JSContext* cx, const JS::CallArgs& args) {
     GSignalMatchType mask;
     unsigned signal_id;
     GQuark detail;
-    void* func_ptr;
+    JS::RootedFunction func(cx);
     if (!signal_match_arguments_from_object(cx, match, &mask, &signal_id,
-                                            &detail, &func_ptr))
+                                            &detail, &func))
         return false;
 
-    uint64_t handler = g_signal_handler_find(m_ptr, mask, signal_id, detail,
-                                             nullptr, nullptr, func_ptr);
+    uint64_t handler = 0;
+    if (!func) {
+        handler = g_signal_handler_find(m_ptr, mask, signal_id, detail, nullptr,
+                                        nullptr, nullptr);
+    } else {
+        for (GClosure* candidate : m_closures) {
+            if (gjs_closure_get_callable(candidate) == func) {
+                handler = g_signal_handler_find(m_ptr, mask, signal_id, detail,
+                                                candidate, nullptr, nullptr);
+                if (handler != 0)
+                    break;
+            }
+        }
+    }
 
     args.rval().setNumber(static_cast<double>(handler));
     return true;
@@ -2109,13 +2121,27 @@ bool ObjectInstance::signal_find_impl(JSContext* cx, const JS::CallArgs& args) {
         GSignalMatchType mask;                                                \
         unsigned signal_id;                                                   \
         GQuark detail;                                                        \
-        void* func_ptr;                                                       \
+        JS::RootedFunction func(cx);                                          \
         if (!signal_match_arguments_from_object(cx, match, &mask, &signal_id, \
-                                                &detail, &func_ptr)) {        \
+                                                &detail, &func)) {            \
             return false;                                                     \
         }                                                                     \
-        unsigned n_matched = g_signal_handlers_##action##_matched(            \
-            m_ptr, mask, signal_id, detail, nullptr, nullptr, func_ptr);      \
+        unsigned n_matched = 0;                                               \
+        if (!func) {                                                          \
+            n_matched = g_signal_handlers_##action##_matched(                 \
+                m_ptr, mask, signal_id, detail, nullptr, nullptr, nullptr);   \
+        } else {                                                              \
+            std::vector<GClosure*> candidates;                                \
+            for (GClosure* candidate : m_closures) {                          \
+                if (gjs_closure_get_callable(candidate) == func)              \
+                    candidates.push_back(candidate);                          \
+            }                                                                 \
+            for (GClosure* candidate : candidates) {                          \
+                n_matched += g_signal_handlers_##action##_matched(            \
+                    m_ptr, mask, signal_id, detail, candidate, nullptr,       \
+                    nullptr);                                                 \
+            }                                                                 \
+        }                                                                     \
                                                                               \
         args.rval().setNumber(n_matched);                                     \
         return true;                                                          \
diff --git a/gi/object.h b/gi/object.h
index 583ba1d4..1f381b4c 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -405,9 +405,12 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     void ensure_uses_toggle_ref(JSContext* cx);
     GJS_USE bool check_gobject_disposed(const char* for_what) const;
     GJS_JSAPI_RETURN_CONVENTION
-    bool signal_match_arguments_from_object(
-        JSContext* cx, JS::HandleObject props_obj, GSignalMatchType* mask_out,
-        unsigned* signal_id_out, GQuark* detail_out, void** func_ptr_out);
+    bool signal_match_arguments_from_object(JSContext* cx,
+                                            JS::HandleObject props_obj,
+                                            GSignalMatchType* mask_out,
+                                            unsigned* signal_id_out,
+                                            GQuark* detail_out,
+                                            JS::MutableHandleFunction func_out);
 
  public:
     static GObject* copy_ptr(JSContext* G_GNUC_UNUSED, GType G_GNUC_UNUSED,