object: Never try to add a toggle reference on a disposed object

This should never happen as it would never be released afterwards, so
protect the call in all the cases, triggering a warning (but without
throwing as it's not a pure JS error) and return a bool so that we can
handle this case better (in fact if this happens we should not consider
the object toggle-reffed).

As per this we can now also warn about access to disposed object also
when trying to set a new expand property to a disposed object (adding
tests ensuring that this won't affect pre-set properties).

3 files changed, 44 insertions, 10 deletions

diff --git a/gi/object.cpp b/gi/object.cpp
index cdb85916..7c8cc732 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -320,10 +320,11 @@ bool ObjectInstance::add_property_impl(JSContext* cx, JS::HandleObject obj,
                                        JS::HandleId id, JS::HandleValue) {
     debug_jsprop("Add property hook", id, obj);
 
-    if (is_custom_js_class() || m_gobj_disposed)
+    if (is_custom_js_class())
         return true;
 
     ensure_uses_toggle_ref(cx);
+
     return true;
 }
 
@@ -1493,11 +1494,15 @@ ObjectInstance::associate_js_gobject(JSContext       *context,
         g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, this);
 }
 
-void
-ObjectInstance::ensure_uses_toggle_ref(JSContext *cx)
-{
+// The return value here isn't intended to be JS API like boolean, as we only
+// return whether the object has a toggle reference, and if we've added one
+// and depending on this callers may need to unref the object on failure.
+bool ObjectInstance::ensure_uses_toggle_ref(JSContext* cx) {
     if (m_uses_toggle_ref)
-        return;
+        return true;
+
+    if (!check_gobject_disposed("add toggle reference on"))
+        return false;
 
     debug_lifecycle("Switching object instance to toggle ref");
 
@@ -1522,6 +1527,8 @@ ObjectInstance::ensure_uses_toggle_ref(JSContext *cx)
      * This may immediately remove the GC root we just added, since refcount
      * may drop to 1. */
     g_object_unref(m_ptr);
+
+    return true;
 }
 
 static void invalidate_closure_list(std::forward_list<GClosure*>* closures) {
@@ -1635,10 +1642,11 @@ ObjectInstance::init_impl(JSContext              *context,
          * we're not actually using it, so just let it get collected. Avoiding
          * this would require a non-trivial amount of work.
          * */
-        other_priv->ensure_uses_toggle_ref(context);
+        if (!other_priv->ensure_uses_toggle_ref(context))
+            gobj = nullptr;
+
         object.set(other_priv->m_wrapper);
-        g_object_unref(gobj); /* We already own a reference */
-        gobj = NULL;
+        g_clear_object(&gobj); /* We already own a reference */
         return true;
     }
 
@@ -2435,7 +2443,11 @@ bool ObjectInstance::init_custom_class_from_gobject(JSContext* cx,
 
     // Custom JS objects will most likely have visible state, so just do this
     // from the start.
-    ensure_uses_toggle_ref(cx);
+    if (!ensure_uses_toggle_ref(cx)) {
+        gjs_throw(cx, "Impossible to set toggle references on %sobject %p",
+                  m_gobj_disposed ? "disposed " : "", gobj);
+        return false;
+    }
 
     const GjsAtoms& atoms = GjsContextPrivate::atoms(cx);
     JS::RootedValue v(cx);
diff --git a/gi/object.h b/gi/object.h
index 2e7b04fb..fd04caf6 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -382,7 +382,7 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     void track_gobject_finalization();
     void ignore_gobject_finalization();
     void check_js_object_finalized(void);
-    void ensure_uses_toggle_ref(JSContext* cx);
+    bool ensure_uses_toggle_ref(JSContext* cx);
     [[nodiscard]] bool check_gobject_disposed(const char* for_what) const;
     GJS_JSAPI_RETURN_CONVENTION
     bool signal_match_arguments_from_object(JSContext* cx,
diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js
index 3b711e81..423da44e 100644
--- a/installed-tests/js/testGObjectDestructionAccess.js
+++ b/installed-tests/js/testGObjectDestructionAccess.js
@@ -39,6 +39,28 @@ describe('Access to destroyed GObject', function () {
             'testExceptionInDestroyedObjectPropertySet');
     });
 
+    it('Add expando property', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x*');
+
+        destroyedWindow.expandoProperty = 'Hello!';
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectExpandoPropertySet');
+    });
+
+    it('Access to unset expando property', function () {
+        expect(destroyedWindow.expandoProperty).toBeUndefined();
+    });
+
+    it('Access previously set expando property', function () {
+        destroyedWindow = new Gtk.Window({type: Gtk.WindowType.TOPLEVEL});
+        destroyedWindow.expandoProperty = 'Hello!';
+        destroyedWindow.destroy();
+
+        expect(destroyedWindow.expandoProperty).toBe('Hello!');
+    });
+
     it('Access to getter method', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
             'Object Gtk.Window (0x*');