[gjs/wip/3v1n0/toggle-queue-tests: 5/15] toggle: Rely on wrapper to cancel finalized objects in queue

[Philip Chimento <pchimento src gnome org>]

commit 4e7524377ea1a60a878119439c103c1c0db8ac4f
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

 gi/object.cpp                                      | 10 ++--
 gi/toggle.cpp                                      | 50 +++---------------
 gi/toggle.h                                        |  7 +--
 .../js/libgjstesttools/gjs-test-tools.cpp          |  5 ++
 .../js/libgjstesttools/gjs-test-tools.h            |  3 ++
 installed-tests/js/testGObjectDestructionAccess.js | 61 ++++++++++++++++++++++
 6 files changed, 83 insertions(+), 53 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 920d8c15..f8f9eb6a 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -261,6 +261,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",
@@ -1120,6 +1122,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());
@@ -1271,7 +1275,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;
@@ -1338,9 +1343,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 f627f3ad..ca68d16b 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,
@@ -90,12 +89,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;
     {
@@ -107,34 +100,10 @@ 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);
-        }
+    debug("handle", item.gobj);
+    handler(item.gobj, item.direction);
 
-        g_weak_ref_clear(&item.weak_ref);
-    } else {
-        debug("handle", item.gobj);
-        handler(item.gobj, item.direction);
-    }
-
-    m_toggling_gobj = nullptr;
 
     return true;
 }
@@ -161,25 +130,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 ae7228b8..028dfd80 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 = 0;
     Handler m_toggle_handler = nullptr;
-    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,
@@ -77,9 +75,6 @@ private:
     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);
-
     /* 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 f50643a3..06431a54 100644
--- a/installed-tests/js/libgjstesttools/gjs-test-tools.cpp
+++ b/installed-tests/js/libgjstesttools/gjs-test-tools.cpp
@@ -239,6 +239,11 @@ GObject* gjs_test_tools_peek_saved() {
     return m_tmp_object;
 }
 
+int gjs_test_tools_get_saved_ref_count() {
+    GObject* saved = gjs_test_tools_peek_saved();
+    return saved ? saved->ref_count : 0;
+}
+
 /**
  * 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 57388cbe..5bb05827 100644
--- a/installed-tests/js/libgjstesttools/gjs-test-tools.h
+++ b/installed-tests/js/libgjstesttools/gjs-test-tools.h
@@ -66,6 +66,9 @@ GObject* gjs_test_tools_steal_saved();
 _GJS_TEST_TOOL_EXTERN
 GObject* gjs_test_tools_peek_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 900edbdf..a622dc59 100644
--- a/installed-tests/js/testGObjectDestructionAccess.js
+++ b/installed-tests/js/testGObjectDestructionAccess.js
@@ -519,6 +519,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('/');
@@ -671,4 +681,55 @@ describe('Disposed or finalized GObject', function () {
         GjsTestTools.clear_saved();
         expect(GjsTestTools.get_weak()).toBeNull();
     });
+
+    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');
 });