[gjs/gnome-40: 25/30] object: Rely on next ToggleQueue iteration to handle already queued toggle events

[Philip Chimento <pchimento src gnome org>]

commit 9e7c0638302a2fe09063e24013614b1c9c4e8395
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Thu Apr 29 05:19:06 2021 +0200

    object: Rely on next ToggleQueue iteration to handle already queued toggle events
    
    When handling the toggle events from the main thread, we may have
    received a notification for a toggle event from multiple threads
    concurrently causing us to consider this an error.
    
    For example:
      Object 0x555555643b30 Toggle up, from main thread 1, queued down 1, up 1
      Object 0x555555643b30 Toggle down, from main thread 1, queued down 1, up 1
      Object 0x555555643b30 Toggle up, from main thread 0, queued down 0, up 0
      Object 0x555555643b30 Toggle up, from main thread 1, queued down 0, up 0
      Object 0x555555643b30 Toggle down, from main thread 0, queued down 0, up 0
      Object 0x555555643b30 Toggle down, from main thread 1, queued down 1, up 0
    
    Till the introduction of locked operations during toggle notifications
    we may not be sure how to handle this, but now we can be quite confident
    that we're working with the actual toggle queue state and so when an
    already-toggled event happens we can just be sure that it will be
    handled at next idle, and so we can stop considering it an error.
    
    To avoid having a queue of events that have no result, change the
    enqueue code to cancel a previously queued event that is the opposite of
    the one we're adding. We need to be able to add multiple events of the
    same type though (as could come from different threads), so adapt the
    cancellation code to this.
    
    As it's just something we can't control our level, but depends on other
    threads fired by applications. What we can do is only react properly,
    and now we do.
    
    Fixes: #297
    
    (cherry-picked from commit f52496c7)

 gi/object.cpp                                      | 23 ++++---------
 gi/toggle.cpp                                      | 38 +++++++++++++++-------
 gi/toggle.h                                        |  3 --
 installed-tests/js/testGObjectDestructionAccess.js |  2 +-
 4 files changed, 33 insertions(+), 33 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index bc6bfa5c..6277004c 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1283,7 +1283,7 @@ static void toggle_handler(ObjectInstance* self,
     }
 }
 
-void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
+void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject*,
                                                 gboolean is_last_ref) {
     bool is_main_thread;
     bool toggle_up_queued, toggle_down_queued;
@@ -1330,21 +1330,16 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
     auto toggle_queue = ToggleQueue::get_default();
     std::tie(toggle_down_queued, toggle_up_queued) =
         toggle_queue->is_queued(self);
+    bool anything_queued = toggle_up_queued || toggle_down_queued;
 
     if (is_last_ref) {
         /* We've transitions from 2 -> 1 references,
          * The JSObject is rooted and we need to unroot it so it
          * can be garbage collected
          */
-        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));
-            }
+        if (is_main_thread && !anything_queued) {
             self->toggle_down();
-        } else if (!toggle_down_queued) {
+        } else {
             toggle_queue->enqueue(self, ToggleQueue::DOWN, toggle_handler);
         }
     } else {
@@ -1353,15 +1348,9 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
          * The JSObject associated with the gobject is not rooted,
          * but it needs to be. We'll root it.
          */
-        if (is_main_thread && !toggle_down_queued) {
-            if (G_UNLIKELY (toggle_up_queued)) {
-                g_error(
-                    "toggling up object %p (%s) that's already queued to "
-                    "toggle up",
-                    gobj, G_OBJECT_TYPE_NAME(gobj));
-            }
+        if (is_main_thread && !anything_queued) {
             self->toggle_up();
-        } else if (!toggle_up_queued) {
+        } else {
             toggle_queue->enqueue(self, ToggleQueue::UP, toggle_handler);
         }
     }
diff --git a/gi/toggle.cpp b/gi/toggle.cpp
index a852e2fa..0884a3b2 100644
--- a/gi/toggle.cpp
+++ b/gi/toggle.cpp
@@ -62,15 +62,6 @@ ToggleQueue::find_operation_locked(const ObjectInstance* obj,
         });
 }
 
-bool ToggleQueue::find_and_erase_operation_locked(
-    const ObjectInstance* obj, ToggleQueue::Direction direction) {
-    auto pos = find_operation_locked(obj, direction);
-    bool had_toggle = (pos != q.end());
-    if (had_toggle)
-        q.erase(pos);
-    return had_toggle;
-}
-
 gboolean
 ToggleQueue::idle_handle_toggle(void *data)
 {
@@ -99,9 +90,21 @@ std::pair<bool, bool> ToggleQueue::is_queued(ObjectInstance* obj) const {
 std::pair<bool, bool> ToggleQueue::cancel(ObjectInstance* obj) {
     debug("cancel", obj);
     g_assert(owns_lock() && "Unsafe access to queue");
-    bool had_toggle_down = find_and_erase_operation_locked(obj, DOWN);
-    bool had_toggle_up = find_and_erase_operation_locked(obj, UP);
-    gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "ToggleQueue: %p was %s", obj->ptr(),
+    bool had_toggle_down = false;
+    bool had_toggle_up = false;
+
+    for (auto it = q.begin(); it != q.end();) {
+        if (it->object == obj) {
+            had_toggle_down |= (it->direction == Direction::DOWN);
+            had_toggle_up |= (it->direction == Direction::UP);
+            it = q.erase(it);
+            continue;
+        }
+        it++;
+    }
+
+    gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "ToggleQueue: %p (%p) was %s", obj,
+                        obj ? obj->ptr() : nullptr,
                         had_toggle_down && had_toggle_up
                             ? "queued to toggle BOTH"
                         : had_toggle_down ? "queued to toggle DOWN"
@@ -149,6 +152,17 @@ void ToggleQueue::enqueue(ObjectInstance* obj, ToggleQueue::Direction direction,
         return;
     }
 
+    auto other_item = find_operation_locked(obj, direction == UP ? DOWN : UP);
+    if (other_item != q.end()) {
+        if (direction == UP) {
+            debug("enqueue UP, dequeuing already DOWN object", obj);
+        } else {
+            debug("enqueue DOWN, dequeuing already UP object", obj);
+        }
+        q.erase(other_item);
+        return;
+    }
+
     /* 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.
diff --git a/gi/toggle.h b/gi/toggle.h
index e1aa69f4..0f2e47da 100644
--- a/gi/toggle.h
+++ b/gi/toggle.h
@@ -67,9 +67,6 @@ public:
     [[nodiscard]] std::deque<Item>::const_iterator find_operation_locked(
         const ObjectInstance* obj, Direction direction) const;
 
-    [[nodiscard]] bool find_and_erase_operation_locked(
-        const ObjectInstance* obj, Direction direction);
-
     static gboolean idle_handle_toggle(void *data);
     static void idle_destroy_notify(void *data);
 
diff --git a/installed-tests/js/testGObjectDestructionAccess.js 
b/installed-tests/js/testGObjectDestructionAccess.js
index cb38557d..4454ce1f 100644
--- a/installed-tests/js/testGObjectDestructionAccess.js
+++ b/installed-tests/js/testGObjectDestructionAccess.js
@@ -475,5 +475,5 @@ describe('GObject with toggle references', function () {
         GjsTestTools.clear_saved();
         System.gc();
         expect(GjsTestTools.get_weak()).toBeNull();
-    }).pend('https://gitlab.gnome.org/GNOME/gjs/-/issues/297');
+    });
 });