arg-cache: Make it clearer how we handle the trampoline references

In arg-cache we add a new reference to the trampoline that is going to
be removed on DestroyNotify callback or(?) when the async calls are
cleared. However this is quite unclear as it picks multiple types of
function scopes.
So put all together adding a ref and using a lambda function to unref,
so that we have a more readable and understandable code.

This fixes also a potential issue when a function is both async and has
a callback destroy, as in such case we'd just add one reference, while
we'd remove two.

2 files changed, 16 insertions, 14 deletions

diff --git a/gi/arg-cache.cpp b/gi/arg-cache.cpp
index f06e905c..ac356abb 100644
--- a/gi/arg-cache.cpp
+++ b/gi/arg-cache.cpp
@@ -67,15 +67,6 @@ static const char* expected_type_names[] = {"object", "function", "string"};
 static_assert(G_N_ELEMENTS(expected_type_names) == ExpectedType::LAST,
               "Names must match the values in ExpectedType");
 
-// The global entry point for any invocations of GDestroyNotify; look up the
-// callback through the user_data and then free it.
-static void gjs_destroy_notify_callback(void* data) {
-    auto* trampoline = static_cast<GjsCallbackTrampoline*>(data);
-
-    g_assert(trampoline);
-    gjs_callback_trampoline_unref(trampoline);
-}
-
 // A helper function to retrieve array lengths from a GIArgument (letting the
 // compiler generate good instructions in case of big endian machines)
 [[nodiscard]] static size_t gjs_g_argument_get_array_length(GITypeTag tag,
@@ -332,17 +323,26 @@ static bool gjs_marshal_callback_in(JSContext* cx, GjsArgumentCache* self,
 
     if (self->has_callback_destroy()) {
         uint8_t destroy_pos = self->contents.callback.destroy_pos;
-        gjs_arg_set(&state->in_cvalues[destroy_pos],
-                    trampoline ? gjs_destroy_notify_callback : nullptr);
+        GDestroyNotify destroy_notify = nullptr;
+        if (trampoline) {
+            /* Adding another reference and a DestroyNotify that unsets it */
+            gjs_callback_trampoline_ref(trampoline);
+            destroy_notify = [](void* data) {
+                g_assert(data);
+                gjs_callback_trampoline_unref(
+                    static_cast<GjsCallbackTrampoline*>(data));
+            };
+        }
+        gjs_arg_set(&state->in_cvalues[destroy_pos], destroy_notify);
     }
     if (self->has_callback_closure()) {
         uint8_t closure_pos = self->contents.callback.closure_pos;
         gjs_arg_set(&state->in_cvalues[closure_pos], trampoline);
     }
 
-    if (trampoline && self->contents.callback.scope != GI_SCOPE_TYPE_CALL) {
-        // Add an extra reference that will be cleared when collecting async
-        // calls, or when GDestroyNotify is called
+    if (trampoline && self->contents.callback.scope == GI_SCOPE_TYPE_ASYNC) {
+        // Add an extra reference that will be cleared when garbage collecting
+        // async calls
         gjs_callback_trampoline_ref(trampoline);
     }
     gjs_arg_set(arg, closure);
diff --git a/gi/function.cpp b/gi/function.cpp
index 1fcbb723..ad2f0af7 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -523,6 +523,8 @@ out:
     }
 
     if (trampoline->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()
         completed_trampolines = g_slist_prepend(completed_trampolines, trampoline);
     }