object: fix GC sweep deadlock with toggle notifies and late toggle notify use-after-free

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<void*>(m_toggle_id);
+}
+
+std::pair<bool, bool> 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<ToggleQueue::ObjectId>(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<ObjectInstance*>(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<ObjectInstance*> 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) {

gi/object.h

@@ -11,6 +11,7 @@
 
 #include <functional>
 #include <unordered_set>
+#include <utility>  // for pair
 #include <vector>
 
 #include <girepository/girepository.h>
@@ -28,6 +29,7 @@
 #include <mozilla/Maybe.h>
 
 #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<ObjectBase, ObjectPrototype,
      * ref on the underlying GObject, and may be finalized at will. */
     bool m_uses_toggle_ref : 1;
 
+    /* Notify data of the toggle reference, see ToggleQueue::register_object() */
+    ToggleQueue::ObjectId m_toggle_id = 0;
+
     static bool s_weak_pointer_callback;
 
     // Constructors
@@ -369,6 +374,7 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     void release_native_object();
     void associate_js_gobject(JSContext*, JS::HandleObject, GObject*);
     void disassociate_js_gobject();
+    void release_disassociated_js_gobject();
     void handle_context_dispose();
     [[nodiscard]] bool weak_pointer_was_finalized(JSTracer*);
     static void ensure_weak_pointer_callback(JSContext*);
@@ -513,8 +519,12 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     // Notification callbacks
     void gobj_dispose_notify();
     static void wrapped_gobj_dispose_notify(void* data, GObject*);
-    static void wrapped_gobj_toggle_notify(void* instance, GObject*,
+    static void wrapped_gobj_toggle_notify(void* data, GObject*,
                                            gboolean is_last_ref);
+    void handle_toggle_notify(bool is_last_ref);
+    void* register_toggle_notify_data();
+    void* toggle_notify_data() const;
+    std::pair<bool, bool> retire_toggle_notify_data();
 
  public:
     static void context_dispose_notify(void* data,

gi/toggle.cpp

@@ -11,6 +11,7 @@
 #include <algorithm>  // for find_if
 #include <atomic>
 #include <deque>
+#include <limits>
 #include <utility>  // for pair
 
 #include "gi/object.h"
@@ -117,6 +118,28 @@ std::pair<bool, bool> 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<ObjectId>::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<bool, bool> 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");
 

gi/toggle.h

@@ -15,6 +15,7 @@
 #include <atomic>
 #include <deque>
 #include <thread>
+#include <unordered_map>
 #include <utility>  // for pair
 
 #include <glib.h>  // 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<Item> q;
+    std::unordered_map<ObjectId, ObjectInstance*> 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<bool, bool> 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<bool, bool> 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. */