[gjs/217-separate-closures-and-vfuncs: 1/2] function: Associate closure with object after creating trampoline

[Philip Chimento <pchimento src gnome org>]

commit d9d58c8947f79aa7fcd6eba16bdad9be89b3a6b8
Author: Philip Chimento <philip chimento gmail com>
Date:   Wed Mar 4 23:23:41 2020 -0800

    function: Associate closure with object after creating trampoline
    
    This makes the vfunc code slightly less complicated, and is needed for
    a refactor in a subsequent commit.
    
    See: #217

 gi/function.cpp | 50 ++++++++++++++++++++++++++++----------------------
 gi/function.h   |  2 +-
 gi/object.cpp   |  8 ++++----
 gi/object.h     |  3 +--
 4 files changed, 34 insertions(+), 29 deletions(-)
---
diff --git a/gi/function.cpp b/gi/function.cpp
index f857426c..02735cfa 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -514,8 +514,8 @@ gjs_destroy_notify_callback(gpointer data)
 
 GjsCallbackTrampoline* gjs_callback_trampoline_new(
     JSContext* context, JS::HandleFunction function,
-    GICallableInfo* callable_info, GIScopeType scope,
-    JS::HandleObject scope_object, bool is_vfunc) {
+    GICallableInfo* callable_info, GIScopeType scope, bool has_scope_object,
+    bool is_vfunc) {
     GjsCallbackTrampoline *trampoline;
     int n_args, i;
 
@@ -527,24 +527,6 @@ GjsCallbackTrampoline* gjs_callback_trampoline_new(
     trampoline->info = callable_info;
     g_base_info_ref((GIBaseInfo*)trampoline->info);
 
-    /* The rule is:
-     * - async and call callbacks are rooted
-     * - callbacks in GObjects methods are traced from the object
-     *   (and same for vfuncs, which are associated with a GObject prototype)
-     */
-    bool should_root = scope != GI_SCOPE_TYPE_NOTIFIED || !scope_object;
-    trampoline->js_function = gjs_closure_new(
-        context, function, g_base_info_get_name(callable_info), should_root);
-    if (!should_root && scope_object) {
-        ObjectBase* scope_priv = ObjectBase::for_js(context, scope_object);
-        if (!scope_priv) {
-            gjs_throw(context, "Signal connected to wrong type of object");
-            return nullptr;
-        }
-
-        scope_priv->associate_closure(context, trampoline->js_function);
-    }
-
     /* Analyze param types and directions, similarly to init_cached_function_data */
     n_args = g_callable_info_get_n_args(trampoline->info);
     trampoline->param_types = g_new0(GjsParamType, n_args);
@@ -612,6 +594,14 @@ GjsCallbackTrampoline* gjs_callback_trampoline_new(
     trampoline->closure = g_callable_info_prepare_closure(callable_info, &trampoline->cif,
                                                           gjs_callback_closure, trampoline);
 
+    // The rule is:
+    // - notify callbacks in GObject methods are traced from the scope object
+    // - async and call callbacks, and other notify callbacks, are rooted
+    // - vfuncs are traced from the GObject prototype
+    bool should_root = scope != GI_SCOPE_TYPE_NOTIFIED || !has_scope_object;
+    trampoline->js_function = gjs_closure_new(
+        context, function, g_base_info_get_name(callable_info), should_root);
+
     trampoline->scope = scope;
     trampoline->is_vfunc = is_vfunc;
 
@@ -974,9 +964,25 @@ static bool gjs_invoke_c_function(JSContext* context, Function* function,
                         context, JS_GetObjectFunction(&current_arg.toObject()));
                     callable_info = (GICallableInfo*) g_type_info_get_interface(&ainfo);
                     trampoline = gjs_callback_trampoline_new(
-                        context, func, callable_info, scope,
-                        is_object_method ? JS::HandleObject(obj) : nullptr,
+                        context, func, callable_info, scope, is_object_method,
                         false);
+                    if (!trampoline) {
+                        failed = true;
+                        break;
+                    }
+                    if (scope == GI_SCOPE_TYPE_NOTIFIED && is_object_method) {
+                        ObjectBase* priv = ObjectBase::for_js(context, obj);
+                        if (!priv) {
+                            gjs_throw(
+                                context,
+                                "Signal connected to wrong type of object");
+                            failed = true;
+                            break;
+                        }
+
+                        priv->associate_closure(context,
+                                                trampoline->js_function);
+                    }
                     closure = trampoline->closure;
                     g_base_info_unref(callable_info);
                 }
diff --git a/gi/function.h b/gi/function.h
index 3d00a992..8b4b1c95 100644
--- a/gi/function.h
+++ b/gi/function.h
@@ -62,7 +62,7 @@ struct GjsCallbackTrampoline {
 GJS_JSAPI_RETURN_CONVENTION
 GjsCallbackTrampoline* gjs_callback_trampoline_new(
     JSContext* cx, JS::HandleFunction function, GICallableInfo* callable_info,
-    GIScopeType scope, JS::HandleObject scope_object, bool is_vfunc);
+    GIScopeType scope, bool has_scope_object, bool is_vfunc);
 
 void gjs_callback_trampoline_unref(GjsCallbackTrampoline *trampoline);
 void gjs_callback_trampoline_ref(GjsCallbackTrampoline *trampoline);
diff --git a/gi/object.cpp b/gi/object.cpp
index 6c861846..d92441ba 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -2507,12 +2507,11 @@ bool ObjectBase::hook_up_vfunc(JSContext* cx, unsigned argc, JS::Value* vp) {
     GJS_GET_WRAPPER_PRIV(cx, argc, vp, args, prototype, ObjectBase, priv);
     /* Normally we wouldn't assert is_prototype(), but this method can only be
      * called internally so it's OK to crash if done wrongly */
-    return priv->to_prototype()->hook_up_vfunc_impl(cx, args, prototype);
+    return priv->to_prototype()->hook_up_vfunc_impl(cx, args);
 }
 
 bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
-                                         const JS::CallArgs& args,
-                                         JS::HandleObject prototype) {
+                                         const JS::CallArgs& args) {
     JS::UniqueChars name;
     JS::RootedObject function(cx);
     if (!gjs_parse_call_args(cx, "hook_up_vfunc", args, "so",
@@ -2586,10 +2585,11 @@ bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
         }
         JS::RootedFunction func(cx, JS_GetObjectFunction(function));
         trampoline = gjs_callback_trampoline_new(
-            cx, func, vfunc, GI_SCOPE_TYPE_NOTIFIED, prototype, true);
+            cx, func, vfunc, GI_SCOPE_TYPE_NOTIFIED, true, true);
         if (!trampoline)
             return false;
 
+        associate_closure(cx, trampoline->js_function);
         *((ffi_closure **)method_ptr) = trampoline->closure;
     }
 
diff --git a/gi/object.h b/gi/object.h
index e48c1bea..5b8cd258 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -317,8 +317,7 @@ class ObjectPrototype
     /* JS methods */
  public:
     GJS_JSAPI_RETURN_CONVENTION
-    bool hook_up_vfunc_impl(JSContext* cx, const JS::CallArgs& args,
-                            JS::HandleObject prototype);
+    bool hook_up_vfunc_impl(JSContext* cx, const JS::CallArgs& args);
 };
 
 class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,