From 167c5e9cf716f61571642348c4f09d2b907e0abe Mon Sep 17 00:00:00 2001 From: Philip Chimento Date: Sun, 6 Nov 2022 20:58:42 -0800 Subject: Revert "closure: Store JSFunction* pointer instead of JSObject*" This is basically a revert of commit 7140cac8, although so much has changed in the meantime that none of the revert applied cleanly, so it's more of an "un-rewrite". This fixes a regression that has been present for almost exactly 4 years. Storing a JSFunction* pointer in closures did not take into account the fact that not all callable JSObjects have an associated JSFunction. They could be exotic objects with a [[Call]] method. In particular, introspected functions are this, so this prevented g-i function objects from being used as signal handlers. (Less importantly, the same probably applied to callable Proxy objects.) Also adds a regression test so that this doesn't happen again. Closes: #518 --- gi/arg-cache.cpp | 11 +++++------ gi/arg.cpp | 7 +++---- gi/closure.cpp | 44 ++++++++++++++++++++++---------------------- gi/closure.h | 18 +++++++++--------- gi/function.cpp | 38 +++++++++++++++++++++++--------------- gi/function.h | 7 +++---- gi/object.cpp | 42 ++++++++++++++++++++---------------------- gi/object.h | 10 ++++------ gi/value.cpp | 13 ++++++++----- 9 files changed, 97 insertions(+), 93 deletions(-) (limited to 'gi') diff --git a/gi/arg-cache.cpp b/gi/arg-cache.cpp index ce814d6b..290577fd 100644 --- a/gi/arg-cache.cpp +++ b/gi/arg-cache.cpp @@ -27,7 +27,6 @@ #include #include #include // for InformalValueTypeName, JS_TypeOfValue -#include // for JS_GetObjectFunction #include // for JSTYPE_FUNCTION #include "gi/arg-cache.h" @@ -844,10 +843,10 @@ bool CallbackIn::in(JSContext* cx, GjsFunctionCallState* state, GIArgument* arg, return false; } - JS::RootedFunction func(cx, JS_GetObjectFunction(&value.toObject())); + JS::RootedObject callable(cx, &value.toObject()); bool is_object_method = !!state->instance_object; - trampoline = GjsCallbackTrampoline::create(cx, func, m_info, m_scope, - is_object_method, false); + trampoline = GjsCallbackTrampoline::create( + cx, callable, m_info, m_scope, is_object_method, false); if (!trampoline) return false; if (m_scope == GI_SCOPE_TYPE_NOTIFIED && is_object_method) { @@ -1179,8 +1178,8 @@ bool GClosureInTransferNone::in(JSContext* cx, GjsFunctionCallState* state, return report_typeof_mismatch(cx, m_arg_name, value, ExpectedType::FUNCTION); - JS::RootedFunction func(cx, JS_GetObjectFunction(&value.toObject())); - GClosure* closure = Gjs::Closure::create_marshaled(cx, func, "boxed"); + JS::RootedObject callable(cx, &value.toObject()); + GClosure* closure = Gjs::Closure::create_marshaled(cx, callable, "boxed"); gjs_arg_set(arg, closure); g_closure_ref(closure); g_closure_sink(closure); diff --git a/gi/arg.cpp b/gi/arg.cpp index d70476c1..56a46093 100644 --- a/gi/arg.cpp +++ b/gi/arg.cpp @@ -29,8 +29,7 @@ #include #include #include -#include // for InformalValueTypeName, IdVector -#include // for JS_GetObjectFunction +#include // for InformalValueTypeName, IdVector #include "gi/arg-inl.h" #include "gi/arg-types-inl.h" @@ -1337,8 +1336,8 @@ static bool value_to_interface_gi_argument( } else if (g_type_is_a(gtype, G_TYPE_BOXED)) { if (g_type_is_a(gtype, G_TYPE_CLOSURE)) { - GClosure* closure = Gjs::Closure::create_marshaled( - cx, JS_GetObjectFunction(obj), "boxed"); + GClosure* closure = + Gjs::Closure::create_marshaled(cx, obj, "boxed"); // GI doesn't know about floating GClosure references. We // guess that if this is a return value going from JS::Value // to GArgument, it's intended to be passed to a C API that diff --git a/gi/closure.cpp b/gi/closure.cpp index 4d85a60a..2d0c1c8c 100644 --- a/gi/closure.cpp +++ b/gi/closure.cpp @@ -12,8 +12,8 @@ #include #include #include +#include #include -#include // for JS_GetFunctionObject #include "gi/closure.h" #include "gjs/context-private.h" @@ -24,7 +24,7 @@ namespace Gjs { -Closure::Closure(JSContext* cx, JSFunction* callable, bool root, +Closure::Closure(JSContext* cx, JSObject* callable, bool root, const char* description GJS_USED_VERBOSE_GCLOSURE) : m_cx(cx) { GJS_INC_COUNTER(closure); @@ -34,7 +34,7 @@ Closure::Closure(JSContext* cx, JSFunction* callable, bool root, // Fully manage closure lifetime if so asked auto* gjs = GjsContextPrivate::from_cx(cx); g_assert(cx == gjs->context()); - m_func.root(cx, callable); + m_callable.root(cx, callable); gjs->register_notifier(global_context_notifier_cb, this); closure_notify = [](void*, GClosure* closure) { static_cast(closure)->closure_invalidated(); @@ -42,7 +42,7 @@ Closure::Closure(JSContext* cx, JSFunction* callable, bool root, } else { // Only mark the closure as invalid if memory is managed // outside (i.e. by object.c for signals) - m_func = callable; + m_callable = callable; closure_notify = [](void*, GClosure* closure) { static_cast(closure)->closure_set_invalid(); }; @@ -50,8 +50,8 @@ Closure::Closure(JSContext* cx, JSFunction* callable, bool root, g_closure_add_invalidate_notifier(this, nullptr, closure_notify); - gjs_debug_closure("Create closure %p which calls function %p '%s'", this, - m_func.debug_addr(), description); + gjs_debug_closure("Create closure %p which calls callable %p '%s'", this, + m_callable.debug_addr(), description); } /* @@ -92,7 +92,7 @@ void Closure::unset_context() { if (!m_cx) return; - if (m_func && m_func.rooted()) { + if (m_callable && m_callable.rooted()) { auto* gjs = GjsContextPrivate::from_cx(m_cx); gjs->unregister_notifier(global_context_notifier_cb, this); } @@ -103,10 +103,10 @@ void Closure::unset_context() { void Closure::global_context_finalized() { gjs_debug_closure( "Context global object destroy notifier on closure %p which calls " - "object %p", - this, m_func.debug_addr()); + "callable %p", + this, m_callable.debug_addr()); - if (m_func) { + if (m_callable) { // Manually unset the context as we don't need to unregister the // notifier here, or we'd end up touching a vector we're iterating m_cx = nullptr; @@ -130,10 +130,10 @@ void Closure::global_context_finalized() { */ void Closure::closure_invalidated() { GJS_DEC_COUNTER(closure); - gjs_debug_closure("Invalidating closure %p which calls function %p", this, - m_func.debug_addr()); + gjs_debug_closure("Invalidating closure %p which calls callable %p", this, + m_callable.debug_addr()); - if (!m_func) { + if (!m_callable) { gjs_debug_closure(" (closure %p already dead, nothing to do)", this); return; } @@ -154,10 +154,10 @@ void Closure::closure_invalidated() { } void Closure::closure_set_invalid() { - gjs_debug_closure("Invalidating signal closure %p which calls function %p", - this, m_func.debug_addr()); + gjs_debug_closure("Invalidating signal closure %p which calls callable %p", + this, m_callable.debug_addr()); - m_func.prevent_collection(); + m_callable.prevent_collection(); reset(); GJS_DEC_COUNTER(closure); @@ -166,13 +166,13 @@ void Closure::closure_set_invalid() { bool Closure::invoke(JS::HandleObject this_obj, const JS::HandleValueArray& args, JS::MutableHandleValue retval) { - if (!m_func) { + if (!m_callable) { /* We were destroyed; become a no-op */ reset(); return false; } - JSAutoRealm ar(m_cx, JS_GetFunctionObject(m_func)); + JSAutoRealm ar(m_cx, m_callable); if (gjs_log_exception(m_cx)) { gjs_debug_closure( @@ -181,8 +181,8 @@ bool Closure::invoke(JS::HandleObject this_obj, this); } - JS::RootedFunction func(m_cx, m_func); - bool ok = JS::Call(m_cx, this_obj, func, args, retval); + JS::RootedValue v_callable(m_cx, JS::ObjectValue(*m_callable)); + bool ok = JS::Call(m_cx, this_obj, v_callable, args, retval); GjsContextPrivate* gjs = GjsContextPrivate::from_cx(m_cx); if (gjs->should_exit(nullptr)) @@ -191,8 +191,8 @@ bool Closure::invoke(JS::HandleObject this_obj, /* Exception thrown... */ gjs_debug_closure( "Closure invocation failed (exception should have been thrown) " - "closure %p function %p", - this, m_func.debug_addr()); + "closure %p callable %p", + this, m_callable.debug_addr()); return false; } diff --git a/gi/closure.h b/gi/closure.h index a13e040a..bf074c67 100644 --- a/gi/closure.h +++ b/gi/closure.h @@ -28,7 +28,7 @@ namespace Gjs { class Closure : public GClosure { protected: - Closure(JSContext*, JSFunction*, bool root, const char* description); + Closure(JSContext*, JSObject* callable, bool root, const char* description); ~Closure() { unset_context(); } // Need to call this if inheriting from Closure to call the dtor @@ -59,7 +59,7 @@ class Closure : public GClosure { return static_cast(static_cast(gclosure)); } - [[nodiscard]] static Closure* create(JSContext* cx, JSFunction* callable, + [[nodiscard]] static Closure* create(JSContext* cx, JSObject* callable, const char* description, bool root) { auto* self = new Closure(cx, callable, root, description); self->add_finalize_notifier(); @@ -67,7 +67,7 @@ class Closure : public GClosure { } [[nodiscard]] static Closure* create_marshaled(JSContext* cx, - JSFunction* callable, + JSObject* callable, const char* description) { auto* self = new Closure(cx, callable, true /* root */, description); self->add_finalize_notifier(); @@ -76,7 +76,7 @@ class Closure : public GClosure { } [[nodiscard]] static Closure* create_for_signal(JSContext* cx, - JSFunction* callable, + JSObject* callable, const char* description, int signal_id) { auto* self = new Closure(cx, callable, false /* root */, description); @@ -86,7 +86,7 @@ class Closure : public GClosure { return self; } - constexpr JSFunction* callable() const { return m_func; } + constexpr JSObject* callable() const { return m_callable; } [[nodiscard]] constexpr JSContext* context() const { return m_cx; } [[nodiscard]] constexpr bool is_valid() const { return !!m_cx; } GJS_JSAPI_RETURN_CONVENTION bool invoke(JS::HandleObject, @@ -94,8 +94,8 @@ class Closure : public GClosure { JS::MutableHandleValue); void trace(JSTracer* tracer) { - if (m_func) - m_func.trace(tracer, "signal connection"); + if (m_callable) + m_callable.trace(tracer, "signal connection"); } private: @@ -103,7 +103,7 @@ class Closure : public GClosure { void reset() { unset_context(); - m_func.reset(); + m_callable.reset(); m_cx = nullptr; } @@ -127,7 +127,7 @@ class Closure : public GClosure { // The context could be attached to the default context of the runtime // using if we wanted the closure to survive the context that created it. JSContext* m_cx; - GjsMaybeOwned m_func; + GjsMaybeOwned m_callable; }; } // namespace Gjs diff --git a/gi/function.cpp b/gi/function.cpp index 08a0ea25..32c7e757 100644 --- a/gi/function.cpp +++ b/gi/function.cpp @@ -18,6 +18,7 @@ #include #include +#include // for IsCallable #include #include #include // for JS_ReportOutOfMemory @@ -33,6 +34,7 @@ #include #include #include // for HandleValueArray +#include // for JS_GetObjectFunction #include // for JSProtoKey #include "gi/arg-cache.h" @@ -313,7 +315,7 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) { return; } - JSAutoRealm ar(context, JS_GetFunctionObject(callable())); + JSAutoRealm ar(context, callable()); int n_args = m_param_types.size(); g_assert(n_args >= 0); @@ -378,10 +380,12 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) { } // Some other uncatchable exception, e.g. out of memory - JSFunction* fn = callable(); - g_error("Function %s (%s.%s) terminated with uncatchable exception", - gjs_debug_string(JS_GetFunctionDisplayId(fn)).c_str(), - m_info.ns(), m_info.name()); + JSFunction* fn = JS_GetObjectFunction(callable()); + std::string descr = + fn ? "function " + gjs_debug_string(JS_GetFunctionDisplayId(fn)) + : "callable object " + gjs_debug_object(callable()); + g_error("Call to %s (%s.%s) terminated with uncatchable exception", + descr.c_str(), m_info.ns(), m_info.name()); } // Fill in the result with some hopefully neutral value @@ -541,12 +545,14 @@ bool GjsCallbackTrampoline::callback_closure_inner( return false; if (!is_array) { - JSFunction* fn = callable(); + JSFunction* fn = JS_GetObjectFunction(callable()); + std::string descr = + fn ? "function " + gjs_debug_string(JS_GetFunctionDisplayId(fn)) + : "callable object " + gjs_debug_object(callable()); gjs_throw(context, - "Function %s (%s.%s) returned unexpected value, " - "expecting an Array", - gjs_debug_string(JS_GetFunctionDisplayId(fn)).c_str(), - m_info.ns(), m_info.name()); + "Call to %s (%s.%s) returned unexpected value, expecting " + "an Array", + descr.c_str(), m_info.ns(), m_info.name()); return false; } @@ -597,11 +603,13 @@ bool GjsCallbackTrampoline::callback_closure_inner( } GjsCallbackTrampoline* GjsCallbackTrampoline::create( - JSContext* cx, JS::HandleFunction function, GICallableInfo* callable_info, + JSContext* cx, JS::HandleObject callable, GICallableInfo* callable_info, GIScopeType scope, bool has_scope_object, bool is_vfunc) { - g_assert(function); + g_assert(JS::IsCallable(callable) && + "tried to create a callback trampoline for a non-callable object"); + auto* trampoline = new GjsCallbackTrampoline( - cx, function, callable_info, scope, has_scope_object, is_vfunc); + cx, callable, callable_info, scope, has_scope_object, is_vfunc); if (!trampoline->initialize()) { g_closure_unref(trampoline); @@ -615,13 +623,13 @@ decltype(GjsCallbackTrampoline::s_forever_closure_list) GjsCallbackTrampoline::s_forever_closure_list; GjsCallbackTrampoline::GjsCallbackTrampoline( - JSContext* cx, JS::HandleFunction function, GICallableInfo* callable_info, + JSContext* cx, JS::HandleObject callable, GICallableInfo* callable_info, GIScopeType scope, bool has_scope_object, bool is_vfunc) // The rooting 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 - : Closure(cx, function, + : Closure(cx, callable, scope != GI_SCOPE_TYPE_NOTIFIED || !has_scope_object, g_base_info_get_name(callable_info)), m_info(callable_info, GjsAutoTakeOwnership()), diff --git a/gi/function.h b/gi/function.h index 5db20ec8..dfc19a99 100644 --- a/gi/function.h +++ b/gi/function.h @@ -43,9 +43,8 @@ using GjsAutoGClosure = struct GjsCallbackTrampoline : public Gjs::Closure { GJS_JSAPI_RETURN_CONVENTION static GjsCallbackTrampoline* create( - JSContext* cx, JS::HandleFunction function, - GICallableInfo* callable_info, GIScopeType scope, bool has_scope_object, - bool is_vfunc); + JSContext* cx, JS::HandleObject callable, GICallableInfo* callable_info, + GIScopeType scope, bool has_scope_object, bool is_vfunc); ~GjsCallbackTrampoline(); @@ -68,7 +67,7 @@ struct GjsCallbackTrampoline : public Gjs::Closure { private: ffi_closure* create_closure(); GJS_JSAPI_RETURN_CONVENTION bool initialize(); - GjsCallbackTrampoline(JSContext* cx, JS::HandleFunction function, + GjsCallbackTrampoline(JSContext* cx, JS::HandleObject callable, GICallableInfo* callable_info, GIScopeType scope, bool has_scope_object, bool is_vfunc); diff --git a/gi/object.cpp b/gi/object.cpp index c9b00dbe..ffb678ae 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -2187,7 +2187,7 @@ ObjectInstance::connect_impl(JSContext *context, } GClosure* closure = Gjs::Closure::create_for_signal( - context, JS_GetObjectFunction(callback), "signal callback", signal_id); + context, callback, "signal callback", signal_id); if (closure == NULL) return false; if (!associate_closure(context, closure)) @@ -2285,7 +2285,7 @@ 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, - JS::MutableHandleFunction func_out) { + JS::MutableHandleObject callable_out) { g_assert(mask_out && signal_id_out && detail_out && "forgot out parameter"); int mask = 0; @@ -2328,7 +2328,7 @@ bool ObjectInstance::signal_match_arguments_from_object( } bool has_func; - JS::RootedFunction func(cx); + JS::RootedObject callable(cx); if (!JS_HasOwnPropertyById(cx, match_obj, atoms.func(), &has_func)) return false; if (has_func) { @@ -2338,12 +2338,12 @@ bool ObjectInstance::signal_match_arguments_from_object( if (!JS_GetPropertyById(cx, match_obj, atoms.func(), &value)) return false; - if (!value.isObject() || !JS_ObjectIsFunction(&value.toObject())) { + if (!value.isObject() || !JS::IsCallable(&value.toObject())) { gjs_throw(cx, "'func' property must be a function"); return false; } - func = JS_GetObjectFunction(&value.toObject()); + callable = &value.toObject(); } if (!has_id && !has_detail && !has_func) { @@ -2357,7 +2357,7 @@ bool ObjectInstance::signal_match_arguments_from_object( if (has_detail) *detail_out = detail; if (has_func) - func_out.set(func); + callable_out.set(callable); return true; } @@ -2386,18 +2386,18 @@ bool ObjectInstance::signal_find_impl(JSContext* cx, const JS::CallArgs& args) { GSignalMatchType mask; unsigned signal_id; GQuark detail; - JS::RootedFunction func(cx); + JS::RootedObject callable(cx); if (!signal_match_arguments_from_object(cx, match, &mask, &signal_id, - &detail, &func)) + &detail, &callable)) return false; uint64_t handler = 0; - if (!func) { + if (!callable) { handler = g_signal_handler_find(m_ptr, mask, signal_id, detail, nullptr, nullptr, nullptr); } else { for (GClosure* candidate : m_closures) { - if (Gjs::Closure::for_gclosure(candidate)->callable() == func) { + if (Gjs::Closure::for_gclosure(candidate)->callable() == callable) { handler = g_signal_handler_find(m_ptr, mask, signal_id, detail, candidate, nullptr, nullptr); if (handler != 0) @@ -2461,19 +2461,19 @@ bool ObjectInstance::signals_action_impl(JSContext* cx, GSignalMatchType mask; unsigned signal_id; GQuark detail; - JS::RootedFunction func(cx); + JS::RootedObject callable(cx); if (!signal_match_arguments_from_object(cx, match, &mask, &signal_id, - &detail, &func)) { + &detail, &callable)) { return false; } unsigned n_matched = 0; - if (!func) { + if (!callable) { n_matched = MatchFunc(m_ptr, mask, signal_id, detail, nullptr, nullptr, nullptr); } else { std::vector candidates; for (GClosure* candidate : m_closures) { - if (Gjs::Closure::for_gclosure(candidate)->callable() == func) + if (Gjs::Closure::for_gclosure(candidate)->callable() == callable) candidates.push_back(candidate); } for (GClosure* candidate : candidates) { @@ -2914,10 +2914,9 @@ bool ObjectBase::hook_up_vfunc(JSContext* cx, unsigned argc, JS::Value* vp) { bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx, const JS::CallArgs& args) { JS::UniqueChars name; - JS::RootedObject function(cx); - if (!gjs_parse_call_args(cx, "hook_up_vfunc", args, "so", - "name", &name, - "function", &function)) + JS::RootedObject callable(cx); + if (!gjs_parse_call_args(cx, "hook_up_vfunc", args, "so", "name", &name, + "callable", &callable)) return false; args.rval().setUndefined(); @@ -3017,13 +3016,12 @@ 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); - if (!js::IsFunctionObject(function)) { - gjs_throw(cx, "Tried to deal with a vfunc that wasn't a function"); + if (!JS::IsCallable(callable)) { + gjs_throw(cx, "Tried to deal with a vfunc that wasn't callable"); return false; } - JS::RootedFunction func(cx, JS_GetObjectFunction(function)); trampoline = GjsCallbackTrampoline::create( - cx, func, vfunc, GI_SCOPE_TYPE_NOTIFIED, true, true); + cx, callable, vfunc, GI_SCOPE_TYPE_NOTIFIED, true, true); if (!trampoline) return false; diff --git a/gi/object.h b/gi/object.h index e6016cb7..d912220e 100644 --- a/gi/object.h +++ b/gi/object.h @@ -380,12 +380,10 @@ class ObjectInstance : public GIWrapperInstance #include #include // for InformalValueTypeName, JS_Get... +#include // for JS_GetObjectFunction #include "gi/arg-inl.h" #include "gi/arg.h" @@ -160,8 +161,7 @@ void Gjs::Closure::marshal(GValue* return_value, unsigned n_param_values, return; } - JSFunction* func = callable(); - JSAutoRealm ar(context, JS_GetFunctionObject(func)); + JSAutoRealm ar(context, callable()); if (marshal_data) { /* we are used for a signal handler */ @@ -273,9 +273,12 @@ void Gjs::Closure::marshal(GValue* return_value, unsigned n_param_values, exit(code); // Some other uncatchable exception, e.g. out of memory - JSFunction* fn = callable(); - g_error("Function %s terminated with uncatchable exception", - gjs_debug_string(JS_GetFunctionDisplayId(fn)).c_str()); + JSFunction* fn = JS_GetObjectFunction(callable()); + std::string descr = + fn ? "function " + gjs_debug_string(JS_GetFunctionDisplayId(fn)) + : "callable object " + gjs_debug_object(callable()); + g_error("Call to %s terminated with uncatchable exception", + descr.c_str()); } } -- cgit v1.2.1