diff options
author | Marco Trevisan (TreviƱo) <mail@3v1n0.net> | 2023-02-23 01:58:11 +0100 |
---|---|---|
committer | Philip Chimento <philip.chimento@gmail.com> | 2023-03-04 11:40:31 -0800 |
commit | 85cd91cf19b8bbfd466924c4ebd292d91cb03453 (patch) | |
tree | 4a603f125aa7920d0cbf5ec853e5b53ef3212384 | |
parent | e8aa880e07a273ac4693158045499f0e4dd2a84a (diff) | |
download | gjs-85cd91cf19b8bbfd466924c4ebd292d91cb03453.tar.gz |
function: Do not leak strings returned by transfer-none trampolines
When a VFunc or a callback returns a transfer-none string we were
breaking the introspection because it was causing a leak.
Now, in case these strings are returned by vfunc's of an object, we can
just safely associate them to the object using qdata, while in case
we're handling a callback we can use GLib internal strings to hold these
strings just once in memory. While this is technically still a waste
we're leaking just once, so it shouldn't be a big deal.
Closes: #519
-rw-r--r-- | gi/function.cpp | 40 | ||||
-rw-r--r-- | gi/function.h | 6 | ||||
-rw-r--r-- | gi/object.cpp | 14 | ||||
-rw-r--r-- | gi/object.h | 6 | ||||
-rw-r--r-- | installed-tests/js/testGIMarshalling.js | 12 |
5 files changed, 51 insertions, 27 deletions
diff --git a/gi/function.cpp b/gi/function.cpp index e6672e05..1629876b 100644 --- a/gi/function.cpp +++ b/gi/function.cpp @@ -342,8 +342,9 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) { AutoCallbackData callback_data(this, gjs); JS::RootedObject this_object(context); int c_args_offset = 0; + GObject* gobj = nullptr; if (m_is_vfunc) { - GObject* gobj = G_OBJECT(gjs_arg_get<GObject*>(args[0])); + gobj = G_OBJECT(gjs_arg_get<GObject*>(args[0])); if (gobj) { this_object = ObjectInstance::wrapper_from_gobject(context, gobj); if (!this_object) { @@ -366,8 +367,8 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) { g_callable_info_load_return_type(m_info, &ret_type); - if (!callback_closure_inner(context, this_object, &rval, args, &ret_type, - n_args, c_args_offset, result)) { + if (!callback_closure_inner(context, this_object, gobj, &rval, args, + &ret_type, n_args, c_args_offset, result)) { if (!JS_IsExceptionPending(context)) { // "Uncatchable" exception thrown, we have to exit. We may be in a // main loop, or maybe not, but there's no way to tell, so we have @@ -423,7 +424,7 @@ inline GIArgument* get_argument_for_arg_info(GIArgInfo* arg_info, } bool GjsCallbackTrampoline::callback_closure_inner( - JSContext* context, JS::HandleObject this_object, + JSContext* context, JS::HandleObject this_object, GObject* gobject, JS::MutableHandleValue rval, GIArgument** args, GITypeInfo* ret_type, int n_args, int c_args_offset, void* result) { int n_outargs = 0; @@ -432,7 +433,8 @@ bool GjsCallbackTrampoline::callback_closure_inner( if (!jsargs.reserve(n_args)) g_error("Unable to reserve space for vector"); - bool ret_type_is_void = g_type_info_get_tag(ret_type) == GI_TYPE_TAG_VOID; + GITypeTag ret_tag = g_type_info_get_tag(ret_type); + bool ret_type_is_void = ret_tag == GI_TYPE_TAG_VOID; for (int i = 0, n_jsargs = 0; i < n_args; i++) { GIArgInfo arg_info; @@ -575,6 +577,21 @@ bool GjsCallbackTrampoline::callback_closure_inner( &argument)) return false; + if ((ret_tag == GI_TYPE_TAG_FILENAME || + ret_tag == GI_TYPE_TAG_UTF8) && + transfer == GI_TRANSFER_NOTHING) { + // We duplicated the string so not to leak we need to both + // ensure that the string is bound to the object lifetime or + // created once + if (gobject) { + ObjectInstance::associate_string( + gobject, gjs_arg_get<char*>(&argument)); + } else { + GjsAutoChar str = gjs_arg_steal<char*>(&argument); + gjs_arg_set<const char*>(&argument, g_intern_string(str)); + } + } + set_return_ffi_arg_from_giargument(ret_type, result, &argument); elem_idx++; @@ -679,19 +696,6 @@ bool GjsCallbackTrampoline::initialize() { g_assert(is_valid()); g_assert(!m_closure); - GITypeInfo return_type; - g_callable_info_load_return_type(m_info, &return_type); - GITypeTag return_tag = g_type_info_get_tag(&return_type); - if (g_callable_info_get_caller_owns(m_info) == GI_TRANSFER_NOTHING && - (return_tag == GI_TYPE_TAG_FILENAME || - return_tag == GI_TYPE_TAG_UTF8)) { - gjs_throw(context(), - "%s %s returns a transfer-none string. This is not supported " - "(https://gitlab.gnome.org/GNOME/gjs/-/issues/519)", - m_is_vfunc ? "VFunc" : "Callback", m_info.name()); - return false; - } - /* Analyze param types and directions, similarly to * init_cached_function_data */ int n_param_types = g_callable_info_get_n_args(m_info); diff --git a/gi/function.h b/gi/function.h index 7cb433b6..c875ecbd 100644 --- a/gi/function.h +++ b/gi/function.h @@ -76,9 +76,9 @@ struct GjsCallbackTrampoline : public Gjs::Closure { void callback_closure(GIArgument** args, void* result); GJS_JSAPI_RETURN_CONVENTION bool callback_closure_inner(JSContext* cx, JS::HandleObject this_object, - JS::MutableHandleValue rval, GIArgument** args, - GITypeInfo* ret_type, int n_args, - int c_args_offset, void* result); + GObject* gobject, JS::MutableHandleValue rval, + GIArgument** args, GITypeInfo* ret_type, + int n_args, int c_args_offset, void* result); void warn_about_illegal_js_callback(const char* when, const char* reason); static std::vector<GjsAutoGClosure> s_forever_closure_list; diff --git a/gi/object.cpp b/gi/object.cpp index a878cf3c..62e79c18 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -99,6 +99,7 @@ static JSObject* gjs_lookup_object_prototype_from_info(JSContext*, // clang-format off G_DEFINE_QUARK(gjs::custom-type, ObjectBase::custom_type) G_DEFINE_QUARK(gjs::custom-property, ObjectBase::custom_property) +G_DEFINE_QUARK(gjs::instance-strings, ObjectBase::instance_strings) G_DEFINE_QUARK(gjs::disposed, ObjectBase::disposed) // clang-format on @@ -3065,3 +3066,16 @@ gjs_lookup_object_constructor(JSContext *context, value_p.setObject(*constructor); return true; } + +void ObjectInstance::associate_string(GObject* obj, char* str) { + auto* instance_strings = static_cast<GPtrArray*>( + g_object_get_qdata(obj, ObjectBase::instance_strings_quark())); + + if (!instance_strings) { + instance_strings = g_ptr_array_new_with_free_func(g_free); + g_object_set_qdata_full( + obj, ObjectBase::instance_strings_quark(), instance_strings, + reinterpret_cast<GDestroyNotify>(g_ptr_array_unref)); + } + g_ptr_array_add(instance_strings, str); +} diff --git a/gi/object.h b/gi/object.h index d912220e..1ef680eb 100644 --- a/gi/object.h +++ b/gi/object.h @@ -157,6 +157,10 @@ class ObjectBase /* Quarks */ + protected: + [[nodiscard]] static GQuark instance_strings_quark(); + + public: [[nodiscard]] static GQuark custom_type_quark(); [[nodiscard]] static GQuark custom_property_quark(); [[nodiscard]] static GQuark disposed_quark(); @@ -394,6 +398,8 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype, bool init_custom_class_from_gobject(JSContext* cx, JS::HandleObject wrapper, GObject* gobj); + static void associate_string(GObject* obj, char* str); + /* Methods to manipulate the linked list of instances */ private: diff --git a/installed-tests/js/testGIMarshalling.js b/installed-tests/js/testGIMarshalling.js index 2140f67b..1df1aadb 100644 --- a/installed-tests/js/testGIMarshalling.js +++ b/installed-tests/js/testGIMarshalling.js @@ -1258,10 +1258,9 @@ let VFuncTester = GObject.registerClass(class VFuncTester extends GIMarshallingT return i + 4; } - // https://gitlab.gnome.org/GNOME/gjs/-/issues/519 - // vfunc_method_str_arg_out_ret(s) { - // return [`Called with ${s}`, 41]; - // } + vfunc_method_str_arg_out_ret(s) { + return [`Called with ${s}`, 41]; + } vfunc_method_with_default_implementation(i) { this.int = i + 2; @@ -1435,9 +1434,10 @@ describe('Virtual function', function () { expect(tester.method_int8_arg_and_out_callee(38)).toEqual(42); }); - xit('marshals a string out argument and return value', function () { + it('marshals a string out argument and return value', function () { expect(tester.method_str_arg_out_ret('a string')).toEqual(['Called with a string', 41]); - }).pend('https://gitlab.gnome.org/GNOME/gjs/-/issues/519'); + expect(tester.method_str_arg_out_ret('a 2nd string')).toEqual(['Called with a 2nd string', 41]); + }); it('can override a default implementation in JS', function () { tester.method_with_default_implementation(40); |