GNOME.org
 

[gjs: 1/2] object: Block access to object only if it is finalized


commit 841a72bcc07958564addb6bb65ed34a6eb46b773
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Thu Apr 22 23:10:19 2021 +0200

    object: Block access to object only if it is finalized
    
    For some time now we're warning whether a disposed object is used and
    this is sane, however the data it holds may be still valid and in
    general all the C code we access to is meant to be used after
    disposition. While such code has not to be called when the object is
    finalized.
    
    Since now we properly track objects finalization we can go back to allow
    most of the object actions (all but the signals connections, these
    involve having toggle notifications that are not a tool we can use
    at this point) with the actual object pointer.
    
    In case the object is finalized instead we behave exactly as before.
    
    Adapted tests and added ones for finalization case

 gi/object.cpp                                      |  35 ++--
 gi/object.h                                        |   4 +-
 installed-tests/js/testGObjectDestructionAccess.js | 202 +++++++++++++++++++--
 installed-tests/js/testGtk3.js                     |   4 +-
 4 files changed, 211 insertions(+), 34 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index e253b843..920d8c15 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -188,20 +188,29 @@ bool ObjectBase::typecheck(JSContext* cx, JS::HandleObject obj,
     return false;
 }
 
-bool ObjectInstance::check_gobject_disposed(const char* for_what) const {
+bool ObjectInstance::check_gobject_disposed_or_finalized(
+    const char* for_what) const {
     if (!m_gobj_disposed)
         return true;
 
     g_critical(
-        "Object %s.%s (%p), has been already deallocated — impossible to %s "
+        "Object %s.%s (%p), has been already %s — impossible to %s "
         "it. This might be caused by the object having been destroyed from C "
         "code using something such as destroy(), dispose(), or remove() "
         "vfuncs.",
-        ns(), name(), m_ptr.get(), for_what);
+        ns(), name(), m_ptr.get(), m_gobj_finalized ? "finalized" : "disposed",
+        for_what);
     gjs_dumpstack();
     return false;
 }
 
