diff --git a/gi/object.cpp b/gi/object.cpp index 5a4bfc66..29920d0d 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -2173,9 +2173,10 @@ void ObjectInstance::gobj_dispose_notify() { 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(this); - wrapped_gobj_toggle_notify(this, m_ptr, TRUE); + retire_toggle_notify_data(); + g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, + toggle_notify_data()); + handle_toggle_notify(true); m_uses_toggle_ref = false; } @@ -2295,10 +2296,39 @@ static void toggle_handler(ObjectInstance* self, } } -void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject*, +void* ObjectInstance::register_toggle_notify_data() { + m_toggle_id = ToggleQueue::get_default()->register_object(this); + return toggle_notify_data(); +} + +void* ObjectInstance::toggle_notify_data() const { + return reinterpret_cast(m_toggle_id); +} + +std::pair ObjectInstance::retire_toggle_notify_data() { + return ToggleQueue::get_default()->retire_object(m_toggle_id, this); +} + +void ObjectInstance::wrapped_gobj_toggle_notify(void* data, GObject*, gboolean is_last_ref) { + // GObject calls the notify after releasing its own locks, so another + // thread can deliver it after the toggle reference was removed and the + // instance freed, with a GObject that may be freed as well (see + // g_object_add_toggle_ref()). The notify data is therefore an identity that + // is never reused, and only the queue lock, held until the notification is + // handled, tells whether it still belongs to a live instance. + auto toggle_queue = ToggleQueue::get_default(); + ObjectInstance* self = toggle_queue->lookup_object( + reinterpret_cast(data)); + if (!self) + return; + + self->handle_toggle_notify(is_last_ref); +} + +void ObjectInstance::handle_toggle_notify(bool is_last_ref) { bool toggle_up_queued, toggle_down_queued; - auto* self = static_cast(instance); + auto* self = this; GjsContextPrivate* gjs = GjsContextPrivate::from_current_context(); if (gjs->destroying()) { @@ -2380,8 +2410,9 @@ void ObjectInstance::release_native_object() { ignore_gobject_finalization(); if (m_uses_toggle_ref && !m_gobj_disposed) { + retire_toggle_notify_data(); g_object_remove_toggle_ref(m_ptr.release(), wrapped_gobj_toggle_notify, - this); + toggle_notify_data()); return; } @@ -2483,15 +2514,28 @@ void ObjectInstance::update_heap_wrapper_weak_pointers(JSTracer* trc, "%zu wrapped GObject(s) to examine", ObjectInstance::num_wrapped_gobjects()); - // Take a lock on the queue till we're done with it, so that we don't - // risk that another thread will queue something else while sweeping - auto locked_queue = ToggleQueue::get_default(); + std::vector disassociated; + { + // Take a lock on the queue till we're done with it, so that we don't + // risk that another thread will queue something else while sweeping + auto locked_queue = ToggleQueue::get_default(); - ObjectInstance::remove_wrapped_gobjects_if( - [&trc](ObjectInstance* instance) -> bool { - return instance->weak_pointer_was_finalized(trc); - }, - std::mem_fn(&ObjectInstance::disassociate_js_gobject)); + ObjectInstance::remove_wrapped_gobjects_if( + [&trc](ObjectInstance* instance) -> bool { + return instance->weak_pointer_was_finalized(trc); + }, + [&disassociated](ObjectInstance* instance) { + instance->disassociate_js_gobject(); + disassociated.push_back(instance); + }); + } + + // Releasing a GObject can run dispose and finalize code that waits for a + // lock held by another thread, while that thread waits for the queue lock + // to deliver a toggle notification. So release them only after unlocking; + // the notifications for the disassociated instances are discarded anyway. + for (ObjectInstance* instance : disassociated) + instance->release_disassociated_js_gobject(); } bool ObjectInstance::weak_pointer_was_finalized(JSTracer* trc) { @@ -2575,7 +2619,8 @@ void 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, this); + g_object_add_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, + register_toggle_notify_data()); /* 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 may @@ -2610,8 +2655,7 @@ static void invalidate_closure_collection(T* closures, void* data, void ObjectInstance::disassociate_js_gobject() { bool had_toggle_down, had_toggle_up; - std::tie(had_toggle_down, had_toggle_up) = - ToggleQueue::get_default()->cancel(this); + std::tie(had_toggle_down, had_toggle_up) = retire_toggle_notify_data(); if (had_toggle_up && !had_toggle_down) { g_error( "JS object wrapper for GObject %p (%s) is being released while " @@ -2626,7 +2670,9 @@ void ObjectInstance::disassociate_js_gobject() { // First, remove the wrapper pointer from the wrapped GObject unset_object_qdata(); } +} +void ObjectInstance::release_disassociated_js_gobject() { // Now release all the resources the current wrapper has invalidate_closures(); release_native_object(); @@ -2799,14 +2845,12 @@ ObjectInstance::~ObjectInstance() { invalidate_closures(); - // Do not keep the queue locked here, as we may want to leave the other - // threads to queue toggle events till we're owning the GObject so that - // eventually (once the toggle reference is finally removed) we can be sure - // that no other toggle event will target this (soon dead) wrapper. + // Do not keep the queue locked here, as releasing the GObject may need + // other threads to make progress. Retiring the toggle notify data first + // ensures that no toggle event will target this (soon dead) wrapper. bool had_toggle_up; bool had_toggle_down; - std::tie(had_toggle_down, had_toggle_up) = - ToggleQueue::get_default()->cancel(this); + std::tie(had_toggle_down, had_toggle_up) = retire_toggle_notify_data(); // GObject is not already freed if (m_ptr) { diff --git a/gi/object.h b/gi/object.h index 745a4b0b..0927ad7a 100644 --- a/gi/object.h +++ b/gi/object.h @@ -11,6 +11,7 @@ #include #include +#include // for pair #include #include @@ -28,6 +29,7 @@ #include #include "gi/info.h" +#include "gi/toggle.h" #include "gi/value.h" #include "gi/wrapperutils.h" #include "cjs/auto.h" @@ -331,6 +333,9 @@ class ObjectInstance : public GIWrapperInstance retire_toggle_notify_data(); public: static void context_dispose_notify(void* data, diff --git a/gi/toggle.cpp b/gi/toggle.cpp index e2b0e142..bdcc6d50 100644 --- a/gi/toggle.cpp +++ b/gi/toggle.cpp @@ -11,6 +11,7 @@ #include // for find_if #include #include +#include #include // for pair #include "gi/object.h" @@ -117,6 +118,28 @@ std::pair ToggleQueue::cancel(ObjectInstance* obj) { return {had_toggle_down, had_toggle_up}; } +ToggleQueue::ObjectId ToggleQueue::register_object(ObjectInstance* obj) { + g_assert(owns_lock() && "Unsafe access to queue"); + if (G_UNLIKELY(m_last_object_id == std::numeric_limits::max())) + g_error("Toggle reference identities exhausted, refusing to reuse one"); + ObjectId id = ++m_last_object_id; + m_objects.emplace(id, obj); + return id; +} + +ObjectInstance* ToggleQueue::lookup_object(ObjectId id) const { + g_assert(owns_lock() && "Unsafe access to queue"); + auto it = m_objects.find(id); + return it == m_objects.end() ? nullptr : it->second; +} + +std::pair ToggleQueue::retire_object(ObjectId id, + ObjectInstance* obj) { + g_assert(owns_lock() && "Unsafe access to queue"); + m_objects.erase(id); + return cancel(obj); +} + bool ToggleQueue::handle_toggle(Handler handler) { g_assert(owns_lock() && "Unsafe access to queue"); diff --git a/gi/toggle.h b/gi/toggle.h index 25a90117..9dbb4731 100644 --- a/gi/toggle.h +++ b/gi/toggle.h @@ -15,6 +15,7 @@ #include #include #include +#include #include // for pair #include // for gboolean @@ -32,6 +33,7 @@ class ToggleQueue { enum Direction : uint8_t { DOWN, UP }; using Handler = void (*)(ObjectInstance*, Direction); + using ObjectId = uintptr_t; private: friend Gjs::Test::ToggleQueue; @@ -49,6 +51,8 @@ class ToggleQueue { }; std::deque q; + std::unordered_map m_objects; + ObjectId m_last_object_id = 0; std::atomic_bool m_shutdown = ATOMIC_VAR_INIT(false); unsigned m_idle_id = 0; @@ -91,6 +95,16 @@ class ToggleQueue { // Cancels pending toggles and returns whether any were queued. std::pair cancel(ObjectInstance*); + /* A toggle notification can arrive after its toggle reference was removed + * and its instance freed, so the notify data is an identity of the toggle + * reference rather than the instance. Identities are never reused. + * register_object() returns a new identity for @obj, lookup_object() + * returns its instance or nullptr once retired, and retire_object() retires + * it and cancels its pending toggles like cancel(). */ + [[nodiscard]] ObjectId register_object(ObjectInstance* obj); + [[nodiscard]] ObjectInstance* lookup_object(ObjectId id) const; + std::pair retire_object(ObjectId id, ObjectInstance* obj); + /* Pops a toggle from the queue and processes it. Call this if you don't * want to wait for it to be processed in idle time. Returns false if queue * is empty. */ diff --git a/test/gjs-test-toggle-queue.cpp b/test/gjs-test-toggle-queue.cpp index 77876ba8..7501d77b 100644 --- a/test/gjs-test-toggle-queue.cpp +++ b/test/gjs-test-toggle-queue.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include // for tie @@ -39,6 +40,10 @@ static std::deque> struct ObjectInstance : ::ObjectInstance { using ::ObjectInstance::ensure_uses_toggle_ref; using ::ObjectInstance::new_for_gobject; + using ::ObjectInstance::disassociate_js_gobject; + using ::ObjectInstance::release_disassociated_js_gobject; + using ::ObjectInstance::toggle_notify_data; + using ::ObjectInstance::wrapped_gobj_toggle_notify; using ::ObjectInstance::wrapper_is_rooted; }; @@ -56,6 +61,9 @@ struct ToggleQueue { static ::ToggleQueue::Handler handler() { return get_default()->m_toggle_handler; } + static void set_last_object_id(::ToggleQueue::ObjectId id) { + get_default()->m_last_object_id = id; + } }; namespace TQ { @@ -633,6 +641,131 @@ static void test_toggle_queue_object_handle_many_up_and_down( g_assert_true(ToggleQueue::queue().empty()); } +static void* toggle_notify_data(::ObjectInstance* instance) { + return static_cast(instance)->toggle_notify_data(); +} + +static ::ToggleQueue::ObjectId toggle_id(::ObjectInstance* instance) { + return reinterpret_cast<::ToggleQueue::ObjectId>( + toggle_notify_data(instance)); +} + +// GObject reads the toggle notify data under its own lock and calls the notify +// only after releasing it, so the call can arrive late from another thread +static void late_toggle_notify(void* data, GObject* gobj, bool is_last_ref) { + std::thread([data, gobj, is_last_ref] { + ObjectInstance::wrapped_gobj_toggle_notify(data, gobj, is_last_ref); + }).join(); +} + +static void test_toggle_queue_object_notify_other_thread(GjsUnitTestFixture* fx, + const void*) { + auto* instance = new_test_gobject(fx); + + late_toggle_notify(toggle_notify_data(instance), instance->ptr(), false); + assert_equal(ToggleQueue::queue().size(), 1LU); + assert_equal(ToggleQueue::queue().at(0).object, instance); + assert_equal(ToggleQueue::queue().at(0).direction, + ::ToggleQueue::Direction::UP); + + ToggleQueue::get_default()->cancel(instance); +} + +static void test_toggle_queue_object_late_notify_retired(GjsUnitTestFixture* fx, + const void*) { + auto* instance = new_test_gobject(fx); + ToggleQueue::get_default()->retire_object(toggle_id(instance), instance); + + late_toggle_notify(toggle_notify_data(instance), instance->ptr(), false); + g_assert_true(ToggleQueue::queue().empty()); + + // A late DOWN may come with a GObject that was finalized meanwhile + late_toggle_notify(toggle_notify_data(instance), nullptr, true); + g_assert_true(ToggleQueue::queue().empty()); +} + +static void test_toggle_queue_object_late_notify_disassociated( + GjsUnitTestFixture* fx, const void*) { + auto* instance = new_test_gobject(fx); + auto* instance_test = static_cast(instance); + void* data = toggle_notify_data(instance); + AutoUnref reffed{instance->ptr(), TakeOwnership{}}; + + // As the GC sweep does: disassociate under the queue lock, release later + instance_test->disassociate_js_gobject(); + late_toggle_notify(data, reffed, false); + g_assert_true(ToggleQueue::queue().empty()); + + instance_test->release_disassociated_js_gobject(); + late_toggle_notify(data, reffed, false); + g_assert_true(ToggleQueue::queue().empty()); + late_toggle_notify(data, reffed, true); + g_assert_true(ToggleQueue::queue().empty()); +} + +static void test_toggle_queue_object_late_notify_finalized( + GjsUnitTestFixture* fx, const void*) { + auto* instance = new_test_gobject(fx); + void* data = toggle_notify_data(instance); + GObject* gobj = instance->ptr(); + + GWeakRef weak_ref; + g_weak_ref_init(&weak_ref, gobj); + TQ::wait_for_gc(fx); + g_assert_null(g_weak_ref_get(&weak_ref)); + g_weak_ref_clear(&weak_ref); + + // The weak ref proves the GObject was freed and the GC retired the toggle + // identity; the late notifications must touch neither + late_toggle_notify(data, gobj, false); + g_assert_true(ToggleQueue::queue().empty()); + late_toggle_notify(data, gobj, true); + g_assert_true(ToggleQueue::queue().empty()); +} + +static void test_toggle_queue_object_late_notify_reused_address( + GjsUnitTestFixture* fx, const void*) { + auto* instance = new_test_gobject(fx); + void* old_data = toggle_notify_data(instance); + ToggleQueue::get_default()->retire_object(toggle_id(instance), instance); + + // Simulates a new instance allocated at the address of the retired one by + // registering the same pointer again + auto new_id = ToggleQueue::get_default()->register_object(instance); + + late_toggle_notify(old_data, instance->ptr(), false); + g_assert_true(ToggleQueue::queue().empty()); + + late_toggle_notify(reinterpret_cast(new_id), instance->ptr(), + false); + assert_equal(ToggleQueue::queue().size(), 1LU); + assert_equal(ToggleQueue::queue().at(0).object, instance); + + ToggleQueue::get_default()->retire_object(new_id, instance); + g_assert_true(ToggleQueue::queue().empty()); +} + +static void test_toggle_queue_object_ids_exhausted(GjsUnitTestFixture* fx, + const void*) { + if (!g_test_subprocess()) { + g_test_trap_subprocess(nullptr, 0, G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_failed(); + g_test_trap_assert_stderr("*identities exhausted*"); + return; + } + + auto* instance = new_test_gobject(fx); + auto max = std::numeric_limits<::ToggleQueue::ObjectId>::max(); + ToggleQueue::set_last_object_id(max - 1); + + auto last_id = ToggleQueue::get_default()->register_object(instance); + assert_equal(last_id, max); + ToggleQueue::get_default()->retire_object(last_id, instance); + + // Wrapping around would hand out identities of retired toggle references + std::ignore = ToggleQueue::get_default()->register_object(instance); +} + void add_tests_for_toggle_queue() { #define ADD_TOGGLE_QUEUE_TEST(path, f) \ g_test_add("/toggle-queue/" path, GjsUnitTestFixture, nullptr, TQ::setup, \ @@ -683,6 +816,18 @@ void add_tests_for_toggle_queue() { test_toggle_queue_object_handle_many_up); ADD_TOGGLE_QUEUE_TEST("object/handle_many_up_and_down", test_toggle_queue_object_handle_many_up_and_down); + ADD_TOGGLE_QUEUE_TEST("object/notify_other_thread", + test_toggle_queue_object_notify_other_thread); + ADD_TOGGLE_QUEUE_TEST("object/late_notify_retired", + test_toggle_queue_object_late_notify_retired); + ADD_TOGGLE_QUEUE_TEST("object/late_notify_disassociated", + test_toggle_queue_object_late_notify_disassociated); + ADD_TOGGLE_QUEUE_TEST("object/late_notify_finalized", + test_toggle_queue_object_late_notify_finalized); + ADD_TOGGLE_QUEUE_TEST("object/late_notify_reused_address", + test_toggle_queue_object_late_notify_reused_address); + ADD_TOGGLE_QUEUE_TEST("object/ids_exhausted", + test_toggle_queue_object_ids_exhausted); #undef ADD_TOGGLE_QUEUE_TEST }