[gjs/gnome-40: 22/30] toggle: Rely on wrapper to cancel finalized objects in queue

[Philip Chimento <pchimento src gnome org>]

commit fa53702e478c2609bfbc8261f60b6be60c70cb66
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Wed Apr 28 01:11:40 2021 +0200

    toggle: Rely on wrapper to cancel finalized objects in queue
    
    While GObject provides native tools to check if an object has been
    finalized using GWeakRef's, they have proved to be too fragile for us
    because they don't work well when used with toggle references
    notifications (not thread safe as glib#2384 demonstrates) and because
    may not be cleared on finalization (glib#2390).
    
    So, instead of adding further qdata to monitor a queued object
    finalization, let's just ensure that a wrapper cancels the toggle queue
    when its wrapped object get finalized.
    
    Since we don't touch the references of the object while adding it to the
    queue, we can also ignore remembering the current handled object, as no
    recursion can happen now.
    
    It may still happen (mostly due to the way we lock) that a finalized
    or unhandled object will be handled by the toggle handler, but this
    can't be really fixed for all the racy cases even at enqueue phase
    (like in the case a thread enqueues a toggle event just after we've
    disassociated the wrapper for such object).
    
    Fixes: #404
    
    (cherry-picked from commit 4e752437)

 gi/object.cpp                                      | 10 ++--
 gi/toggle.cpp                                      | 52 +++---------------
 gi/toggle.h                                        |  7 +--
 .../js/libgjstesttools/gjs-test-tools.cpp          |  6 +++
 .../js/libgjstesttools/gjs-test-tools.h            |  3 ++
 installed-tests/js/testGObjectDestructionAccess.js | 61 ++++++++++++++++++++++
 6 files changed, 84 insertions(+), 55 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index a8ac6d86..3a995b03 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -252,6 +252,8 @@ ObjectInstance::set_object_qdata(void)
                     self->m_ptr.get(), g_type_name(self->gtype()));
                 self->m_gobj_disposed = true;
             }
