[gjs/ewlsh/fix-vfunc-clobberin] Store interface gtypes on dynamic classes to not clobber vfuncs

[Evan Welsh <ewlsh src gnome org>]

commit 14ffcd93f9123afb70e7f6124cce91e09eab80b8
Author: Evan Welsh <contact evanwelsh com>
Date:   Sat Aug 28 14:45:57 2021 -0700

    Store interface gtypes on dynamic classes to not clobber vfuncs
    
    We previously used g_type_interfaces to look up interfaces when
    hooking up vfuncs. Unfortunately, g_type_interfaces returns all
    interfaces a class adheres to, including ones its parent
    implements.
    
    This commit retains the g_type_interfaces usage to provide a
    helpful error message.
    
    Fixes #89

 gi/object.cpp                          | 82 +++++++++++++++++++++++++++-------
 gi/object.h                            |  6 +++
 gi/private.cpp                         |  3 +-
 gi/repo.cpp                            |  3 +-
 installed-tests/js/testGObjectClass.js | 47 +++++++++++++++++++
 5 files changed, 122 insertions(+), 19 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 77acf3f4..1798c638 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1820,7 +1820,7 @@ JSObject* gjs_lookup_object_constructor_from_info(JSContext* context,
         */
         JS::RootedObject ignored(context);
         if (!ObjectPrototype::define_class(context, in_object, nullptr, gtype,
-                                           &constructor, &ignored))
+                                           nullptr, 0, &constructor, &ignored))
             return nullptr;
     } else {
         if (G_UNLIKELY (!value.isObject()))
@@ -2393,15 +2393,21 @@ bool ObjectPrototype::get_parent_proto(JSContext* cx,
  * constructor and prototype objects as out parameters, for convenience
  * elsewhere.
  */
-bool ObjectPrototype::define_class(JSContext* context,
-                                   JS::HandleObject in_object,
-                                   GIObjectInfo* info, GType gtype,
-                                   JS::MutableHandleObject constructor,
-                                   JS::MutableHandleObject prototype) {
+bool ObjectPrototype::define_class(
+    JSContext* context, JS::HandleObject in_object, GIObjectInfo* info,
+    GType gtype, GType* interface_gtypes, uint32_t n_interface_gtypes,
+    JS::MutableHandleObject constructor, JS::MutableHandleObject prototype) {
     if (!ObjectPrototype::create_class(context, in_object, info, gtype,
                                        constructor, prototype))
         return false;
 
+    ObjectPrototype* priv = ObjectPrototype::for_js(context, prototype);
+    if (interface_gtypes) {
+        for (uint32_t n = 0; n < n_interface_gtypes; n++) {
+            priv->m_interface_gtypes.push_back(interface_gtypes[n]);
+        }
+    }
+
     // hook_up_vfunc and the signal handler matcher functions can't be included
     // in gjs_object_instance_proto_funcs because they are custom symbols.
     const GjsAtoms& atoms = GjsContextPrivate::atoms(context);
@@ -2689,20 +2695,26 @@ bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
      * This is awful, so abort now. */
     g_assert(info != NULL);
 
-    GjsAutoVFuncInfo vfunc = find_vfunc_on_parents(info, name.get(), nullptr);
-
-    if (!vfunc) {
-        guint i, n_interfaces;
-        GType *interface_list;
+    GjsAutoVFuncInfo vfunc = g_object_info_find_vfunc(info, name.get());
+    // Search the parent type chain
+    while (!vfunc && info_gtype != G_TYPE_OBJECT) {
+        info_gtype = g_type_parent(info_gtype);
 
-        interface_list = g_type_interfaces(m_gtype, &n_interfaces);
+        info = g_irepository_find_by_gtype(nullptr, info_gtype);
+        if (info)
+            vfunc = g_object_info_find_vfunc(info, name.get());
+    }
 
-        for (i = 0; i < n_interfaces; i++) {
+    // If the vfunc doesn't exist in the parent
+    // type chain, loop through the explicitly
+    // defined interfaces...
+    if (!vfunc) {
+        for (GType interface_gtype : m_interface_gtypes) {
             GjsAutoInterfaceInfo interface =
-                g_irepository_find_by_gtype(nullptr, interface_list[i]);
+                g_irepository_find_by_gtype(nullptr, interface_gtype);
 
-            /* The interface doesn't have to exist -- it could be private
-             * or dynamic. */
+            // Private and dynamic interfaces (defined in JS) do not have type
+            // info.
             if (interface) {
                 vfunc = g_interface_info_find_vfunc(interface, name.get());
 
@@ -2710,8 +2722,44 @@ bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
                     break;
             }
         }
+    }
+
+    // If the vfunc is still not found, it could exist on an interface
+    // implemented by a parent. This is an error, as hooking up the vfunc
+    // would create an implementation on the interface itself. In this
+    // case, print a more helpful error than...
+    // "Could not find definition of virtual function"
+    //
+    // See https://gitlab.gnome.org/GNOME/gjs/-/issues/89
+    if (!vfunc) {
+        unsigned n_interfaces;
+        GjsAutoPointer<GType> interface_list =
+            g_type_interfaces(m_gtype, &n_interfaces);
+
+        for (unsigned i = 0; i < n_interfaces; i++) {
+            GjsAutoInterfaceInfo interface =
+                g_irepository_find_by_gtype(nullptr, interface_list[i]);
 
-        g_free(interface_list);
+            if (interface) {
+                GjsAutoVFuncInfo parent_vfunc =
+                    g_interface_info_find_vfunc(interface, name.get());
+
+                if (parent_vfunc) {
+                    const char* interface_name =
+                        g_base_info_get_name(interface);
+                    const char* namespace_name =
+                        g_base_info_get_namespace(interface);
+                    GjsAutoChar identifier = g_strdup_printf(
+                        "%s.%s", namespace_name, interface_name);
+                    gjs_throw(cx,
+                              "%s does not implement %s, add %s to your "
+                              "implements array",
+                              g_type_name(m_gtype), identifier.get(),
+                              identifier.get());
+                    return false;
+                }
+            }
+        }
     }
 
     if (!vfunc) {
diff --git a/gi/object.h b/gi/object.h
index 0d9d40fe..d5e32e87 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -8,6 +8,7 @@
 #include <config.h>
 
 #include <stddef.h>  // for size_t
+#include <stdint.h>  // for uint32_t
 
 #include <forward_list>
 #include <functional>
@@ -198,6 +199,9 @@ class ObjectPrototype
     NegativeLookupCache m_unresolvable_cache;
     // a list of vfunc GClosures installed on this prototype, used when tracing
     std::forward_list<GClosure*> m_vfuncs;
+    // a list of interface types explicitly associated with this prototype,
+    // by gjs_add_interface
+    std::vector<GType> m_interface_gtypes;
 
     ObjectPrototype(GIObjectInfo* info, GType gtype);
     ~ObjectPrototype();
@@ -243,6 +247,8 @@ class ObjectPrototype
     GJS_JSAPI_RETURN_CONVENTION
     static bool define_class(JSContext* cx, JS::HandleObject in_object,
                              GIObjectInfo* info, GType gtype,
+                             GType* interface_gtypes,
+                             uint32_t n_interface_gtypes,
                              JS::MutableHandleObject constructor,
                              JS::MutableHandleObject prototype);
 
diff --git a/gi/private.cpp b/gi/private.cpp
index a6ecc723..33e9ff00 100644
--- a/gi/private.cpp
+++ b/gi/private.cpp
@@ -310,7 +310,8 @@ static bool gjs_register_type(JSContext* cx, unsigned argc, JS::Value* vp) {
     JS::RootedObject module(cx, gjs_lookup_private_namespace(cx));
     JS::RootedObject constructor(cx), prototype(cx);
     if (!ObjectPrototype::define_class(cx, module, nullptr, instance_type,
-                                       &constructor, &prototype))
+                                       iface_types, n_interfaces, &constructor,
+                                       &prototype))
         return false;
 
     auto* priv = ObjectPrototype::for_js(cx, prototype);
diff --git a/gi/repo.cpp b/gi/repo.cpp
index 87030fd4..70564e5c 100644
--- a/gi/repo.cpp
+++ b/gi/repo.cpp
@@ -368,7 +368,8 @@ gjs_define_info(JSContext       *context,
             } else if (g_type_is_a (gtype, G_TYPE_OBJECT)) {
                 JS::RootedObject ignored1(context), ignored2(context);
                 if (!ObjectPrototype::define_class(context, in_object, info,
-                                                   gtype, &ignored1, &ignored2))
+                                                   gtype, nullptr, 0, &ignored1,
+                                                   &ignored2))
                     return false;
             } else if (G_TYPE_IS_INSTANTIATABLE(gtype)) {
                 JS::RootedObject ignored(context);
diff --git a/installed-tests/js/testGObjectClass.js b/installed-tests/js/testGObjectClass.js
index 0b46d417..98c735b8 100644
--- a/installed-tests/js/testGObjectClass.js
+++ b/installed-tests/js/testGObjectClass.js
@@ -462,6 +462,53 @@ describe('GObject class with decorator', function () {
         expect(new Derived().toString()).toMatch(
             /\[object instance wrapper GType:Gjs_Derived jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/);
     });
+
+    it('does not clobber native parent interface vfunc definitions', function () {
+        const resetImplementationSpy = jasmine.createSpy('vfunc_reset');
+        expect(() => {
+            // This is a random interface in Gio with a virtual function
+            GObject.registerClass({
+                // Forgotten interface
+                // Implements: [Gio.Converter],
+            }, class MyZlibConverter extends Gio.ZlibCompressor {
+                vfunc_reset() {
+                    resetImplementationSpy();
+                }
+            });
+        }).toThrowError('Gjs_MyZlibConverter does not implement Gio.Converter, add Gio.Converter to your 
implements array');
+
+        let potentiallyClobbered = new Gio.ZlibCompressor();
+        potentiallyClobbered.reset();
+
+        expect(resetImplementationSpy).not.toHaveBeenCalled();
+    });
+
+    it('does not clobber dynamic parent interface vfunc definitions', function () {
+        const resetImplementationSpy = jasmine.createSpy('vfunc_reset');
+
+        const MyJSConverter = GObject.registerClass({
+            Implements: [Gio.Converter],
+        }, class MyJSConverter extends GObject.Object {
+            vfunc_reset() {
+            }
+        });
+
+        expect(() => {
+            GObject.registerClass({
+                // Forgotten interface
+                // Implements: [Gio.Converter],
+            }, class MyBadConverter extends MyJSConverter {
+                vfunc_reset() {
+                    resetImplementationSpy();
+                }
+            });
+        }).toThrowError('Gjs_MyBadConverter does not implement Gio.Converter, add Gio.Converter to your 
implements array');
+
+        let potentiallyClobbered = new MyJSConverter();
+        potentiallyClobbered.reset();
+
+        expect(resetImplementationSpy).not.toHaveBeenCalled();
+    });
 });
 
 describe('GObject virtual function', function () {