From 9b0232c0c728d2b6a668a263004883556af43178 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 24 Mar 2021 03:26:11 +0100 Subject: [PATCH 01/21] object: Pass the instance pointer to toggle notify There's no need to lookup the object pointer again during toggle notify callback as we can just use the current instance, as in case the underlying gobject is destroyed earlier we'd still remove the toggle notification on dispose notify. --- gi/object.cpp | 20 +++++++++----------- gi/object.h | 2 ++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index 39ce7f5fd..75b29be30 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1084,17 +1084,14 @@ static void wrapped_gobj_dispose_notify( where_the_object_was); } -static void wrapped_gobj_toggle_notify(void*, GObject* gobj, - gboolean is_last_ref); - void ObjectInstance::gobj_dispose_notify(void) { m_gobj_disposed = true; if (m_uses_toggle_ref) { - g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, nullptr); - wrapped_gobj_toggle_notify(nullptr, m_ptr, TRUE); + g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this); + wrapped_gobj_toggle_notify(this, m_ptr, TRUE); } } @@ -1222,10 +1219,11 @@ toggle_handler(GObject *gobj, } } -static void wrapped_gobj_toggle_notify(void*, GObject* gobj, - gboolean is_last_ref) { +void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj, + gboolean is_last_ref) { bool is_main_thread; bool toggle_up_queued, toggle_down_queued; + auto* self = static_cast(instance); GjsContextPrivate* gjs = GjsContextPrivate::from_current_context(); if (gjs->destroying()) { @@ -1281,7 +1279,7 @@ static void wrapped_gobj_toggle_notify(void*, GObject* gobj, toggle_up_queued? "up" : "down"); } - ObjectInstance::for_gobject(gobj)->toggle_down(); + self->toggle_down(); } else { toggle_queue.enqueue(gobj, ToggleQueue::DOWN, toggle_handler); } @@ -1296,7 +1294,7 @@ static void wrapped_gobj_toggle_notify(void*, GObject* gobj, g_error("toggling up object %s that's already queued to toggle up\n", G_OBJECT_TYPE_NAME(gobj)); } - ObjectInstance::for_gobject(gobj)->toggle_up(); + self->toggle_up(); } else { toggle_queue.enqueue(gobj, ToggleQueue::UP, toggle_handler); } @@ -1311,7 +1309,7 @@ ObjectInstance::release_native_object(void) m_ptr.release(); else if (m_uses_toggle_ref) g_object_remove_toggle_ref(m_ptr.release(), wrapped_gobj_toggle_notify, - nullptr); + this); else m_ptr = nullptr; } @@ -1459,7 +1457,7 @@ ObjectInstance::ensure_uses_toggle_ref(JSContext *cx) */ m_uses_toggle_ref = true; switch_to_rooted(cx); - g_object_add_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, nullptr); + g_object_add_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this); /* We now have both a ref and a toggle ref, we only want the toggle ref. * This may immediately remove the GC root we just added, since refcount diff --git a/gi/object.h b/gi/object.h index 3d7079713..bdc50de6c 100644 --- a/gi/object.h +++ b/gi/object.h @@ -477,6 +477,8 @@ class ObjectInstance : public GIWrapperInstance Date: Thu, 25 Mar 2021 20:03:41 +0100 Subject: [PATCH 02/21] object: Switch back to normal references on disposal When an object is disposed we already remove the toggle reference on it and we avoid that nothing will unref it again, however this can lead to troubles when something else owns the last reference at garbage collecting point if we didn't release the object earlier because in ~ObjectInstance we still expect to have a reference. To avoid this, let's just keep it simpler and switch back to normal reference again at dispose notify. The ordering here is important though, as referencing the object may cause a toggle-up event to happen, but we need to do it before removing the toggle reference (as that could be the only one we have), however after that we're still forcing a toggle-down event to happen, so we'll still be unrooted afterwards. Fixes: #399 --- gi/object.cpp | 6 +++--- installed-tests/js/testGtk3.js | 20 ++++++++++++++------ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index 75b29be30..f42fb0c8a 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1090,8 +1090,10 @@ ObjectInstance::gobj_dispose_notify(void) m_gobj_disposed = true; if (m_uses_toggle_ref) { + g_object_ref(m_ptr.get()); g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this); wrapped_gobj_toggle_notify(this, m_ptr, TRUE); + m_uses_toggle_ref = false; } } @@ -1305,9 +1307,7 @@ void ObjectInstance::release_native_object(void) { discard_wrapper(); - if (m_uses_toggle_ref && m_gobj_disposed) - m_ptr.release(); - else if (m_uses_toggle_ref) + if (m_uses_toggle_ref && !m_gobj_disposed) g_object_remove_toggle_ref(m_ptr.release(), wrapped_gobj_toggle_notify, this); else diff --git a/installed-tests/js/testGtk3.js b/installed-tests/js/testGtk3.js index fd0608d1d..8d08481f8 100644 --- a/installed-tests/js/testGtk3.js +++ b/installed-tests/js/testGtk3.js @@ -191,19 +191,27 @@ describe('Gtk overrides', function () { 'Gtk overrides avoid crashing and print a stack trace'); }); - it('GTK vfuncs can be explicitly called during disposition', function () { - let called; - const GoodLabel = GObject.registerClass(class GoodLabel extends Gtk.Label { + it('GTK vfuncs are not called if the object is disposed', function () { + const spy = jasmine.createSpy('vfunc_destroy'); + const NotSoGoodLabel = GObject.registerClass(class NotSoGoodLabel extends Gtk.Label { vfunc_destroy() { - called = true; + spy(); } }); - let label = new GoodLabel(); + let label = new NotSoGoodLabel(); + label.destroy(); - expect(called).toBeTruthy(); + expect(spy).toHaveBeenCalledTimes(1); + + GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL, + '*during garbage collection*'); + GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL, + '*destroy*'); label = null; System.gc(); + GLib.test_assert_expected_messages_internal('Gjs', 'testGtk3.js', 0, + 'GTK vfuncs are not called if the object is disposed'); }); it('accepts string in place of GdkAtom', function () { -- GitLab From b22497a768bdc71ecc7d4fb8971453a65df522d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 24 Mar 2021 07:17:20 +0100 Subject: [PATCH 03/21] object: Use the term "DISPOSED" for objects in such state, not FINALIZED As they're still objects, but in disposed state --- gi/object.cpp | 6 +++--- installed-tests/js/testGObjectDestructionAccess.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index f42fb0c8a..45ce94bdd 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -2235,11 +2235,11 @@ bool ObjectBase::to_string(JSContext* cx, unsigned argc, JS::Value* vp) { /* * ObjectInstance::to_string_kind: * - * ObjectInstance shows a "finalized" marker in its toString() method if the - * wrapped GObject has already been finalized. + * ObjectInstance shows a "disposed" marker in its toString() method if the + * wrapped GObject has already been disposed. */ const char* ObjectInstance::to_string_kind(void) const { - return m_gobj_disposed ? "object (FINALIZED)" : "object"; + return m_gobj_disposed ? "object (DISPOSED)" : "object"; } /* diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index 0b35d8596..2391aacbe 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -126,7 +126,7 @@ describe('Access to destroyed GObject', function () { 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]+\]/); + /\[object \(DISPOSED\) instance wrapper GIName:Gtk.Window jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); }); it('Proto function toString before/after', function () { @@ -138,6 +138,6 @@ describe('Access to destroyed GObject', function () { validWindow.destroy(); expect(validWindow.toString()).toMatch( - /\[object \(FINALIZED\) instance wrapper GIName:Gtk.Window jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + /\[object \(DISPOSED\) instance wrapper GIName:Gtk.Window jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); }); }); -- GitLab From 3303948fd850963831925b5e213d1ada17e07eab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 24 Mar 2021 07:40:47 +0100 Subject: [PATCH 04/21] object: Catch finalized objects that are still under gjs scope When an object is finalized its qdata is finally removed, so if while we track the object in the wrapper the qdata is removed (after a disposal) then we can be sure that the object is finalized. We can so track this state by using a GDestroyNotify function as it will be called with the wrapper pointer and so we can easily mark the object as finalized in such case. While the object is in the disposed state, waiting for finalization we have to temporary give the role of monitoring the qdata to the disposed quark so that during this phase the object has still the private qdata unset, but still monitored for finalization, until we stop caring about it. If instead the object is created as a wrapper for a disposed object, the private qdata still serves us to monitor its finalization without having to override the disposed one, that may still be used by another wrapper that is still living waiting for being garbage collected. If this happens we can finally write proper critical messages and avoid accessing to the object again. Added a test to cover this case --- gi/object.cpp | 64 +++++++++++++++++-- gi/object.h | 4 ++ .../js/testGObjectDestructionAccess.js | 32 +++++++++- 3 files changed, 92 insertions(+), 8 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index 45ce94bdd..74001f927 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -79,9 +79,12 @@ static_assert(sizeof(ObjectInstance) <= 88, bool ObjectInstance::s_weak_pointer_callback = false; ObjectInstance *ObjectInstance::wrapped_gobject_list = nullptr; +static const auto DISPOSED_OBJECT = std::numeric_limits::max(); + // 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::disposed, ObjectBase::disposed) // clang-format on [[nodiscard]] static GQuark gjs_object_priv_quark() { @@ -237,13 +240,27 @@ void ObjectPrototype::set_type_qdata(void) { void ObjectInstance::set_object_qdata(void) { - g_object_set_qdata(m_ptr, gjs_object_priv_quark(), this); + g_object_set_qdata_full( + m_ptr, gjs_object_priv_quark(), this, [](void* object) { + auto* self = static_cast(object); + if (G_UNLIKELY(!self->m_gobj_disposed)) { + g_warning( + "Object %p (a %s) was finalized but we didn't track " + "its disposal", + self->m_ptr.get(), g_type_name(self->gtype())); + self->m_gobj_disposed = true; + } + self->m_gobj_finalized = true; + gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, + "Wrapped GObject %p finalized", + self->m_ptr.get()); + }); } void ObjectInstance::unset_object_qdata(void) { - g_object_set_qdata(m_ptr, gjs_object_priv_quark(), nullptr); + g_object_steal_qdata(m_ptr, gjs_object_priv_quark()); } GParamSpec* ObjectPrototype::find_param_spec_from_id(JSContext* cx, @@ -1084,11 +1101,32 @@ static void wrapped_gobj_dispose_notify( where_the_object_was); } +void ObjectInstance::track_gobject_finalization() { + auto quark = ObjectBase::disposed_quark(); + g_object_steal_qdata(m_ptr, quark); + g_object_set_qdata_full(m_ptr, quark, this, [](void* data) { + auto* self = static_cast(data); + self->m_gobj_finalized = true; + gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "Wrapped GObject %p finalized", + self->m_ptr.get()); + }); +} + +void ObjectInstance::ignore_gobject_finalization() { + auto quark = ObjectBase::disposed_quark(); + if (g_object_get_qdata(m_ptr, quark) == this) { + g_object_steal_qdata(m_ptr, quark); + g_object_set_qdata(m_ptr, quark, gjs_int_to_pointer(DISPOSED_OBJECT)); + } +} + void ObjectInstance::gobj_dispose_notify(void) { m_gobj_disposed = true; + track_gobject_finalization(); + if (m_uses_toggle_ref) { g_object_ref(m_ptr.get()); g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this); @@ -1307,6 +1345,19 @@ void ObjectInstance::release_native_object(void) { discard_wrapper(); + + if (m_gobj_finalized) { + g_critical( + "Object %p of type %s has been finalized while it was still " + "owned by gjs, this is due to invalid memory management.", + m_ptr.get(), g_type_name(gtype())); + m_ptr.release(); + return; + } + + if (m_gobj_disposed) + ignore_gobject_finalization(); + if (m_uses_toggle_ref && !m_gobj_disposed) g_object_remove_toggle_ref(m_ptr.release(), wrapped_gobj_toggle_notify, this); @@ -1352,6 +1403,7 @@ ObjectInstance::ObjectInstance(JSContext* cx, JS::HandleObject object) : GIWrapperInstance(cx, object), m_wrapper_finalized(false), m_gobj_disposed(false), + m_gobj_finalized(false), m_uses_toggle_ref(false) { GTypeQuery query; type_query_dynamic_safe(&query); @@ -1667,11 +1719,11 @@ ObjectInstance::~ObjectInstance() { bool had_toggle_up; bool had_toggle_down; - if (G_UNLIKELY(m_ptr->ref_count <= 0)) { + if (m_gobj_finalized || G_UNLIKELY(m_ptr->ref_count <= 0)) { g_error( "Finalizing wrapper for an already freed object of type: " - "%s.%s\n", - ns(), name()); + "%s.%s: %p\n", + ns(), name(), m_ptr.get()); } auto& toggle_queue = ToggleQueue::get_default(); @@ -2239,6 +2291,8 @@ bool ObjectBase::to_string(JSContext* cx, unsigned argc, JS::Value* vp) { * wrapped GObject has already been disposed. */ const char* ObjectInstance::to_string_kind(void) const { + if (m_gobj_finalized) + return "object (FINALIZED)"; return m_gobj_disposed ? "object (DISPOSED)" : "object"; } diff --git a/gi/object.h b/gi/object.h index bdc50de6c..e6d5a365c 100644 --- a/gi/object.h +++ b/gi/object.h @@ -177,6 +177,7 @@ class ObjectBase [[nodiscard]] static GQuark custom_type_quark(); [[nodiscard]] static GQuark custom_property_quark(); + [[nodiscard]] static GQuark disposed_quark(); }; // See https://bugzilla.mozilla.org/show_bug.cgi?id=1614220 @@ -304,6 +305,7 @@ class ObjectInstance : public GIWrapperInstance Date: Wed, 24 Mar 2021 05:57:04 +0100 Subject: [PATCH 05/21] object: Discard disposed GObject's and do not create wrappers for them As per commit d37d6604 and commit c0003eb we may destroy an object wrapper earlier than we used to do, this has some memory benefits but we also need to make sure that we cleanup the object qdata so that it never points to the just free'd object wrapper. By doing this, however, we may end up re-associating a GObject that is disposed but not yet finalized to another valid wrapper. This was allowed before but wasn't really correct either because in such case it will be just useless and won't be able to monitor its disposal again. So, now during disposal notify save a qdata (that won't be removed until finalization) on gobject marking it as disposed, then use this data to prevent an object to be re-associated again to a valid wrapper. In fact we just create a new wrapper that is marked as "disposed", and so preventing most of actions to be performed. We don't warn though, because this may just happen at the moment the invalid object is incorrectly used. As per this, we need to unset the object qdata only if the object is not finalized, as we may still need to do it when it's finalized, or we'll still crash when a disposed GObject will point to a wrapper that has been already destroyed. Added tests simulating this. Fixes: #395 --- gi/arg.cpp | 11 +++---- gi/function.cpp | 5 +++ gi/object.cpp | 32 +++++++++++++++++-- gi/object.h | 4 +++ gi/value.cpp | 15 ++------- .../js/testGObjectDestructionAccess.js | 14 ++++++++ 6 files changed, 59 insertions(+), 22 deletions(-) diff --git a/gi/arg.cpp b/gi/arg.cpp index 49224e839..f5f83202f 100644 --- a/gi/arg.cpp +++ b/gi/arg.cpp @@ -2681,13 +2681,10 @@ gjs_value_from_g_argument (JSContext *context, } if (g_type_is_a(gtype, G_TYPE_OBJECT)) { - // Null arg is already handled above - JSObject* obj = ObjectInstance::wrapper_from_gobject( - context, G_OBJECT(gjs_arg_get(arg))); - if (!obj) - return false; - value_p.setObject(*obj); - return true; + g_assert(gjs_arg_get(arg) && + "Null arg is already handled above"); + return ObjectInstance::set_value_from_gobject( + context, gjs_arg_get(arg), value_p); } if (g_type_is_a(gtype, G_TYPE_BOXED) || diff --git a/gi/function.cpp b/gi/function.cpp index b0a27a7c5..3c3cacbcb 100644 --- a/gi/function.cpp +++ b/gi/function.cpp @@ -365,6 +365,11 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) { if (gobj) { this_object = ObjectInstance::wrapper_from_gobject(context, gobj); if (!this_object) { + if (g_object_get_qdata(gobj, ObjectBase::disposed_quark())) { + warn_about_illegal_js_callback( + "on disposed object", + "using the destroy(), dispose(), or remove() vfuncs"); + } gjs_log_exception(context); return; } diff --git a/gi/object.cpp b/gi/object.cpp index 74001f927..a7757770a 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -10,6 +10,7 @@ #include // for find #include // for mem_fn +#include #include #include // for tie #include // for move @@ -46,6 +47,7 @@ #include "gi/object.h" #include "gi/repo.h" #include "gi/toggle.h" +#include "gi/utils-inl.h" // for gjs_int_to_pointer #include "gi/value.h" #include "gi/wrapperutils.h" #include "gjs/atoms.h" @@ -260,7 +262,9 @@ ObjectInstance::set_object_qdata(void) void ObjectInstance::unset_object_qdata(void) { - g_object_steal_qdata(m_ptr, gjs_object_priv_quark()); + auto priv_quark = gjs_object_priv_quark(); + if (g_object_get_qdata(m_ptr, priv_quark) == this) + g_object_steal_qdata(m_ptr, priv_quark); } GParamSpec* ObjectPrototype::find_param_spec_from_id(JSContext* cx, @@ -1125,6 +1129,7 @@ ObjectInstance::gobj_dispose_notify(void) { m_gobj_disposed = true; + unset_object_qdata(); track_gobject_finalization(); if (m_uses_toggle_ref) { @@ -1479,11 +1484,13 @@ ObjectInstance::associate_js_gobject(JSContext *context, m_ptr = gobj; set_object_qdata(); m_wrapper = object; + m_gobj_disposed = !!g_object_get_qdata(gobj, ObjectBase::disposed_quark()); ensure_weak_pointer_callback(context); link(); - g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, this); + if (!G_UNLIKELY(m_gobj_disposed)) + g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, this); } void @@ -1550,9 +1557,10 @@ ObjectInstance::disassociate_js_gobject(void) m_ptr.get(), type_name()); } - if (!m_gobj_disposed) { + if (!m_gobj_disposed) g_object_weak_unref(m_ptr.get(), wrapped_gobj_dispose_notify, this); + if (!m_gobj_finalized) { /* Fist, remove the wrapper pointer from the wrapped GObject */ unset_object_qdata(); } @@ -2497,6 +2505,24 @@ JSObject* ObjectInstance::wrapper_from_gobject(JSContext* cx, GObject* gobj) { return priv->wrapper(); } +bool ObjectInstance::set_value_from_gobject(JSContext* cx, GObject* gobj, + JS::MutableHandleValue value_p) { + if (!gobj) { + value_p.setNull(); + return true; + } + + auto* wrapper = ObjectInstance::wrapper_from_gobject(cx, gobj); + if (wrapper) { + value_p.setObject(*wrapper); + return true; + } + + gjs_throw(cx, "Failed to find JS object for GObject %p of type %s", gobj, + g_type_name(G_TYPE_FROM_INSTANCE(gobj))); + return false; +} + // Replaces GIWrapperBase::to_c_ptr(). The GIWrapperBase version is deleted. bool ObjectBase::to_c_ptr(JSContext* cx, JS::HandleObject obj, GObject** ptr) { g_assert(ptr); diff --git a/gi/object.h b/gi/object.h index e6d5a365c..2e7b04fbc 100644 --- a/gi/object.h +++ b/gi/object.h @@ -362,6 +362,10 @@ class ObjectInstance : public GIWrapperInstance(g_value_get_object(gvalue)), + value_p); } else if (gtype == G_TYPE_STRV) { if (!gjs_array_from_strv (context, value_p, diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index 9a59ac0d7..289014604 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -142,6 +142,20 @@ describe('Access to destroyed GObject', function () { }); describe('Disposed or finalized GObject', function () { + it('is marked as disposed when it is a manually disposed property', function () { + const emblem = new Gio.EmblemedIcon({ + gicon: new Gio.ThemedIcon({ name: 'alarm' }), + }); + + let { gicon } = emblem; + gicon.run_dispose(); + gicon = null; + System.gc(); + + expect(emblem.gicon.toString()).toMatch( + /\[object \(DISPOSED\) instance wrapper .* jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + }); + it('generates a warn on object garbage collection', function () { Gio.File.new_for_path('/').unref(); -- GitLab From c746a6685f9b11f624c31bbac6e7bb422895e16e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 24 Mar 2021 07:31:39 +0100 Subject: [PATCH 06/21] object: Also unset the object qdata during wrapper destruction This codepath is hardly hit, but at this point we're sure that the private-gjs we set as qdata is not valid anymore so it's safe to delete it. We've to do it only if the object has not been finalized though, not to crash and not to break the assumptions for "disposed" gobject wrappers. --- gi/object.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gi/object.cpp b/gi/object.cpp index a7757770a..cdb859162 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1746,6 +1746,10 @@ ObjectInstance::~ObjectInstance() { if (!m_gobj_disposed) g_object_weak_unref(m_ptr, wrapped_gobj_dispose_notify, this); + + if (!m_gobj_finalized) + unset_object_qdata(); + release_native_object(); } -- GitLab From 805d07e79265022c40b586dc7b12ec29c5b6d068 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 24 Mar 2021 20:42:19 +0100 Subject: [PATCH 07/21] testGtk3: Ensure that disposed signal callback gets called with valid object As per previous commits we handle the disposal differently, so ensure that all works as expected. --- installed-tests/js/testGtk3.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/installed-tests/js/testGtk3.js b/installed-tests/js/testGtk3.js index 8d08481f8..a6ba26426 100644 --- a/installed-tests/js/testGtk3.js +++ b/installed-tests/js/testGtk3.js @@ -214,6 +214,23 @@ describe('Gtk overrides', function () { 'GTK vfuncs are not called if the object is disposed'); }); + it('destroy signal is emitted while disposing objects', function () { + const label = new Gtk.Label({label: 'Hello'}); + const handleDispose = jasmine.createSpy('handleDispose').and.callFake(() => { + expect(label.label).toBe('Hello'); + }); + label.connect('destroy', handleDispose); + label.destroy(); + + expect(handleDispose).toHaveBeenCalledWith(label); + + GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL, + 'Object Gtk.Label (0x* deallocated *'); + expect(label.label).toBeUndefined(); + GLib.test_assert_expected_messages_internal('Gjs', 'testGtk3.js', 0, + 'GTK destroy signal is emitted while disposing objects'); + }); + it('accepts string in place of GdkAtom', function () { expect(() => Gtk.Clipboard.get(1)).toThrow(); expect(() => Gtk.Clipboard.get(true)).toThrow(); -- GitLab From a90459e4ae873e257b55adf4e4cb43f875bf0d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 24 Mar 2021 22:40:09 +0100 Subject: [PATCH 08/21] testGobjectDestructionAccess: Verify usage of vfunc_dispose Like any other virtual function it can be "safely" invoked during normal usage without warnings, but will warn on finalization caused by GC. --- .../js/testGObjectDestructionAccess.js | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index 289014604..3b711e813 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -144,10 +144,10 @@ describe('Access to destroyed GObject', function () { describe('Disposed or finalized GObject', function () { it('is marked as disposed when it is a manually disposed property', function () { const emblem = new Gio.EmblemedIcon({ - gicon: new Gio.ThemedIcon({ name: 'alarm' }), + gicon: new Gio.ThemedIcon({name: 'alarm'}), }); - let { gicon } = emblem; + let {gicon} = emblem; gicon.run_dispose(); gicon = null; System.gc(); @@ -156,6 +156,34 @@ describe('Disposed or finalized GObject', function () { /\[object \(DISPOSED\) instance wrapper .* jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); }); + it('calls dispose vfunc on explicit disposal only', function () { + const callSpy = jasmine.createSpy('vfunc_dispose'); + const DisposeFile = GObject.registerClass(class DisposeFile extends Gio.ThemedIcon { + vfunc_dispose(...args) { + expect(this.names).toEqual(['dummy']); + callSpy(...args); + } + }); + + let file = new DisposeFile({name: 'dummy'}); + file.run_dispose(); + expect(callSpy).toHaveBeenCalledOnceWith(); + + file.run_dispose(); + expect(callSpy).toHaveBeenCalledTimes(2); + file = null; + + GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL, + '*during garbage collection*'); + GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL, + '*dispose*'); + System.gc(); + GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0, + 'calls dispose vfunc on explicit disposal only'); + + expect(callSpy).toHaveBeenCalledTimes(2); + }); + it('generates a warn on object garbage collection', function () { Gio.File.new_for_path('/').unref(); -- GitLab From 02899a7bd092af071ec4deaa2fdc3aaaaa3179f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 24 Mar 2021 07:15:47 +0100 Subject: [PATCH 09/21] 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). --- gi/object.cpp | 30 +++++++++++++------ gi/object.h | 2 +- .../js/testGObjectDestructionAccess.js | 22 ++++++++++++++ 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index cdb859162..7c8cc732a 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* 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 2e7b04fbc..fd04caf63 100644 --- a/gi/object.h +++ b/gi/object.h @@ -382,7 +382,7 @@ class ObjectInstance : public GIWrapperInstance Date: Thu, 25 Mar 2021 21:29:10 +0100 Subject: [PATCH 10/21] installed-tests: Add GjsTestTools utilities to call native code for evil ops At times we may need to simulate bad behaviors of other libraries, so to check this we need to do evil hacks with the objects like ref or unref them without having JS envolved. To allow this, add an utility class that we introspect and can use for testing. --- .../js/libgjstesttools/gjs-test-tools.cpp | 39 +++++++++++++++++++ .../js/libgjstesttools/gjs-test-tools.h | 23 +++++++++++ .../js/libgjstesttools/meson.build | 19 +++++++++ installed-tests/js/meson.build | 3 ++ meson.build | 5 ++- 5 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 installed-tests/js/libgjstesttools/gjs-test-tools.cpp create mode 100644 installed-tests/js/libgjstesttools/gjs-test-tools.h create mode 100644 installed-tests/js/libgjstesttools/meson.build diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp new file mode 100644 index 000000000..4474e6e00 --- /dev/null +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp @@ -0,0 +1,39 @@ +/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */ +// SPDX-License-Identifier: MIT OR LGPL-2.0-or-later +// SPDX-FileCopyrightText: 2021 Marco Trevisan + +#include "installed-tests/js/libgjstesttools/gjs-test-tools.h" + +void gjs_test_tools_init() {} + +void gjs_test_tools_reset() {} + +void gjs_test_tools_delayed_ref(GObject* object, int interval) { + g_timeout_add( + interval, + [](void *data) { + g_object_ref(G_OBJECT(data)); + return G_SOURCE_REMOVE; + }, + object); +} + +void gjs_test_tools_delayed_unref(GObject* object, int interval) { + g_timeout_add( + interval, + [](void *data) { + g_object_unref(G_OBJECT(data)); + return G_SOURCE_REMOVE; + }, + object); +} + +void gjs_test_tools_delayed_dispose(GObject* object, int interval) { + g_timeout_add( + interval, + [](void *data) { + g_object_run_dispose(G_OBJECT(data)); + return G_SOURCE_REMOVE; + }, + object); +} diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.h b/installed-tests/js/libgjstesttools/gjs-test-tools.h new file mode 100644 index 000000000..478a2bc32 --- /dev/null +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.h @@ -0,0 +1,23 @@ +/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */ +// SPDX-License-Identifier: MIT OR LGPL-2.0-or-later +// SPDX-FileCopyrightText: 2021 Marco Trevisan + +#pragma once + +#include +#include +#include + +G_BEGIN_DECLS + +void gjs_test_tools_init(void); + +void gjs_test_tools_reset(void); + +void gjs_test_tools_delayed_ref(GObject* object, int interval); + +void gjs_test_tools_delayed_unref(GObject* object, int interval); + +void gjs_test_tools_delayed_dispose(GObject* object, int interval); + +G_END_DECLS diff --git a/installed-tests/js/libgjstesttools/meson.build b/installed-tests/js/libgjstesttools/meson.build new file mode 100644 index 000000000..2371f6fd7 --- /dev/null +++ b/installed-tests/js/libgjstesttools/meson.build @@ -0,0 +1,19 @@ +# SPDX-License-Identifier: MIT OR LGPL-2.0-or-later +# SPDX-FileCopyrightText: 2021 Marco Trevisan + +gjstest_tools_sources = [ + 'gjs-test-tools.cpp', + 'gjs-test-tools.h', +] +libgjstesttools = library('gjstesttools', + gjstest_tools_sources, dependencies: [glib, gobject, gio], + include_directories: top_include, dependencies: libgjs_dep, + cpp_args: libgjs_cpp_args + test_gir_extra_c_args + test_gir_warning_c_args, + install: get_option('installed_tests'), install_dir: installed_tests_execdir) +gjstest_tools_gir = gnome.generate_gir(libgjstesttools, + includes: ['GObject-2.0', 'Gio-2.0'], sources: gjstest_tools_sources, + namespace: 'GjsTestTools', nsversion: '1.0', + symbol_prefix: 'gjs_test_tools_', extra_args: '--warn-error', + install: get_option('installed_tests'), install_dir_gir: false, + install_dir_typelib: installed_tests_execdir) +gjstest_tools_typelib = gjstest_tools_gir[1] diff --git a/installed-tests/js/meson.build b/installed-tests/js/meson.build index 8026f903f..97f9cd07d 100644 --- a/installed-tests/js/meson.build +++ b/installed-tests/js/meson.build @@ -89,6 +89,8 @@ gimarshallingtests_gir = gnome.generate_gir(libgimarshallingtests, install_dir_typelib: installed_tests_execdir) gimarshallingtests_typelib = gimarshallingtests_gir[1] +subdir('libgjstesttools') + jasmine_tests = [ 'self', 'ByteArray', @@ -149,6 +151,7 @@ foreach test : jasmine_tests test(test, minijasmine, args: test_file, depends: [ gschemas_compiled, + gjstest_tools_typelib, gimarshallingtests_typelib, regress_typelib, warnlib_typelib, diff --git a/meson.build b/meson.build index 33a3389d6..272f20334 100644 --- a/meson.build +++ b/meson.build @@ -577,15 +577,16 @@ pkg.generate(libgjs, name: api_name, description: 'JS bindings for GObjects', tests_environment = environment() js_tests_builddir = meson.current_build_dir() / 'installed-tests' / 'js' +libgjs_test_tools_builddir = js_tests_builddir / 'libgjstesttools' # GJS_PATH is empty here since we want to force the use of our own # resources. G_FILENAME_ENCODING ensures filenames are not UTF-8 tests_environment.set('TOP_BUILDDIR', meson.build_root()) tests_environment.set('GJS_USE_UNINSTALLED_FILES', '1') tests_environment.set('GJS_PATH', '') tests_environment.prepend('GI_TYPELIB_PATH', meson.current_build_dir(), - js_tests_builddir) + js_tests_builddir, libgjs_test_tools_builddir) tests_environment.prepend('LD_LIBRARY_PATH', meson.current_build_dir(), - js_tests_builddir) + js_tests_builddir, libgjs_test_tools_builddir) tests_environment.set('G_FILENAME_ENCODING', 'latin1') # Workaround for https://github.com/google/sanitizers/issues/1322 tests_environment.set('ASAN_OPTIONS', 'intercept_tls_get_addr=0') -- GitLab From a1bbcdae30df3d3c30aa3833366ded062c01d483 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 25 Mar 2021 21:42:24 +0100 Subject: [PATCH 11/21] testGObjectDestructionAccess: Verify that disposed object is monitored Thanks to the evil hacks we have now we can perform a delayed unref without having to rely on pure JS code. So we can test the case in which a JS-disposed (only) object is later finalized by C code. --- .../js/testGObjectDestructionAccess.js | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index 423da44e2..c5d86ccb9 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -4,7 +4,7 @@ imports.gi.versions.Gtk = '3.0'; -const {GLib, Gio, GObject, Gtk} = imports.gi; +const {GLib, Gio, GjsTestTools, GObject, Gtk} = imports.gi; const {system: System} = imports; describe('Access to destroyed GObject', function () { @@ -164,6 +164,14 @@ describe('Access to destroyed GObject', function () { }); describe('Disposed or finalized GObject', function () { + beforeAll(function () { + GjsTestTools.init(); + }); + + afterEach(function () { + GjsTestTools.reset(); + }); + it('is marked as disposed when it is a manually disposed property', function () { const emblem = new Gio.EmblemedIcon({ gicon: new Gio.ThemedIcon({name: 'alarm'}), @@ -223,11 +231,30 @@ describe('Disposed or finalized GObject', function () { expect(file.toString()).toMatch( /\[object \(FINALIZED\) instance wrapper GType:GLocalFile jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); file = 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 if has expando property'); + }); + + it('generates a warn if already disposed at garbage collection', function () { + const loop = new GLib.MainLoop(null, false); + + let file = Gio.File.new_for_path('/'); + GjsTestTools.delayed_unref(file, 1); // Will happen after dispose + file.run_dispose(); + + let done = false; + GLib.timeout_add(GLib.PRIORITY_DEFAULT, 50, () => (done = true)); + while (!done) + loop.get_context().iteration(true); + file = 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'); + 'generates a warn if already disposed at garbage collection'); }); }); -- GitLab From 593cf7959d8de2872a1ddb48c9cf5502ac4796fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 26 Mar 2021 02:45:01 +0100 Subject: [PATCH 12/21] object: Discard wrapper when GObject is disposed from main thread When disposing the object we can safely mark the object as discarded as it's not going to be useful anyway. As per this we can avoid triggering an error when disposing because we are now able to track this case while disassociating the object already. --- gi/object.cpp | 10 +- .../js/libgjstesttools/gjs-test-tools.cpp | 157 +++++++++++++++++- .../js/libgjstesttools/gjs-test-tools.h | 19 +++ .../js/testGObjectDestructionAccess.js | 45 +++++ 4 files changed, 223 insertions(+), 8 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index 7c8cc732a..de63354c5 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1139,6 +1139,9 @@ ObjectInstance::gobj_dispose_notify(void) wrapped_gobj_toggle_notify(this, m_ptr, TRUE); m_uses_toggle_ref = false; } + + if (GjsContextPrivate::from_current_context()->is_owner_thread()) + discard_wrapper(); } void ObjectInstance::iterate_wrapped_gobjects( @@ -1735,13 +1738,6 @@ ObjectInstance::~ObjectInstance() { bool had_toggle_up; bool had_toggle_down; - if (m_gobj_finalized || G_UNLIKELY(m_ptr->ref_count <= 0)) { - g_error( - "Finalizing wrapper for an already freed object of type: " - "%s.%s: %p\n", - ns(), name(), m_ptr.get()); - } - auto& toggle_queue = ToggleQueue::get_default(); std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(m_ptr); diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp index 4474e6e00..1e588c975 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp @@ -4,9 +4,43 @@ #include "installed-tests/js/libgjstesttools/gjs-test-tools.h" +#include +#include + +#include "gjs/jsapi-util.h" + +static GObject* m_tmp_object = NULL; +static std::unordered_set m_finalized_objects; +static std::mutex m_finalized_objects_lock; + +struct FinalizedObjectsLocked { + FinalizedObjectsLocked() : hold(m_finalized_objects_lock) {} + + std::unordered_set* operator->() { return &m_finalized_objects; } + std::lock_guard hold; +}; + void gjs_test_tools_init() {} -void gjs_test_tools_reset() {} +void gjs_test_tools_reset() { + if (!FinalizedObjectsLocked()->count(m_tmp_object)) + g_clear_object(&m_tmp_object); + else + m_tmp_object = nullptr; + + FinalizedObjectsLocked()->clear(); +} + +// clang-format off +static G_DEFINE_QUARK(gjs-test-utils::finalize, finalize); +// clang-format on + +static void monitor_object_finalization(GObject* object) { + g_object_steal_qdata(object, finalize_quark()); + g_object_set_qdata_full(object, finalize_quark(), object, [](void* data) { + FinalizedObjectsLocked()->insert(static_cast(data)); + }); +} void gjs_test_tools_delayed_ref(GObject* object, int interval) { g_timeout_add( @@ -37,3 +71,124 @@ void gjs_test_tools_delayed_dispose(GObject* object, int interval) { }, object); } + +void gjs_test_tools_save_object(GObject* object) { + g_assert(!m_tmp_object); + g_set_object(&m_tmp_object, object); + monitor_object_finalization(object); +} + +void gjs_test_tools_ref_other_thread(GObject* object) { + // cppcheck-suppress leakNoVarFunctionCall + g_thread_join(g_thread_new("ref_object", g_object_ref, object)); +} + +typedef enum { + REF = 1 << 0, + UNREF = 1 << 1, +} RefType; + +typedef struct { + GObject* object; + RefType ref_type; + int delay; +} RefThreadData; + +static RefThreadData* ref_thread_data_new(GObject* object, int interval, + RefType ref_type) { + auto* ref_data = g_new(RefThreadData, 1); + + ref_data->object = object; + ref_data->delay = interval; + ref_data->ref_type = ref_type; + + monitor_object_finalization(object); + + return ref_data; +} + +static void* ref_thread_func(void* data) { + GjsAutoPointer ref_data = + static_cast(data); + + if (FinalizedObjectsLocked()->count(ref_data->object)) + return nullptr; + + if (ref_data->delay > 0) + g_usleep(ref_data->delay); + + if (FinalizedObjectsLocked()->count(ref_data->object)) + return nullptr; + + if (ref_data->ref_type & REF) + g_object_ref(ref_data->object); + + if (!(ref_data->ref_type & UNREF)) { + return ref_data->object; + } else if (ref_data->ref_type & REF) { + g_usleep(ref_data->delay); + + if (FinalizedObjectsLocked()->count(ref_data->object)) + return nullptr; + } + + if (ref_data->object != m_tmp_object) + g_object_steal_qdata(ref_data->object, finalize_quark()); + g_object_unref(ref_data->object); + return nullptr; +} + +void gjs_test_tools_unref_other_thread(GObject* object) { + // cppcheck-suppress leakNoVarFunctionCall + g_thread_join(g_thread_new("unref_object", ref_thread_func, + ref_thread_data_new(object, -1, UNREF))); +} + +void gjs_test_tools_delayed_ref_other_thread(GObject* object, int interval) { + g_thread_unref(g_thread_new("ref_object", ref_thread_func, + ref_thread_data_new(object, interval, REF))); +} + +void gjs_test_tools_delayed_unref_other_thread(GObject* object, int interval) { + g_thread_unref(g_thread_new("unref_object", ref_thread_func, + ref_thread_data_new(object, interval, UNREF))); +} + +void gjs_test_tools_delayed_ref_unref_other_thread(GObject* object, + int interval) { + g_thread_unref( + g_thread_new("ref_unref_object", ref_thread_func, + ref_thread_data_new(object, interval, + static_cast(REF | UNREF)))); +} + +void gjs_test_tools_run_dispose_other_thread(GObject* object) { + // cppcheck-suppress leakNoVarFunctionCall + g_thread_join(g_thread_new( + "run_dispose", + [](void* object) -> void* { + g_object_run_dispose(G_OBJECT(object)); + return nullptr; + }, + object)); +} + +/** + * gjs_test_tools_get_saved: + * Returns: (transfer full) + */ +GObject* gjs_test_tools_get_saved() { + if (FinalizedObjectsLocked()->count(m_tmp_object)) + m_tmp_object = nullptr; + + return static_cast(g_steal_pointer(&m_tmp_object)); +} + +/** + * gjs_test_tools_get_disposed: + * Returns: (transfer none) + */ +GObject* gjs_test_tools_get_disposed(GObject* object) { + g_object_run_dispose(G_OBJECT(object)); + return object; +} diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.h b/installed-tests/js/libgjstesttools/gjs-test-tools.h index 478a2bc32..d56fa0f34 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.h +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.h @@ -20,4 +20,23 @@ void gjs_test_tools_delayed_unref(GObject* object, int interval); void gjs_test_tools_delayed_dispose(GObject* object, int interval); +void gjs_test_tools_ref_other_thread(GObject* object); + +void gjs_test_tools_delayed_ref_other_thread(GObject* object, int interval); + +void gjs_test_tools_unref_other_thread(GObject* object); + +void gjs_test_tools_delayed_unref_other_thread(GObject* object, int interval); + +void gjs_test_tools_delayed_ref_unref_other_thread(GObject* object, + int interval); + +void gjs_test_tools_run_dispose_other_thread(GObject* object); + +void gjs_test_tools_save_object(GObject* object); + +GObject* gjs_test_tools_get_saved(); + +GObject* gjs_test_tools_get_disposed(GObject* object); + G_END_DECLS diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index c5d86ccb9..05e79a32b 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -257,4 +257,49 @@ describe('Disposed or finalized GObject', function () { GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0, 'generates a warn if already disposed at garbage collection'); }); + + it('created from other function is marked as disposed', function () { + let file = Gio.File.new_for_path('/'); + GjsTestTools.save_object(file); + file.run_dispose(); + file = null; + System.gc(); + + expect(GjsTestTools.get_saved()).toMatch( + /\[object \(DISPOSED\) instance wrapper GType:GLocalFile jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + }); + + it('returned from function is marked as disposed', function () { + expect(GjsTestTools.get_disposed(Gio.File.new_for_path('/'))).toMatch( + /\[object \(DISPOSED\) instance wrapper GType:GLocalFile jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + }); + + it('returned from function is marked as disposed and then as finalized', function () { + let file = Gio.File.new_for_path('/'); + GjsTestTools.save_object(file); + GjsTestTools.delayed_unref(file, 30); + file.run_dispose(); + + let disposedFile = GjsTestTools.get_saved(); + expect(disposedFile).toEqual(file); + expect(disposedFile).toMatch( + /\[object \(DISPOSED\) instance wrapper GType:GLocalFile jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + + file = null; + System.gc(); + + const loop = new GLib.MainLoop(null, false); + GLib.timeout_add(GLib.PRIORITY_DEFAULT, 50, () => loop.quit()); + loop.run(); + + expect(disposedFile).toMatch( + /\[object \(FINALIZED\) instance wrapper GType:GLocalFile jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + + GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL, + '*Object 0x* has been finalized *'); + disposedFile = null; + System.gc(); + GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0, + 'returned from function is marked as disposed and then as finalized'); + }); }); -- GitLab From eb28da76bd3995250d0b3ee729338d0c184871ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 29 Mar 2021 04:05:47 +0200 Subject: [PATCH 13/21] object: Cancel queued toggles on dispose notify If when disposing an object we've toggle requests coming from another thread (as it happens with GSettings) we need to cancel the queue as the object won't be usable anymore and will be marked for being garbage collected. This was currently causing us to hit a g_error, that isn't actually problematic in this case because the object isn't going to be usable anyways. Add more test tools to simulate such case, and test it. --- gi/object.cpp | 1 + .../js/testGObjectDestructionAccess.js | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/gi/object.cpp b/gi/object.cpp index de63354c5..e18620ec3 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1136,6 +1136,7 @@ ObjectInstance::gobj_dispose_notify(void) if (m_uses_toggle_ref) { g_object_ref(m_ptr.get()); g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this); + ToggleQueue::get_default().cancel(m_ptr); wrapped_gobj_toggle_notify(this, m_ptr, TRUE); m_uses_toggle_ref = false; } diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index 05e79a32b..6d592934b 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -302,4 +302,20 @@ describe('Disposed or finalized GObject', function () { GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0, 'returned from function is marked as disposed and then as finalized'); }); + + it('ignores toggling queued unref toggles', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + file.ref(); + GjsTestTools.unref_other_thread(file); + file.run_dispose(); + }); + + it('ignores toggling queued toggles', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + GjsTestTools.ref_other_thread(file); + GjsTestTools.unref_other_thread(file); + file.run_dispose(); + }); }); -- GitLab From c4fc8213b3c0b64b00dcfb26cf140d5924531230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 29 Mar 2021 04:16:26 +0200 Subject: [PATCH 14/21] object: Safely handle disposal when happening from other thread In case we dispose an object from another thread we still queue a toggle down event to happen in an idle, so that it can be picked by main thread at next iteration. However, in such case we'll have no more the instance set on the object private data. We still keep track of it through the disposed data though, so we can use it to trigger a toggle down event. It's an error if we get an invalid ("true") pointer here, so we log about it. --- gi/object.cpp | 19 +++++++++++++++++-- .../js/testGObjectDestructionAccess.js | 16 ++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index e18620ec3..072c46c24 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1257,12 +1257,27 @@ static void toggle_handler(GObject *gobj, ToggleQueue::Direction direction) { + auto* self = ObjectInstance::for_gobject(gobj); + + if (G_UNLIKELY(!self)) { + void* disposed = g_object_get_qdata(gobj, ObjectBase::disposed_quark()); + + if (G_UNLIKELY(disposed == gjs_int_to_pointer(DISPOSED_OBJECT))) { + g_critical("Handling toggle %s for an unknown object %p", + direction == ToggleQueue::UP ? "up" : "down", gobj); + return; + } + + // In this case the object has been disposed but its wrapper not yet + self = static_cast(disposed); + } + switch (direction) { case ToggleQueue::UP: - ObjectInstance::for_gobject(gobj)->toggle_up(); + self->toggle_up(); break; case ToggleQueue::DOWN: - ObjectInstance::for_gobject(gobj)->toggle_down(); + self->toggle_down(); break; default: g_assert_not_reached(); diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index 6d592934b..4b6e9112a 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -318,4 +318,20 @@ describe('Disposed or finalized GObject', function () { GjsTestTools.unref_other_thread(file); file.run_dispose(); }); + + it('can be disposed from other thread', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + file.ref(); + GjsTestTools.unref_other_thread(file); + GjsTestTools.run_dispose_other_thread(file); + }); + + it('can be garbage collected once disposed from other thread', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + GjsTestTools.run_dispose_other_thread(file); + file = null; + System.gc(); + }); }); -- GitLab From 5adb51caa691943d0a516225b2cb5c14f71728ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 29 Mar 2021 06:24:24 +0200 Subject: [PATCH 15/21] object: Do not disassociate a JS object if we have queued toggle up In case an object has toggle up events queued we need them to happen in order to make the object to be rooted again, and there's no need to disassociate it the object. If instead the object has queued toggle downs, we can instead just cancel them as the object is unrooted anyways, so there's no issue in proceeding. Add test to check whether this is happening, we can simulate such scenario by queuing a further ref on an object from another thread and wait for GC to pick the object. Fixes: #294 --- gi/object.cpp | 44 +++++-- gi/toggle.cpp | 76 ++++++++---- gi/toggle.h | 8 +- .../js/libgjstesttools/gjs-test-tools.cpp | 39 +++++++ .../js/libgjstesttools/gjs-test-tools.h | 10 ++ .../js/testGObjectDestructionAccess.js | 109 ++++++++++++++++++ 6 files changed, 248 insertions(+), 38 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index 072c46c24..a8ac6d865 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1329,6 +1329,9 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj, is_main_thread = gjs->is_owner_thread(); auto& toggle_queue = ToggleQueue::get_default(); + if (is_main_thread && toggle_queue.is_being_handled(gobj)) + return; + std::tie(toggle_down_queued, toggle_up_queued) = toggle_queue.is_queued(gobj); if (is_last_ref) { @@ -1336,16 +1339,16 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj, * The JSObject is rooted and we need to unroot it so it * can be garbage collected */ - if (is_main_thread) { - if (G_UNLIKELY (toggle_up_queued || toggle_down_queued)) { - g_error("toggling down object %s that's already queued to toggle %s\n", - G_OBJECT_TYPE_NAME(gobj), - toggle_up_queued && toggle_down_queued? "up and down" : - toggle_up_queued? "up" : "down"); + if (is_main_thread && !toggle_up_queued) { + if (G_UNLIKELY(toggle_down_queued)) { + g_error( + "toggling down object %p (%s) that's already queued to " + "toggle down", + gobj, G_OBJECT_TYPE_NAME(gobj)); } self->toggle_down(); - } else { + } else if (!toggle_down_queued) { toggle_queue.enqueue(gobj, ToggleQueue::DOWN, toggle_handler); } } else { @@ -1356,11 +1359,13 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj, */ if (is_main_thread && !toggle_down_queued) { if (G_UNLIKELY (toggle_up_queued)) { - g_error("toggling up object %s that's already queued to toggle up\n", - G_OBJECT_TYPE_NAME(gobj)); + g_error( + "toggling up object %p (%s) that's already queued to " + "toggle up", + gobj, G_OBJECT_TYPE_NAME(gobj)); } self->toggle_up(); - } else { + } else if (!toggle_up_queued) { toggle_queue.enqueue(gobj, ToggleQueue::UP, toggle_handler); } } @@ -1467,7 +1472,22 @@ void ObjectInstance::update_heap_wrapper_weak_pointers(JSContext*, bool ObjectInstance::weak_pointer_was_finalized(void) { - if (has_wrapper() && !wrapper_is_rooted() && update_after_gc()) { + if (has_wrapper() && !wrapper_is_rooted()) { + bool toggle_down_queued, toggle_up_queued; + + auto& toggle_queue = ToggleQueue::get_default(); + std::tie(toggle_down_queued, toggle_up_queued) = + toggle_queue.is_queued(m_ptr); + + if (!toggle_down_queued && toggle_up_queued) + return false; + + if (!update_after_gc()) + return false; + + if (toggle_down_queued) + toggle_queue.cancel(m_ptr); + /* Ouch, the JS object is dead already. Disassociate the * GObject and hope the GObject dies too. (Remove it from * the weak pointer list first, since the disassociation @@ -1576,7 +1596,7 @@ ObjectInstance::disassociate_js_gobject(void) auto& toggle_queue = ToggleQueue::get_default(); std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(m_ptr.get()); - if (had_toggle_down != had_toggle_up) { + if (had_toggle_up && !had_toggle_down) { g_error( "JS object wrapper for GObject %p (%s) is being released while " "toggle references are still pending.", diff --git a/gi/toggle.cpp b/gi/toggle.cpp index 4f7db32ff..9714ebdb1 100644 --- a/gi/toggle.cpp +++ b/gi/toggle.cpp @@ -13,6 +13,7 @@ #include #include "gi/toggle.h" +#include "gjs/jsapi-util.h" std::deque::iterator ToggleQueue::find_operation_locked(const GObject *gobj, @@ -85,9 +86,13 @@ std::pair ToggleQueue::cancel(GObject* gobj) { return {had_toggle_down, had_toggle_up}; } -bool -ToggleQueue::handle_toggle(Handler handler) -{ +bool ToggleQueue::is_being_handled(GObject* gobj) { + GObject* tmp_gobj = gobj; + + return m_toggling_gobj.compare_exchange_strong(tmp_gobj, gobj); +} + +bool ToggleQueue::handle_toggle(Handler handler) { Item item; { std::lock_guard hold(lock); @@ -95,13 +100,37 @@ ToggleQueue::handle_toggle(Handler handler) return false; item = q.front(); - handler(item.gobj, item.direction); q.pop_front(); } - debug("handle", item.gobj); - if (item.needs_unref) - g_object_unref(item.gobj); + /* When getting the object from the weak reference we're implicitly + * adding a new reference to the object, this may cause the toggle + * notification to be triggered again and this may lead to enqueuing + * the object again, so let's save the toggling object in an atomic + * pointer so that we can check it quickly to ensure that we're not + * recursing into ourself. + */ + GObject* null_gobj = nullptr; + m_toggling_gobj.compare_exchange_strong(null_gobj, item.gobj); + + if (item.direction == Direction::DOWN) { + GjsSmartPointer gobj( + static_cast(g_weak_ref_get(&item.weak_ref))); + + if (gobj) { + debug("handle", gobj); + handler(gobj, item.direction); + } else { + debug("not handling finalized", item.gobj); + } + + g_weak_ref_clear(&item.weak_ref); + } else { + debug("handle", item.gobj); + handler(item.gobj, item.direction); + } + + m_toggling_gobj = nullptr; return true; } @@ -128,30 +157,27 @@ ToggleQueue::enqueue(GObject *gobj, return; } - Item item{gobj, direction}; - /* If we're toggling up we take a reference to the object now, - * so it won't toggle down before we process it. This ensures we - * only ever have at most two toggle notifications queued. - * (either only up, or down-up) + if (is_being_handled(gobj)) + return; + + std::lock_guard hold(lock); + /* Only keep a weak reference on the object here, as if we're here, the + * JSObject wrapper has already a reference and we don't want to cause + * any weak notify in case it has lost one already in the main thread. + * So let's use the weak reference to keep track of the object till we + * don't handle this toggle. + * Unfortunately due to GNOME/glib#2384 we can't be symmetric here and + * behave differently on toggle up and down events, however when toggling + * up we already have a strong reference, so there's no much need to do */ + auto& item = q.emplace_back(gobj, direction); + if (direction == UP) { debug("enqueue UP", gobj); - g_object_ref(gobj); - item.needs_unref = true; } else { + g_weak_ref_init(&item.weak_ref, gobj); debug("enqueue DOWN", gobj); } - /* If we're toggling down, we don't need to take a reference since - * the associated JSObject already has one, and that JSObject won't - * get finalized until we've completed toggling (since it's rooted, - * until we unroot it when we dispatch the toggle down idle). - * - * Taking a reference now would be bad anyway, since it would force - * the object to toggle back up again. - */ - - std::lock_guard hold(lock); - q.push_back(item); if (m_idle_id) { g_assert(((void) "Should always enqueue with the same handler", diff --git a/gi/toggle.h b/gi/toggle.h index e77c5419a..38ad88f66 100644 --- a/gi/toggle.h +++ b/gi/toggle.h @@ -31,9 +31,11 @@ public: private: struct Item { + Item() {} + Item(GObject* o, ToggleQueue::Direction d) : gobj(o), direction(d) {} GObject *gobj; + GWeakRef weak_ref; ToggleQueue::Direction direction; - unsigned needs_unref : 1; }; mutable std::mutex lock; @@ -42,6 +44,7 @@ private: unsigned m_idle_id; Handler m_toggle_handler; + std::atomic m_toggling_gobj = nullptr; /* No-op unless GJS_VERBOSE_ENABLE_LIFECYCLE is defined to 1. */ inline void debug(const char* did GJS_USED_VERBOSE_LIFECYCLE, @@ -73,6 +76,9 @@ private: * is empty. */ bool handle_toggle(Handler handler); + /* Checks if the gobj is currently being handled, to avoid recursion */ + bool is_being_handled(GObject* gobj); + /* After calling this, the toggle queue won't accept any more toggles. Only * intended for use when destroying the JSContext and breaking the * associations between C and JS objects. */ diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp index 1e588c975..d5736ca06 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp @@ -10,6 +10,7 @@ #include "gjs/jsapi-util.h" static GObject* m_tmp_object = NULL; +static GWeakRef m_tmp_weak; static std::unordered_set m_finalized_objects; static std::mutex m_finalized_objects_lock; @@ -28,6 +29,8 @@ void gjs_test_tools_reset() { else m_tmp_object = nullptr; + g_weak_ref_set(&m_tmp_weak, nullptr); + FinalizedObjectsLocked()->clear(); } @@ -78,6 +81,12 @@ void gjs_test_tools_save_object(GObject* object) { monitor_object_finalization(object); } +void gjs_test_tools_save_object_unreffed(GObject* object) { + g_assert(!m_tmp_object); + m_tmp_object = object; + monitor_object_finalization(object); +} + void gjs_test_tools_ref_other_thread(GObject* object) { // cppcheck-suppress leakNoVarFunctionCall g_thread_join(g_thread_new("ref_object", g_object_ref, object)); @@ -184,6 +193,36 @@ GObject* gjs_test_tools_get_saved() { return static_cast(g_steal_pointer(&m_tmp_object)); } +/** + * gjs_test_tools_steal_saved: + * Returns: (transfer none) + */ +GObject* gjs_test_tools_steal_saved() { return gjs_test_tools_get_saved(); } + +void gjs_test_tools_save_weak(GObject* object) { + g_weak_ref_set(&m_tmp_weak, object); +} + +/** + * gjs_test_tools_get_weak: + * Returns: (transfer full) + */ +GObject* gjs_test_tools_get_weak() { + return static_cast(g_weak_ref_get(&m_tmp_weak)); +} + +/** + * gjs_test_tools_get_weak_other_thread: + * Returns: (transfer full) + */ +GObject* gjs_test_tools_get_weak_other_thread() { + return static_cast( + // cppcheck-suppress leakNoVarFunctionCall + g_thread_join(g_thread_new( + "weak_get", + [](void*) -> void* { return gjs_test_tools_get_weak(); }, NULL))); +} + /** * gjs_test_tools_get_disposed: * Returns: (transfer none) diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.h b/installed-tests/js/libgjstesttools/gjs-test-tools.h index d56fa0f34..7f67413f8 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.h +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.h @@ -35,8 +35,18 @@ void gjs_test_tools_run_dispose_other_thread(GObject* object); void gjs_test_tools_save_object(GObject* object); +void gjs_test_tools_save_object_unreffed(GObject* object); + GObject* gjs_test_tools_get_saved(); +GObject* gjs_test_tools_steal_saved(); + +void gjs_test_tools_save_weak(GObject* object); + +GObject* gjs_test_tools_get_weak(); + +GObject* gjs_test_tools_get_weak_other_thread(); + GObject* gjs_test_tools_get_disposed(GObject* object); G_END_DECLS diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index 4b6e9112a..63819f5e5 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -334,4 +334,113 @@ describe('Disposed or finalized GObject', function () { file = null; System.gc(); }); + + it('can be re-reffed from other thread delayed', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + const objectAddress = System.addressOfGObject(file); + GjsTestTools.save_object_unreffed(file); + GjsTestTools.delayed_ref_other_thread(file, 10); + file = null; + System.gc(); + + const loop = new GLib.MainLoop(null, false); + GLib.timeout_add(GLib.PRIORITY_DEFAULT, 50, () => loop.quit()); + loop.run(); + + // We need to cleanup the extra ref we added before now. + // However, depending on whether the thread ref happens the object + // may be already finalized, and in such case we need to throw + try { + file = GjsTestTools.steal_saved(); + if (file) { + expect(System.addressOfGObject(file)).toBe(objectAddress); + expect(file instanceof Gio.File).toBeTruthy(); + file.unref(); + } + } catch (e) { + expect(() => { + throw e; + }).toThrowError(/.*Unhandled GType.*/); + } + }); + + it('can be re-reffed and unreffed again from other thread', function () { + let file = Gio.File.new_for_path('/'); + const objectAddress = System.addressOfGObject(file); + file.expandMeWithToggleRef = true; + GjsTestTools.save_object(file); + GjsTestTools.delayed_unref_other_thread(file.ref(), 10); + file = null; + System.gc(); + + const loop = new GLib.MainLoop(null, false); + GLib.timeout_add(GLib.PRIORITY_DEFAULT, 50, () => loop.quit()); + loop.run(); + + file = GjsTestTools.get_saved(); + expect(System.addressOfGObject(file)).toBe(objectAddress); + expect(file instanceof Gio.File).toBeTruthy(); + }); + + it('can be re-reffed and unreffed again from other thread with delay', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + GjsTestTools.delayed_ref_unref_other_thread(file, 10); + file = null; + System.gc(); + + const loop = new GLib.MainLoop(null, false); + GLib.timeout_add(GLib.PRIORITY_DEFAULT, 50, () => loop.quit()); + loop.run(); + }); + + it('can be toggled up by getting a GWeakRef', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + GjsTestTools.save_weak(file); + GjsTestTools.get_weak(); + }); + + it('can be toggled up by getting a GWeakRef from another thread', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + GjsTestTools.save_weak(file); + GjsTestTools.get_weak_other_thread(); + }); + + it('can be toggled up by getting a GWeakRef from another thread and re-reffed in main thread', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + GjsTestTools.save_weak(file); + GjsTestTools.get_weak_other_thread(); + + // Ok, let's play more dirty now... + file.ref(); // toggle up + file.unref(); // toggle down + + file.ref(); + file.ref(); + file.unref(); + file.unref(); + }); + + it('can be toggled up by getting a GWeakRef from another and re-reffed from various threads', function () { + let file = Gio.File.new_for_path('/'); + file.expandMeWithToggleRef = true; + GjsTestTools.save_weak(file); + GjsTestTools.get_weak_other_thread(); + + GjsTestTools.ref_other_thread(file); + GjsTestTools.unref_other_thread(file); + + file.ref(); + file.unref(); + + GjsTestTools.ref_other_thread(file); + file.unref(); + + file.ref(); + GjsTestTools.unref_other_thread(file); + }); }); -- GitLab From 51dd60a101dd34cef10edfcc2df6519320cde02a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Sat, 3 Apr 2021 04:45:59 +0200 Subject: [PATCH 16/21] toggle: Initialize private variables on construction --- gi/toggle.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gi/toggle.h b/gi/toggle.h index 38ad88f66..9c9ab2db5 100644 --- a/gi/toggle.h +++ b/gi/toggle.h @@ -42,8 +42,8 @@ private: std::deque q; std::atomic_bool m_shutdown = ATOMIC_VAR_INIT(false); - unsigned m_idle_id; - Handler m_toggle_handler; + unsigned m_idle_id = 0; + Handler m_toggle_handler = nullptr; std::atomic m_toggling_gobj = nullptr; /* No-op unless GJS_VERBOSE_ENABLE_LIFECYCLE is defined to 1. */ -- GitLab From 3ebcc34eddfde2deaae18f6b77fff61892fbee60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Sat, 3 Apr 2021 04:48:04 +0200 Subject: [PATCH 17/21] toggle: Add utility function to handle all toggles Avoid redo the same in various places, just do it once. --- gi/object.cpp | 4 +--- gi/toggle.cpp | 8 ++++++-- gi/toggle.h | 1 + 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index a8ac6d865..b32f094fd 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1401,9 +1401,7 @@ ObjectInstance::release_native_object(void) void gjs_object_clear_toggles(void) { - auto& toggle_queue = ToggleQueue::get_default(); - while (toggle_queue.handle_toggle(toggle_handler)) - ; + ToggleQueue::get_default().handle_all_toggles(toggle_handler); } void diff --git a/gi/toggle.cpp b/gi/toggle.cpp index 9714ebdb1..f627f3ade 100644 --- a/gi/toggle.cpp +++ b/gi/toggle.cpp @@ -44,12 +44,16 @@ ToggleQueue::find_and_erase_operation_locked(const GObject *gobj, return had_toggle; } +void ToggleQueue::handle_all_toggles(Handler handler) { + while (handle_toggle(handler)) + ; +} + gboolean ToggleQueue::idle_handle_toggle(void *data) { auto self = static_cast(data); - while (self->handle_toggle(self->m_toggle_handler)) - ; + self->handle_all_toggles(self->m_toggle_handler); return G_SOURCE_REMOVE; } diff --git a/gi/toggle.h b/gi/toggle.h index 9c9ab2db5..ae7228b8f 100644 --- a/gi/toggle.h +++ b/gi/toggle.h @@ -75,6 +75,7 @@ private: * want to wait for it to be processed in idle time. Returns false if queue * is empty. */ bool handle_toggle(Handler handler); + void handle_all_toggles(Handler handler); /* Checks if the gobj is currently being handled, to avoid recursion */ bool is_being_handled(GObject* gobj); -- GitLab From a6e8c2855d975668db280671b35e31f8c6c4d721 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 6 Apr 2021 19:54:11 +0200 Subject: [PATCH 18/21] GjsTestTools: Move open_bytes here from GjsPrivate This is only needed for testing purposes, so no need to expose it in the exported typelib. --- .../js/libgjstesttools/gjs-test-tools.cpp | 91 ++++++++++++++++++ .../js/libgjstesttools/gjs-test-tools.h | 2 + installed-tests/js/testGDBus.js | 10 +- libgjs-private/gjs-util.c | 92 ------------------- libgjs-private/gjs-util.h | 4 - 5 files changed, 98 insertions(+), 101 deletions(-) diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp index d5736ca06..c018cc2ee 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp @@ -9,6 +9,15 @@ #include "gjs/jsapi-util.h" +#ifdef G_OS_UNIX +# include +# include /* for FD_CLOEXEC */ +# include +# include /* for close, write */ + +# include /* for g_unix_open_pipe */ +#endif + static GObject* m_tmp_object = NULL; static GWeakRef m_tmp_weak; static std::unordered_set m_finalized_objects; @@ -231,3 +240,85 @@ GObject* gjs_test_tools_get_disposed(GObject* object) { g_object_run_dispose(G_OBJECT(object)); return object; } + +#ifdef G_OS_UNIX + +// Adapted from glnx_throw_errno_prefix() +G_GNUC_PRINTF(2, 3) +static gboolean throw_errno_prefix(GError** error, const char* fmt, ...) { + int errsv = errno; + char* old_msg; + GString* buf; + + va_list args; + + if (!error) + return FALSE; + + va_start(args, fmt); + + g_set_error_literal(error, G_IO_ERROR, g_io_error_from_errno(errsv), + g_strerror(errsv)); + + old_msg = g_steal_pointer(&(*error)->message); + buf = g_string_new(""); + g_string_append_vprintf(buf, fmt, args); + g_string_append(buf, ": "); + g_string_append(buf, old_msg); + g_free(old_msg); + (*error)->message = g_string_free(g_steal_pointer(&buf), FALSE); + + va_end(args); + + errno = errsv; + return FALSE; +} + +#endif /* G_OS_UNIX */ + +/** + * gjs_open_bytes: + * @bytes: bytes to send to the pipe + * @error: Return location for a #GError, or %NULL + * + * Creates a pipe and sends @bytes to it, such that it is suitable for passing + * to g_subprocess_launcher_take_fd(). + * + * Returns: file descriptor, or -1 on error + */ +int gjs_test_tools_open_bytes(GBytes* bytes, GError** error) { + int pipefd[2], result; + size_t count; + const void* buf; + ssize_t bytes_written; + + g_return_val_if_fail(bytes, -1); + g_return_val_if_fail(error == NULL || *error == NULL, -1); + +#ifdef G_OS_UNIX + if (!g_unix_open_pipe(pipefd, FD_CLOEXEC, error)) + return -1; + + buf = g_bytes_get_data(bytes, &count); + + bytes_written = write(pipefd[1], buf, count); + if (bytes_written < 0) { + throw_errno_prefix(error, "write"); + return -1; + } + + if ((size_t)bytes_written != count) + g_warning("%s: %zu bytes sent, only %zd bytes written", __func__, count, + bytes_written); + + result = close(pipefd[1]); + if (result == -1) { + throw_errno_prefix(error, "close"); + return -1; + } + + return pipefd[0]; +#else + g_error("%s is currently supported on UNIX only", __func__); +#endif +} diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.h b/installed-tests/js/libgjstesttools/gjs-test-tools.h index 7f67413f8..9ba75df1c 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.h +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.h @@ -49,4 +49,6 @@ GObject* gjs_test_tools_get_weak_other_thread(); GObject* gjs_test_tools_get_disposed(GObject* object); +int gjs_test_tools_open_bytes(GBytes* bytes, GError** error); + G_END_DECLS diff --git a/installed-tests/js/testGDBus.js b/installed-tests/js/testGDBus.js index 6464d3d1f..ae93cda20 100644 --- a/installed-tests/js/testGDBus.js +++ b/installed-tests/js/testGDBus.js @@ -2,7 +2,7 @@ // SPDX-FileCopyrightText: 2008 litl, LLC const ByteArray = imports.byteArray; -const {Gio, GjsPrivate, GLib} = imports.gi; +const {Gio, GjsTestTools, GLib} = imports.gi; /* The methods list with their signatures. * @@ -240,14 +240,14 @@ class Test { } fdOut(bytes) { - const fd = GjsPrivate.open_bytes(bytes); + const fd = GjsTestTools.open_bytes(bytes); const fdList = Gio.UnixFDList.new_from_array([fd]); return [0, fdList]; } fdOut2Async([bytes], invocation) { GLib.idle_add(GLib.PRIORITY_DEFAULT, function () { - const fd = GjsPrivate.open_bytes(bytes); + const fd = GjsTestTools.open_bytes(bytes); const fdList = Gio.UnixFDList.new_from_array([fd]); invocation.return_value_with_unix_fd_list(new GLib.Variant('(h)', [0]), fdList); @@ -556,7 +556,7 @@ describe('Exported DBus object', function () { it('can call a remote method with a Unix FD', function (done) { const expectedBytes = ByteArray.fromString('some bytes'); - const fd = GjsPrivate.open_bytes(expectedBytes); + const fd = GjsTestTools.open_bytes(expectedBytes); const fdList = Gio.UnixFDList.new_from_array([fd]); proxy.fdInRemote(0, fdList, ([bytes], exc, outFdList) => { expect(exc).toBeNull(); @@ -568,7 +568,7 @@ describe('Exported DBus object', function () { it('can call an asynchronously implemented remote method with a Unix FD', function (done) { const expectedBytes = ByteArray.fromString('some bytes'); - const fd = GjsPrivate.open_bytes(expectedBytes); + const fd = GjsTestTools.open_bytes(expectedBytes); const fdList = Gio.UnixFDList.new_from_array([fd]); proxy.fdIn2Remote(0, fdList, ([bytes], exc, outFdList) => { expect(exc).toBeNull(); diff --git a/libgjs-private/gjs-util.c b/libgjs-private/gjs-util.c index 4fd268cd0..9dbe5f6f1 100644 --- a/libgjs-private/gjs-util.c +++ b/libgjs-private/gjs-util.c @@ -9,21 +9,11 @@ #include /* for setlocale */ #include /* for size_t */ -#include #include #include #include #include /* for bindtextdomain, bind_textdomain_codeset, textdomain */ -#ifdef G_OS_UNIX -# include -# include /* for FD_CLOEXEC */ -# include -# include /* for close, write */ - -# include /* for g_unix_open_pipe */ -#endif - #include "libgjs-private/gjs-util.h" char * @@ -108,88 +98,6 @@ gjs_param_spec_get_owner_type(GParamSpec *pspec) return pspec->owner_type; } -#ifdef G_OS_UNIX - -// Adapted from glnx_throw_errno_prefix() -G_GNUC_PRINTF(2, 3) -static gboolean throw_errno_prefix(GError** error, const char* fmt, ...) { - int errsv = errno; - char* old_msg; - GString* buf; - - va_list args; - - if (!error) - return FALSE; - - va_start(args, fmt); - - g_set_error_literal(error, G_IO_ERROR, g_io_error_from_errno(errsv), - g_strerror(errsv)); - - old_msg = g_steal_pointer(&(*error)->message); - buf = g_string_new(""); - g_string_append_vprintf(buf, fmt, args); - g_string_append(buf, ": "); - g_string_append(buf, old_msg); - g_free(old_msg); - (*error)->message = g_string_free(g_steal_pointer(&buf), FALSE); - - va_end(args); - - errno = errsv; - return FALSE; -} - -#endif /* G_OS_UNIX */ - -/** - * gjs_open_bytes: - * @bytes: bytes to send to the pipe - * @error: Return location for a #GError, or %NULL - * - * Creates a pipe and sends @bytes to it, such that it is suitable for passing - * to g_subprocess_launcher_take_fd(). - * - * Returns: file descriptor, or -1 on error - */ -int gjs_open_bytes(GBytes* bytes, GError** error) { - int pipefd[2], result; - size_t count; - const void* buf; - ssize_t bytes_written; - - g_return_val_if_fail(bytes, -1); - g_return_val_if_fail(error == NULL || *error == NULL, -1); - -#ifdef G_OS_UNIX - if (!g_unix_open_pipe(pipefd, FD_CLOEXEC, error)) - return -1; - - buf = g_bytes_get_data(bytes, &count); - - bytes_written = write(pipefd[1], buf, count); - if (bytes_written < 0) { - throw_errno_prefix(error, "write"); - return -1; - } - - if ((size_t)bytes_written != count) - g_warning("%s: %zu bytes sent, only %zd bytes written", __func__, count, - bytes_written); - - result = close(pipefd[1]); - if (result == -1) { - throw_errno_prefix(error, "close"); - return -1; - } - - return pipefd[0]; -#else - g_error("%s is currently supported on UNIX only", __func__); -#endif -} - static GParamSpec* gjs_gtk_container_class_find_child_property( GIObjectInfo* container_info, GObject* container, const char* property) { GIBaseInfo* class_info = NULL; diff --git a/libgjs-private/gjs-util.h b/libgjs-private/gjs-util.h index d4f8bbf6c..c24f23590 100644 --- a/libgjs-private/gjs-util.h +++ b/libgjs-private/gjs-util.h @@ -57,10 +57,6 @@ void gjs_gtk_container_child_set_property(GObject* container, GObject* child, const char* property, const GValue* value); -/* For tests */ -GJS_EXPORT -int gjs_open_bytes(GBytes* bytes, GError** error); - G_END_DECLS #endif /* LIBGJS_PRIVATE_GJS_UTIL_H_ */ -- GitLab From 57083717017c850a8a634d9753c20b5b3fae63cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 6 Apr 2021 22:30:58 +0200 Subject: [PATCH 19/21] object: Improve debugging during wrapping/unwrapping GObject --- gi/object.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index b32f094fd..e253b8430 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -1385,6 +1385,10 @@ ObjectInstance::release_native_object(void) return; } + if (m_ptr) + gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "Releasing native object %s %p", + g_type_name(gtype()), m_ptr.get()); + if (m_gobj_disposed) ignore_gobject_finalization(); @@ -2507,8 +2511,8 @@ ObjectInstance* ObjectInstance::new_for_gobject(JSContext* cx, GObject* gobj) { GType gtype = G_TYPE_FROM_INSTANCE(gobj); - gjs_debug_marshal(GJS_DEBUG_GOBJECT, "Wrapping %s with JSObject", - g_type_name(gtype)); + gjs_debug_marshal(GJS_DEBUG_GOBJECT, "Wrapping %s %p with JSObject", + g_type_name(gtype), gobj); JS::RootedObject proto(cx, gjs_lookup_object_prototype(cx, gtype)); if (!proto) -- GitLab From 08bf63bfea524e9a4a3ccc4656e1d161336a38eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 7 Apr 2021 19:38:38 +0200 Subject: [PATCH 20/21] gjs-test-tools: Use `g_prefix_error` instead of using that ourself --- .../js/libgjstesttools/gjs-test-tools.cpp | 23 ++----------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp index c018cc2ee..54c457bcb 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp @@ -244,31 +244,12 @@ GObject* gjs_test_tools_get_disposed(GObject* object) { #ifdef G_OS_UNIX // Adapted from glnx_throw_errno_prefix() -G_GNUC_PRINTF(2, 3) -static gboolean throw_errno_prefix(GError** error, const char* fmt, ...) { +static gboolean throw_errno_prefix(GError** error, const char* prefix) { int errsv = errno; - char* old_msg; - GString* buf; - - va_list args; - - if (!error) - return FALSE; - - va_start(args, fmt); g_set_error_literal(error, G_IO_ERROR, g_io_error_from_errno(errsv), g_strerror(errsv)); - - old_msg = g_steal_pointer(&(*error)->message); - buf = g_string_new(""); - g_string_append_vprintf(buf, fmt, args); - g_string_append(buf, ": "); - g_string_append(buf, old_msg); - g_free(old_msg); - (*error)->message = g_string_free(g_steal_pointer(&buf), FALSE); - - va_end(args); + g_prefix_error(error, "%s: ", prefix); errno = errsv; return FALSE; -- GitLab From f897012f9075733b530aa07f9f40a24f724f311b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 22 Apr 2021 22:35:49 +0200 Subject: [PATCH 21/21] testGObjectDestructionAccess: Ensure that disposed wrappers are unbound When an object is disposed we discard its wrapper, if a new wrapper for such objects is created it will stay connected to the object till the wrapper is alive or the object is finalized. However, we need to make sure that also the "disposed" wrapper object is always unbound from its wrapped or we'll crash as in issue #395. To test this, we can just repeatedly get from a C function a disposed object so that we're going it to rewrap it over and over. --- .../js/libgjstesttools/gjs-test-tools.cpp | 11 ++++ .../js/libgjstesttools/gjs-test-tools.h | 2 + .../js/testGObjectDestructionAccess.js | 59 ++++++++++++------- 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp index 54c457bcb..276303af4 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp @@ -212,6 +212,17 @@ void gjs_test_tools_save_weak(GObject* object) { g_weak_ref_set(&m_tmp_weak, object); } +/** + * gjs_test_tools_peek_saved: + * Returns: (transfer none) + */ +GObject* gjs_test_tools_peek_saved() { + if (FinalizedObjectsLocked()->count(m_tmp_object)) + return nullptr; + + return m_tmp_object; +} + /** * gjs_test_tools_get_weak: * Returns: (transfer full) diff --git a/installed-tests/js/libgjstesttools/gjs-test-tools.h b/installed-tests/js/libgjstesttools/gjs-test-tools.h index 9ba75df1c..9f692941e 100644 --- a/installed-tests/js/libgjstesttools/gjs-test-tools.h +++ b/installed-tests/js/libgjstesttools/gjs-test-tools.h @@ -41,6 +41,8 @@ GObject* gjs_test_tools_get_saved(); GObject* gjs_test_tools_steal_saved(); +GObject* gjs_test_tools_peek_saved(); + void gjs_test_tools_save_weak(GObject* object); GObject* gjs_test_tools_get_weak(); diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js index 63819f5e5..1ec25c143 100644 --- a/installed-tests/js/testGObjectDestructionAccess.js +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -172,18 +172,29 @@ describe('Disposed or finalized GObject', function () { GjsTestTools.reset(); }); - it('is marked as disposed when it is a manually disposed property', function () { - const emblem = new Gio.EmblemedIcon({ - gicon: new Gio.ThemedIcon({name: 'alarm'}), + [true, false].forEach(gc => { + it(`is marked as disposed when it is a manually disposed property ${gc ? '' : 'not '}garbage collected`, function () { + const emblem = new Gio.EmblemedIcon({ + gicon: new Gio.ThemedIcon({name: 'alarm'}), + }); + + let {gicon} = emblem; + gicon.run_dispose(); + gicon = null; + System.gc(); + + Array(10).fill().forEach(() => { + // We need to repeat the test to ensure that we disassociate + // wrappers from disposed objects on destruction. + gicon = emblem.gicon; + expect(gicon.toString()).toMatch( + /\[object \(DISPOSED\) instance wrapper .* jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + + gicon = null; + if (gc) + System.gc(); + }); }); - - let {gicon} = emblem; - gicon.run_dispose(); - gicon = null; - System.gc(); - - expect(emblem.gicon.toString()).toMatch( - /\[object \(DISPOSED\) instance wrapper .* jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); }); it('calls dispose vfunc on explicit disposal only', function () { @@ -258,15 +269,23 @@ describe('Disposed or finalized GObject', function () { 'generates a warn if already disposed at garbage collection'); }); - it('created from other function is marked as disposed', function () { - let file = Gio.File.new_for_path('/'); - GjsTestTools.save_object(file); - file.run_dispose(); - file = null; - System.gc(); - - expect(GjsTestTools.get_saved()).toMatch( - /\[object \(DISPOSED\) instance wrapper GType:GLocalFile jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + [true, false].forEach(gc => { + it(`created from other function is marked as disposed and ${gc ? '' : 'not '}garbage collected`, function () { + let file = Gio.File.new_for_path('/'); + GjsTestTools.save_object(file); + file.run_dispose(); + file = null; + System.gc(); + + Array(10).fill().forEach(() => { + // We need to repeat the test to ensure that we disassociate + // wrappers from disposed objects on destruction. + expect(GjsTestTools.peek_saved()).toMatch( + /\[object \(DISPOSED\) instance wrapper GType:GLocalFile jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/); + if (gc) + System.gc(); + }); + }); }); it('returned from function is marked as disposed', function () { -- GitLab