+            if (ToggleQueue::get_default().cancel(self->m_ptr).first)
+                self->toggle_down();
             self->m_gobj_finalized = true;
             gjs_debug_lifecycle(GJS_DEBUG_GOBJECT,
                                 "Wrapped GObject %p finalized",
@@ -1111,6 +1113,8 @@ void ObjectInstance::track_gobject_finalization() {
     g_object_steal_qdata(m_ptr, quark);
     g_object_set_qdata_full(m_ptr, quark, this, [](void* data) {
         auto* self = static_cast<ObjectInstance*>(data);
+        if (ToggleQueue::get_default().cancel(self->m_ptr).first)
+            self->toggle_down();
         self->m_gobj_finalized = true;
         gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "Wrapped GObject %p finalized",
                             self->m_ptr.get());
@@ -1262,7 +1266,8 @@ toggle_handler(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))) {
+        if (G_UNLIKELY(!disposed ||
+                       disposed == gjs_int_to_pointer(DISPOSED_OBJECT))) {
             g_critical("Handling toggle %s for an unknown object %p",
                        direction == ToggleQueue::UP ? "up" : "down", gobj);
             return;
@@ -1329,9 +1334,6 @@ 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) {
diff --git a/gi/toggle.cpp b/gi/toggle.cpp
index 9714ebdb..601e0aa4 100644
--- a/gi/toggle.cpp
+++ b/gi/toggle.cpp
@@ -13,7 +13,6 @@
 #include <glib.h>
 
 #include "gi/toggle.h"
-#include "gjs/jsapi-util.h"
 
 std::deque<ToggleQueue::Item>::iterator
 ToggleQueue::find_operation_locked(const GObject               *gobj,
@@ -86,12 +85,6 @@ std::pair<bool, bool> ToggleQueue::cancel(GObject* gobj) {
     return {had_toggle_down, had_toggle_up};
 }
 
-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;
     {
@@ -103,34 +96,8 @@ bool ToggleQueue::handle_toggle(Handler handler) {
         q.pop_front();
     }
 
-    /* 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<GObject> gobj(
-            static_cast<GObject*>(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;
+    debug("handle", item.gobj);
+    handler(item.gobj, item.direction);
 
     return true;
 }
@@ -157,25 +124,20 @@ ToggleQueue::enqueue(GObject               *gobj,
         return;
     }
 
-    if (is_being_handled(gobj))
-        return;
-
     std::lock_guard<std::mutex> hold(lock);
-    /* Only keep a weak reference on the object here, as if we're here, the
+    /* Only keep an unowned 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
+     * So let's just save the pointer 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
+     * We rely on object's cancelling the queue in case an object gets
+     * finalized earlier than we've processed it.
      */
-    auto& item = q.emplace_back(gobj, direction);
+    q.emplace_back(gobj, direction);
 
     if (direction == UP) {
         debug("enqueue UP", gobj);
     } else {
-        g_weak_ref_init(&item.weak_ref, gobj);
         debug("enqueue DOWN", gobj);
     }
 
diff --git a/gi/toggle.h b/gi/toggle.h
index 38ad88f6..93dd3c97 100644
--- a/gi/toggle.h
+++ b/gi/toggle.h
@@ -32,9 +32,8 @@ public:
 private:
     struct Item {
         Item() {}
-        Item(GObject* o, ToggleQueue::Direction d) : gobj(o), direction(d) {}
+        Item(GObject* o, Direction d) : gobj(o), direction(d) {}
         GObject *gobj;
-        GWeakRef weak_ref;
         ToggleQueue::Direction direction;
     };
 
@@ -44,7 +43,6 @@ private:
 
     unsigned m_idle_id;
     Handler m_toggle_handler;
-    std::atomic<GObject*> 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,
@@ -76,9 +74,6 @@ 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 43357258..8f401e30 100644
--- a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp
+++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp
@@ -219,6 +219,12 @@ void gjs_test_tools_save_weak(GObject* object) {
     g_weak_ref_set(&m_tmp_weak, object);
 }
 
+int gjs_test_tools_get_saved_ref_count() {
+    if (FinalizedObjectsLocked()->count(m_tmp_object))
+        return 0;
+    return m_tmp_object.load()->ref_count;
+}
+
 /**
  * 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 20f439a5..3948e3ff 100644
--- a/installed-tests/js/libgjstesttools/gjs-test-tools.h
+++ b/installed-tests/js/libgjstesttools/gjs-test-tools.h
@@ -63,6 +63,9 @@ GObject* gjs_test_tools_get_saved();
 _GJS_TEST_TOOL_EXTERN
 GObject* gjs_test_tools_steal_saved();
 
+_GJS_TEST_TOOL_EXTERN
+int gjs_test_tools_get_saved_ref_count();
+
 _GJS_TEST_TOOL_EXTERN
 void gjs_test_tools_clear_saved();
 
diff --git a/installed-tests/js/testGObjectDestructionAccess.js 
b/installed-tests/js/testGObjectDestructionAccess.js
index 967989b8..cb38557d 100644
--- a/installed-tests/js/testGObjectDestructionAccess.js
+++ b/installed-tests/js/testGObjectDestructionAccess.js
@@ -306,6 +306,16 @@ describe('Disposed or finalized GObject', function () {
         file = null;
         System.gc();
     });
+});
+
+describe('GObject with toggle references', function () {
+    beforeAll(function () {
+        GjsTestTools.init();
+    });
+
+    afterEach(function () {
+        GjsTestTools.reset();
+    });
 
     it('can be re-reffed from other thread delayed', function () {
         let file = Gio.File.new_for_path('/');
@@ -415,4 +425,55 @@ describe('Disposed or finalized GObject', function () {
         file.ref();
         GjsTestTools.unref_other_thread(file);
     });
+
+    it('can be finalized while queued in toggle queue', function () {
+        let file = Gio.File.new_for_path('/');
+        file.expandMeWithToggleRef = true;
+        file.ref();
+        GjsTestTools.unref_other_thread(file);
+        GjsTestTools.unref_other_thread(file);
+
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            '*Object 0x* has been finalized *');
+        file = null;
+        System.gc();
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'can be finalized while queued in toggle queue');
+    });
+
+    it('can be toggled up-down from various threads while getting a GWeakRef from main', function () {
+        let file = Gio.File.new_for_path('/');
+        file.expandMeWithToggleRef = true;
+        GjsTestTools.save_weak(file);
+
+        const ids = [];
+        let threads = [];
+        ids.push(GLib.idle_add(GLib.PRIORITY_DEFAULT, () => {
+            threads = threads.slice(-50);
+            threads.push(GjsTestTools.delayed_ref_unref_other_thread(file, 1));
+            return true;
+        }));
+
+        const loop = new GLib.MainLoop(null, false);
+        ids.push(GLib.idle_add(GLib.PRIORITY_DEFAULT, () => {
+            expect(GjsTestTools.get_weak()).toEqual(file);
+            return true;
+        }));
+
+        // We must not timeout due to deadlock #404 and finally not crash per #297
+        GLib.timeout_add(GLib.PRIORITY_DEFAULT, 3000, () => loop.quit());
+        loop.run();
+        ids.forEach(id => GLib.source_remove(id));
+
+        // We also check that late thread events won't affect the destroyed wrapper
+        GjsTestTools.save_object(file);
+        file = null;
+        System.gc();
+        threads.forEach(th => th.join());
+        expect(GjsTestTools.get_saved_ref_count()).toBeGreaterThan(0);
+
+        GjsTestTools.clear_saved();
+        System.gc();
+        expect(GjsTestTools.get_weak()).toBeNull();
+    }).pend('https://gitlab.gnome.org/GNOME/gjs/-/issues/297');
 });