+bool ObjectInstance::check_gobject_finalized(const char* for_what) const {
+    if (check_gobject_disposed_or_finalized(for_what))
+        return true;
+
+    return !m_gobj_finalized;
+}
+
 ObjectInstance *
 ObjectInstance::for_gobject(GObject *gobj)
 {
@@ -349,7 +358,7 @@ bool ObjectBase::prop_getter(JSContext* cx, unsigned argc, JS::Value* vp) {
 
 bool ObjectInstance::prop_getter_impl(JSContext* cx, JS::HandleString name,
                                       JS::MutableHandleValue rval) {
-    if (!check_gobject_disposed("get any property from")) {
+    if (!check_gobject_finalized("get any property from")) {
         rval.setUndefined();
         return true;
     }
@@ -426,7 +435,7 @@ bool ObjectBase::field_getter(JSContext* cx, unsigned argc, JS::Value* vp) {
 
 bool ObjectInstance::field_getter_impl(JSContext* cx, JS::HandleString name,
                                        JS::MutableHandleValue rval) {
-    if (!check_gobject_disposed("get any property from"))
+    if (!check_gobject_finalized("get any property from"))
         return true;
 
     ObjectPrototype* proto_priv = get_prototype();
@@ -495,7 +504,7 @@ bool ObjectBase::prop_setter(JSContext* cx, unsigned argc, JS::Value* vp) {
 
 bool ObjectInstance::prop_setter_impl(JSContext* cx, JS::HandleString name,
                                       JS::HandleValue value) {
-    if (!check_gobject_disposed("set any property on"))
+    if (!check_gobject_finalized("set any property on"))
         return true;
 
     ObjectPrototype* proto_priv = get_prototype();
@@ -558,7 +567,7 @@ bool ObjectBase::field_setter(JSContext* cx, unsigned argc, JS::Value* vp) {
 
 bool ObjectInstance::field_setter_not_impl(JSContext* cx,
                                            JS::HandleString name) {
-    if (!check_gobject_disposed("set GObject field on"))
+    if (!check_gobject_finalized("set GObject field on"))
         return true;
 
     ObjectPrototype* proto_priv = get_prototype();
@@ -1542,7 +1551,7 @@ bool ObjectInstance::ensure_uses_toggle_ref(JSContext* cx) {
     if (m_uses_toggle_ref)
         return true;
 
-    if (!check_gobject_disposed("add toggle reference on"))
+    if (!check_gobject_disposed_or_finalized("add toggle reference on"))
         return false;
 
     debug_lifecycle("Switching object instance to toggle ref");
@@ -1983,7 +1992,7 @@ ObjectInstance::connect_impl(JSContext          *context,
 
     gjs_debug_gsignal("connect obj %p priv %p", m_wrapper.get(), this);
 
-    if (!check_gobject_disposed("connect to any signal on")) {
+    if (!check_gobject_disposed_or_finalized("connect to any signal on")) {
         args.rval().setInt32(0);
         return true;
     }
@@ -2048,7 +2057,7 @@ ObjectInstance::emit_impl(JSContext          *context,
     gjs_debug_gsignal("emit obj %p priv %p argc %d", m_wrapper.get(), this,
                       argv.length());
 
-    if (!check_gobject_disposed("emit any signal on")) {
+    if (!check_gobject_finalized("emit any signal on")) {
         argv.rval().setUndefined();
         return true;
     }
@@ -2214,7 +2223,7 @@ bool ObjectInstance::signal_find_impl(JSContext* cx, const JS::CallArgs& args) {
     gjs_debug_gsignal("[Gi.signal_find_symbol]() obj %p priv %p argc %d",
                       m_wrapper.get(), this, args.length());
 
-    if (!check_gobject_disposed("find any signal on")) {
+    if (!check_gobject_finalized("find any signal on")) {
         args.rval().setInt32(0);
         return true;
     }
@@ -2290,7 +2299,7 @@ bool ObjectInstance::signals_action_impl(JSContext* cx,
     gjs_debug_gsignal("[%s]() obj %p priv %p argc %d", action_tag.c_str(),
                       m_wrapper.get(), this, args.length());
 
-    if (!check_gobject_disposed((action_name + " any signal on").c_str())) {
+    if (!check_gobject_finalized((action_name + " any signal on").c_str())) {
         args.rval().setInt32(0);
         return true;
     }
@@ -2582,7 +2591,7 @@ bool ObjectBase::to_c_ptr(JSContext* cx, JS::HandleObject obj, GObject** ptr) {
         return false;
 
     ObjectInstance* instance = priv->to_instance();
-    if (!instance->check_gobject_disposed("access")) {
+    if (!instance->check_gobject_finalized("access")) {
         *ptr = nullptr;
         return true;
     }
diff --git a/gi/object.h b/gi/object.h
index fd04caf6..abe2655b 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -383,7 +383,9 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     void ignore_gobject_finalization();
     void check_js_object_finalized(void);
     bool ensure_uses_toggle_ref(JSContext* cx);
-    [[nodiscard]] bool check_gobject_disposed(const char* for_what) const;
+    [[nodiscard]] bool check_gobject_disposed_or_finalized(
+        const char* for_what) const;
+    [[nodiscard]] bool check_gobject_finalized(const char* for_what) const;
     GJS_JSAPI_RETURN_CONVENTION
     bool signal_match_arguments_from_object(JSContext* cx,
                                             JS::HandleObject props_obj,
diff --git a/installed-tests/js/testGObjectDestructionAccess.js 
b/installed-tests/js/testGObjectDestructionAccess.js
index 1ec25c14..ca647ac4 100644
--- a/installed-tests/js/testGObjectDestructionAccess.js
+++ b/installed-tests/js/testGObjectDestructionAccess.js
@@ -16,14 +16,15 @@ describe('Access to destroyed GObject', function () {
 
     beforeEach(function () {
         destroyedWindow = new Gtk.Window({type: Gtk.WindowType.TOPLEVEL});
+        destroyedWindow.set_title('To be destroyed');
         destroyedWindow.destroy();
     });
 
     it('Get property', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
 
-        expect(destroyedWindow.title).toBeUndefined();
+        expect(destroyedWindow.title).toBe('To be destroyed');
 
         GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
             'testExceptionInDestroyedObjectPropertyGet');
@@ -31,9 +32,12 @@ describe('Access to destroyed GObject', function () {
 
     it('Set property', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* disposed *');
 
         destroyedWindow.title = 'I am dead';
+        expect(destroyedWindow.title).toBe('I am dead');
 
         GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
             'testExceptionInDestroyedObjectPropertySet');
@@ -41,7 +45,7 @@ describe('Access to destroyed GObject', function () {
 
     it('Add expando property', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
 
         destroyedWindow.expandoProperty = 'Hello!';
 
@@ -63,11 +67,9 @@ describe('Access to destroyed GObject', function () {
 
     it('Access to getter method', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
-        GLib.test_expect_message('Gtk', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            '*GTK_IS_WINDOW*');
+            'Object Gtk.Window (0x* disposed *');
 
-        expect(destroyedWindow.get_title()).toBeNull();
+        expect(destroyedWindow.get_title()).toBe('To be destroyed');
 
         GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
             'testExceptionInDestroyedObjectMethodGet');
@@ -75,11 +77,12 @@ describe('Access to destroyed GObject', function () {
 
     it('Access to setter method', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
-        GLib.test_expect_message('Gtk', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            '*GTK_IS_WINDOW*');
+            'Object Gtk.Window (0x* disposed *');
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* disposed *');
 
         destroyedWindow.set_title('I am dead');
+        expect(destroyedWindow.get_title()).toBe('I am dead');
 
         GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
             'testExceptionInDestroyedObjectMethodSet');
@@ -87,7 +90,7 @@ describe('Access to destroyed GObject', function () {
 
     it('Proto function connect', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
 
         expect(destroyedWindow.connect('foo-signal', () => {})).toBe(0);
 
@@ -97,7 +100,7 @@ describe('Access to destroyed GObject', function () {
 
     it('Proto function connect_after', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
 
         expect(destroyedWindow.connect_after('foo-signal', () => {})).toBe(0);
 
@@ -107,9 +110,9 @@ describe('Access to destroyed GObject', function () {
 
     it('Proto function emit', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
 
-        expect(destroyedWindow.emit('foo-signal')).toBeUndefined();
+        expect(destroyedWindow.emit('keys-changed')).toBeUndefined();
 
         GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
             'testExceptionInDestroyedObjectEmit');
@@ -117,7 +120,7 @@ describe('Access to destroyed GObject', function () {
 
     it('Proto function signals_disconnect', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
 
         expect(GObject.signal_handlers_disconnect_by_func(destroyedWindow, () => {})).toBe(0);
 
@@ -127,7 +130,7 @@ describe('Access to destroyed GObject', function () {
 
     it('Proto function signals_block', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
 
         expect(GObject.signal_handlers_block_by_func(destroyedWindow, () => {})).toBe(0);
 
@@ -137,7 +140,7 @@ describe('Access to destroyed GObject', function () {
 
     it('Proto function signals_unblock', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Window (0x*');
+            'Object Gtk.Window (0x* disposed *');
 
         expect(GObject.signal_handlers_unblock_by_func(destroyedWindow, () => {})).toBe(0);
 
@@ -163,6 +166,169 @@ describe('Access to destroyed GObject', function () {
     });
 });
 
+describe('Access to finalized GObject', function () {
+    let destroyedWindow;
+
+    beforeAll(function () {
+        Gtk.init(null);
+    });
+
+    beforeEach(function () {
+        destroyedWindow = new Gtk.Window({type: Gtk.WindowType.TOPLEVEL});
+        destroyedWindow.set_title('To be destroyed');
+        destroyedWindow.destroy();
+
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* disposed *');
+        destroyedWindow.unref();
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectPropertyGet');
+    });
+
+    afterEach(function () {
+        destroyedWindow = null;
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            '*Object 0x* has been finalized *');
+        System.gc();
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'generates a warn on object garbage collection');
+    });
+
+    it('Get property', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        expect(destroyedWindow.title).toBeUndefined();
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectPropertyGet');
+    });
+
+    it('Set property', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        destroyedWindow.title = 'I am dead';
+        expect(destroyedWindow.title).toBeUndefined();
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectPropertySet');
+    });
+
+    it('Add expando property', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        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* finalized *');
+        GLib.test_expect_message('Gtk', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            '*GTK_IS_WINDOW*');
+
+        expect(destroyedWindow.get_title()).toBeNull();
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectMethodGet');
+    });
+
+    it('Access to setter method', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+        GLib.test_expect_message('Gtk', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            '*GTK_IS_WINDOW*');
+
+        destroyedWindow.set_title('I am dead');
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectMethodSet');
+    });
+
+    it('Proto function connect', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        expect(destroyedWindow.connect('foo-signal', () => { })).toBe(0);
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectConnect');
+    });
+
+    it('Proto function connect_after', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        expect(destroyedWindow.connect_after('foo-signal', () => { })).toBe(0);
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectConnectAfter');
+    });
+
+    it('Proto function emit', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        expect(destroyedWindow.emit('keys-changed')).toBeUndefined();
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectEmit');
+    });
+
+    it('Proto function signals_disconnect', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        expect(GObject.signal_handlers_disconnect_by_func(destroyedWindow, () => { })).toBe(0);
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectSignalsDisconnect');
+    });
+
+    it('Proto function signals_block', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        expect(GObject.signal_handlers_block_by_func(destroyedWindow, () => { })).toBe(0);
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectSignalsBlock');
+    });
+
+    it('Proto function signals_unblock', function () {
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            'Object Gtk.Window (0x* finalized *');
+
+        expect(GObject.signal_handlers_unblock_by_func(destroyedWindow, () => { })).toBe(0);
+
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'testExceptionInDestroyedObjectSignalsUnblock');
+    });
+
+    it('Proto function toString', function () {
+        expect(destroyedWindow.toString()).toMatch(
+            /\[object \(FINALIZED\) instance wrapper GIName:Gtk.Window jsobj@0x[a-f0-9]+ 
native@0x[a-f0-9]+\]/);
+    });
+});
+
 describe('Disposed or finalized GObject', function () {
     beforeAll(function () {
         GjsTestTools.init();
diff --git a/installed-tests/js/testGtk3.js b/installed-tests/js/testGtk3.js
index a6ba2642..73f3ecb4 100644
--- a/installed-tests/js/testGtk3.js
+++ b/installed-tests/js/testGtk3.js
@@ -225,8 +225,8 @@ describe('Gtk overrides', function () {
         expect(handleDispose).toHaveBeenCalledWith(label);
 
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
-            'Object Gtk.Label (0x* deallocated *');
-        expect(label.label).toBeUndefined();
+            'Object Gtk.Label (0x* disposed *');
+        expect(label.label).toBe('Hello');
         GLib.test_assert_expected_messages_internal('Gjs', 'testGtk3.js', 0,
             'GTK destroy signal is emitted while disposing objects');
     });
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]