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

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);