closure: Store JSFunction* pointer instead of JSObject*

In practice not much difference, but this should give a small extra
safeguard against accidentally using a non-function object as the
callable of a signal connection.

9 files changed, 91 insertions, 107 deletions

diff --git a/gi/arg.cpp b/gi/arg.cpp
index 911730bc..a4e96e94 100644
--- a/gi/arg.cpp
+++ b/gi/arg.cpp
@@ -1776,11 +1776,11 @@ gjs_value_to_g_argument(JSContext      *context,
                         }
                     } else if (g_type_is_a(gtype, G_TYPE_BOXED)) {
                         if (g_type_is_a(gtype, G_TYPE_CLOSURE)) {
-                            arg->v_pointer = gjs_closure_new_marshaled(context,
-                                                                       &value.toObject(),
-                                                                       "boxed");
-                            g_closure_ref((GClosure *) arg->v_pointer);
-                            g_closure_sink((GClosure *) arg->v_pointer);
+                            GClosure* closure = gjs_closure_new_marshaled(
+                                context, JS_GetObjectFunction(obj), "boxed");
+                            g_closure_ref(closure);
+                            g_closure_sink(closure);
+                            arg->v_pointer = closure;
                         } else {
                             /* Should have been caught above as STRUCT/BOXED/UNION */
                             gjs_throw(context,
diff --git a/gi/closure.cpp b/gi/closure.cpp
index e5c38413..5bf082d3 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -34,7 +34,7 @@
 
 struct Closure {
     JSContext *context;
-    GjsMaybeOwned<JSObject *> obj;
+    GjsMaybeOwned<JSFunction*> func;
 };
 
 struct GjsClosure {
@@ -86,10 +86,10 @@ invalidate_js_pointers(GjsClosure *gc)
 
     c = &gc->priv;
 
-    if (c->obj == nullptr)
+    if (!c->func)
         return;
 
-    c->obj.reset();
+    c->func.reset();
     c->context = NULL;
 
     /* Notify any closure reference holders they
@@ -98,19 +98,17 @@ invalidate_js_pointers(GjsClosure *gc)
     g_closure_invalidate(&gc->base);
 }
 
-static void
-global_context_finalized(JS::HandleObject obj,
-                         void            *data)
-{
+static void global_context_finalized(JS::HandleFunction func, void* data) {
     GjsClosure *gc = (GjsClosure*) data;
     Closure *c = &gc->priv;
 
-    gjs_debug_closure("Context global object destroy notifier on closure %p "
-                      "which calls object %p",
-                      c, c->obj.get());
+    gjs_debug_closure(
+        "Context global object destroy notifier on closure %p which calls "
+        "object %p",
+        c, c->func.get());
 
-    if (c->obj) {
-        g_assert(c->obj == obj.get());
+    if (c->func) {
+        g_assert(c->func == func.get());
 
         invalidate_js_pointers(gc);
     }
@@ -136,10 +134,10 @@ closure_invalidated(gpointer data,
     c = &((GjsClosure*) closure)->priv;
 
     GJS_DEC_COUNTER(closure);
-    gjs_debug_closure("Invalidating closure %p which calls object %p",
-                      closure, c->obj.get());
+    gjs_debug_closure("Invalidating closure %p which calls function %p",
+                      closure, c->func.get());
 
-    if (c->obj == nullptr) {
+    if (!c->func) {
         gjs_debug_closure("   (closure %p already dead, nothing to do)",
                           closure);
         return;
@@ -156,7 +154,7 @@ closure_invalidated(gpointer data,
                       "removing our destroy notifier on global object)",
                       closure);
 
-    c->obj.reset();
+    c->func.reset();
     c->context = nullptr;
 }
 
@@ -166,11 +164,11 @@ closure_set_invalid(gpointer  data,
 {
     Closure *self = &((GjsClosure*) closure)->priv;
 
-    gjs_debug_closure("Invalidating signal closure %p which calls object %p",
-                      closure, self->obj.get());
+    gjs_debug_closure("Invalidating signal closure %p which calls function %p",
+                      closure, self->func.get());
 
-    self->obj.prevent_collection();
-    self->obj.reset();
+    self->func.prevent_collection();
+    self->func.reset();
     self->context = nullptr;
 
     GJS_DEC_COUNTER(closure);
@@ -197,7 +195,7 @@ gjs_closure_invoke(GClosure                   *closure,
 
     c = &((GjsClosure*) closure)->priv;
 
-    if (c->obj == nullptr) {
+    if (!c->func) {
         /* We were destroyed; become a no-op */
         c->context = NULL;
         return false;
@@ -205,7 +203,7 @@ gjs_closure_invoke(GClosure                   *closure,
 
     context = c->context;
     JSAutoRequest ar(context);
-    JSAutoCompartment ac(context, c->obj);
+    JSAutoCompartment ac(context, JS_GetFunctionObject(c->func));
 
     if (JS_IsExceptionPending(context)) {
         gjs_debug_closure("Exception was pending before invoking callback??? "
@@ -213,12 +211,13 @@ gjs_closure_invoke(GClosure                   *closure,
         gjs_log_exception(context);
     }
 
-    JS::RootedValue v_closure(context, JS::ObjectValue(*c->obj));
-    if (!gjs_call_function_value(context, this_obj, v_closure, args, retval)) {
+    JS::RootedFunction func(context, c->func);
+    if (!JS::Call(context, this_obj, func, args, retval)) {
         /* Exception thrown... */
-        gjs_debug_closure("Closure invocation failed (exception should "
-                          "have been thrown) closure %p callable %p",
-                          closure, c->obj.get());
+        gjs_debug_closure(
+            "Closure invocation failed (exception should have been thrown) "
+            "closure %p function %p",
+            closure, c->func.get());
         /* If an exception has been thrown, log it, unless the caller
          * explicitly wants to handle it manually (for example to turn it
          * into a GError), in which case it replaces the return value
@@ -241,7 +240,7 @@ gjs_closure_invoke(GClosure                   *closure,
                           " - closure %p", closure);
     }
 
-    JS_MaybeGC(context);
+    gjs_schedule_gc_if_needed(context);
     return true;
 }
 
@@ -265,14 +264,12 @@ gjs_closure_get_context(GClosure *closure)
     return c->context;
 }
 
-JSObject*
-gjs_closure_get_callable(GClosure *closure)
-{
+JSFunction* gjs_closure_get_callable(GClosure* closure) {
     Closure *c;
 
     c = &((GjsClosure*) closure)->priv;
 
-    return c->obj;
+    return c->func;
 }
 
 void
@@ -283,18 +280,14 @@ gjs_closure_trace(GClosure *closure,
 
     c = &((GjsClosure*) closure)->priv;
 
-    if (c->obj == nullptr)
+    if (!c->func)
         return;
 
-    c->obj.trace(tracer, "signal connection");
+    c->func.trace(tracer, "signal connection");
 }
 
-GClosure*
-gjs_closure_new(JSContext  *context,
-                JSObject   *callable,
-                const char *description,
-                bool        root_function)
-{
+GClosure* gjs_closure_new(JSContext* context, JSFunction* callable,
+                          const char* description, bool root_function) {
     GjsClosure *gc;
     Closure *c;
 
@@ -313,11 +306,11 @@ gjs_closure_new(JSContext  *context,
 
     if (root_function) {
         /* Fully manage closure lifetime if so asked */
-        c->obj.root(context, callable, global_context_finalized, gc);
+        c->func.root(context, callable, global_context_finalized, gc);
 
         g_closure_add_invalidate_notifier(&gc->base, NULL, closure_invalidated);
     } else {
-        c->obj = callable;
+        c->func = callable;
         /* Only mark the closure as invalid if memory is managed
            outside (i.e. by object.c for signals) */
         g_closure_add_invalidate_notifier(&gc->base, NULL, closure_set_invalid);
@@ -325,8 +318,8 @@ gjs_closure_new(JSContext  *context,
 
     g_closure_add_finalize_notifier(&gc->base, NULL, closure_finalize);
 
-    gjs_debug_closure("Create closure %p which calls object %p '%s'",
-                      gc, c->obj.get(), description);
+    gjs_debug_closure("Create closure %p which calls function %p '%s'", gc,
+                      c->func.get(), description);
 
     JS_EndRequest(context);
 
diff --git a/gi/closure.h b/gi/closure.h
index 40f5b0d8..33cc5341 100644
--- a/gi/closure.h
+++ b/gi/closure.h
@@ -33,10 +33,8 @@
 G_BEGIN_DECLS
 
 GJS_USE
-GClosure*  gjs_closure_new           (JSContext    *context,
-                                      JSObject     *callable,
-                                      const char   *description,
-                                      bool          root_function);
+GClosure* gjs_closure_new(JSContext* cx, JSFunction* callable,
+                          const char* description, bool root_function);
 
 GJS_USE
 bool gjs_closure_invoke(GClosure                   *closure,
@@ -50,7 +48,7 @@ JSContext* gjs_closure_get_context   (GClosure     *closure);
 GJS_USE
 bool       gjs_closure_is_valid      (GClosure     *closure);
 GJS_USE
-JSObject*  gjs_closure_get_callable  (GClosure     *closure);
+JSFunction* gjs_closure_get_callable(GClosure* closure);
 
 void       gjs_closure_trace         (GClosure     *closure,
                                       JSTracer     *tracer);
diff --git a/gi/function.cpp b/gi/function.cpp
index 2673eaf5..41586f14 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -226,8 +226,8 @@ gjs_callback_closure(ffi_cif *cif,
     }
 
     JS_BeginRequest(context);
-    JSAutoCompartment ac(context,
-                         gjs_closure_get_callable(trampoline->js_function));
+    JSAutoCompartment ac(context, JS_GetFunctionObject(gjs_closure_get_callable(
+                                      trampoline->js_function)));
 
     bool can_throw_gerror = g_callable_info_can_throw_gerror(trampoline->info);
     n_args = g_callable_info_get_n_args(trampoline->info);
@@ -490,19 +490,14 @@ gjs_destroy_notify_callback(gpointer data)
     gjs_callback_trampoline_unref(trampoline);
 }
 
-GjsCallbackTrampoline*
-gjs_callback_trampoline_new(JSContext       *context,
-                            JS::HandleValue  function,
-                            GICallableInfo  *callable_info,
-                            GIScopeType      scope,
-                            JS::HandleObject scope_object,
-                            bool             is_vfunc)
-{
+GjsCallbackTrampoline* gjs_callback_trampoline_new(
+    JSContext* context, JS::HandleFunction function,
+    GICallableInfo* callable_info, GIScopeType scope,
+    JS::HandleObject scope_object, bool is_vfunc) {
     GjsCallbackTrampoline *trampoline;
     int n_args, i;
 
-    g_assert(function.isObject());
-    g_assert(JS_TypeOfValue(context, function) == JSTYPE_FUNCTION);
+    g_assert(function);
 
     trampoline = g_slice_new(GjsCallbackTrampoline);
     new (trampoline) GjsCallbackTrampoline();
@@ -516,9 +511,8 @@ gjs_callback_trampoline_new(JSContext       *context,
      *   (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.toObject(),
-                                              g_base_info_get_name(callable_info),
-                                              should_root);
+    trampoline->js_function = gjs_closure_new(
+        context, function, g_base_info_get_name(callable_info), should_root);
     if (!should_root && scope_object)
         gjs_object_associate_closure(context, scope_object,
                                      trampoline->js_function);
@@ -959,13 +953,12 @@ gjs_invoke_c_function(JSContext                             *context,
                         break;
                     }
 
+                    JS::RootedFunction func(
+                        context, JS_GetObjectFunction(&current_arg.toObject()));
                     callable_info = (GICallableInfo*) g_type_info_get_interface(&ainfo);
-                    trampoline = gjs_callback_trampoline_new(context,
-                                                             current_arg,
-                                                             callable_info,
-                                                             scope,
-                                                             is_object_method ? obj : nullptr,
-                                                             false);
+                    trampoline = gjs_callback_trampoline_new(
+                        context, func, callable_info, scope,
+                        is_object_method ? obj : nullptr, false);
                     closure = trampoline->closure;
                     g_base_info_unref(callable_info);
                 }
diff --git a/gi/function.h b/gi/function.h
index 30f473c7..42861d39 100644
--- a/gi/function.h
+++ b/gi/function.h
@@ -56,12 +56,9 @@ struct GjsCallbackTrampoline {
 };
 
 GJS_JSAPI_RETURN_CONVENTION
-GjsCallbackTrampoline* gjs_callback_trampoline_new(JSContext       *context,
-                                                   JS::HandleValue  function,
-                                                   GICallableInfo  *callable_info,
-                                                   GIScopeType      scope,
-                                                   JS::HandleObject scope_object,
-                                                   bool             is_vfunc);
+GjsCallbackTrampoline* gjs_callback_trampoline_new(
+    JSContext* cx, JS::HandleFunction function, GICallableInfo* callable_info,
+    GIScopeType scope, JS::HandleObject 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 0edb003e..ee31e1b6 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1917,7 +1917,8 @@ ObjectInstance::connect_impl(JSContext          *context,
         return false;
     }
 
-    closure = gjs_closure_new_for_signal(context, callback, "signal callback", signal_id);
+    closure = gjs_closure_new_for_signal(
+        context, JS_GetObjectFunction(callback), "signal callback", signal_id);
     if (closure == NULL)
         return false;
     associate_closure(context, closure);
@@ -2533,9 +2534,13 @@ bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
         offset = g_field_info_get_offset(field_info);
         method_ptr = G_STRUCT_MEMBER_P(implementor_vtable, offset);
 
-        JS::RootedValue v_function(cx, JS::ObjectValue(*function));
+        if (!JS_ObjectIsFunction(cx, function)) {
+            gjs_throw(cx, "Tried to deal with a vfunc that wasn't a function");
+            return false;
+        }
+        JS::RootedFunction func(cx, JS_GetObjectFunction(function));
         trampoline = gjs_callback_trampoline_new(
-            cx, v_function, vfunc, GI_SCOPE_TYPE_NOTIFIED, prototype, true);
+            cx, func, vfunc, GI_SCOPE_TYPE_NOTIFIED, prototype, true);
 
         *((ffi_closure **)method_ptr) = trampoline->closure;
     }
diff --git a/gi/value.cpp b/gi/value.cpp
index fabecca2..1e51fd96 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -124,7 +124,6 @@ closure_marshal(GClosure        *closure,
                 gpointer         marshal_data)
 {
     JSContext *context;
-    JSObject *obj;
     unsigned i;
     GSignalQuery signal_query = { 0, };
     GISignalInfo *signal_info;
@@ -163,9 +162,9 @@ closure_marshal(GClosure        *closure,
         return;
     }
 
-    obj = gjs_closure_get_callable(closure);
+    JSFunction* func = gjs_closure_get_callable(closure);
     JSAutoRequest ar(context);
-    JSAutoCompartment ac(context, obj);
+    JSAutoCompartment ac(context, JS_GetFunctionObject(func));
 
     if (marshal_data) {
         /* we are used for a signal handler */
@@ -292,12 +291,8 @@ closure_marshal(GClosure        *closure,
     }
 }
 
-GClosure*
-gjs_closure_new_for_signal(JSContext  *context,
-                           JSObject   *callable,
-                           const char *description,
-                           guint       signal_id)
-{
+GClosure* gjs_closure_new_for_signal(JSContext* context, JSFunction* callable,
+                                     const char* description, guint signal_id) {
     GClosure *closure;
 
     closure = gjs_closure_new(context, callable, description, false);
@@ -307,11 +302,8 @@ gjs_closure_new_for_signal(JSContext  *context,
     return closure;
 }
 
-GClosure*
-gjs_closure_new_marshaled (JSContext    *context,
-                           JSObject     *callable,
-                           const char   *description)
-{
+GClosure* gjs_closure_new_marshaled(JSContext* context, JSFunction* callable,
+                                    const char* description) {
     GClosure *closure;
 
     closure = gjs_closure_new(context, callable, description, true);
diff --git a/gi/value.h b/gi/value.h
index af61b8d8..ab9f8194 100644
--- a/gi/value.h
+++ b/gi/value.h
@@ -47,14 +47,12 @@ bool gjs_value_from_g_value(JSContext             *context,
                             const GValue          *gvalue);
 
 GJS_USE
-GClosure*  gjs_closure_new_marshaled    (JSContext    *context,
-                                         JSObject     *callable,
-                                         const char   *description);
+GClosure* gjs_closure_new_marshaled(JSContext* cx, JSFunction* callable,
+                                    const char* description);
 GJS_USE
-GClosure*  gjs_closure_new_for_signal   (JSContext    *context,
-                                         JSObject     *callable,
-                                         const char   *description,
-                                         guint         signal_id);
+GClosure* gjs_closure_new_for_signal(JSContext* cx, JSFunction* callable,
+                                     const char* description,
+                                     unsigned signal_id);
 
 G_END_DECLS
 
diff --git a/gjs/jsapi-util-root.h b/gjs/jsapi-util-root.h
index 299562bd..c81c4e55 100644
--- a/gjs/jsapi-util-root.h
+++ b/gjs/jsapi-util-root.h
@@ -87,8 +87,16 @@ struct GjsHeapOperation<JSObject *> {
     }
 };
 
-template<>
-struct GjsHeapOperation<JS::Value> {};
+template <>
+struct GjsHeapOperation<JSFunction*> {
+    static void expose_to_js(const JS::Heap<JSFunction*>& thing) {
+        JSFunction* func = thing.unbarrieredGet();
+        if (!func || !js::gc::detail::GetGCThingZone(uintptr_t(func)))
+            return;
+        if (!JS::CurrentThreadIsHeapCollecting())
+            js::gc::ExposeGCThingToActiveJS(JS::GCCellPtr(func));
+    }
+};
 
 /* GjsMaybeOwned is intended only for use in heap allocation. Do not allocate it
  * on the stack, and do not allocate any instances of structures that have it as