[gjs/wip/3v1n0/toggle-queue-tests: 6/15] toggle: Keep track of objects via ObjectInstance not GObject's




commit c2ba2ab068af3310680ac45c2271529f6b4bc84c
Author: Marco Trevisan (TreviƱo) <mail 3v1n0 net>
Date:   Wed Apr 28 20:16:55 2021 +0200

    toggle: Keep track of objects via ObjectInstance not GObject's
    
    While the ToggleQueue serves us to track GObject's using their pointers
    is too fragile for us, because they can be also finalized from other
    threads and we may not be able to track when this happens, leaving them
    in the queue to be handled by the main thread, causing us to access to
    invalid memory.
    
    At the contrary, we totally control the life time of ObjectInstance's
    and we're sure that they always are created and destroyed in the main
    thread, so let's use them even inside the ToggleQueue.
    
    In this way:
     - We can keep using the same strategy to enqueue them
     - We don't have to rely on monitoring GObject's finalization to clear
       the queue (that again may not happen in main thread).
     - When an object wrapper is destroyed, we are sure that we've removed
       the gobject toggle reference, and so that we'll never get another
       toggle event queued again till another wrapper is created.
    
    Related to: #404

 gi/object.cpp | 75 +++++++++++++++++++++++++------------------------
 gi/toggle.cpp | 90 ++++++++++++++++++++++++++++++-----------------------------
 gi/toggle.h   | 37 ++++++++++--------------
 3 files changed, 98 insertions(+), 104 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index f8f9eb6a..ca82c3a0 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -261,8 +261,6 @@ 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",
@@ -1122,8 +1120,6 @@ 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());
@@ -1149,7 +1145,7 @@ ObjectInstance::gobj_dispose_notify(void)
     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(m_ptr);
+        ToggleQueue::get_default().cancel(this);
         wrapped_gobj_toggle_notify(this, m_ptr, TRUE);
         m_uses_toggle_ref = false;
     }
@@ -1245,6 +1241,22 @@ ObjectInstance::toggle_down(void)
 void
 ObjectInstance::toggle_up(void)
 {
+    if (G_UNLIKELY(!m_ptr || m_gobj_disposed || m_gobj_finalized)) {
+        if (m_ptr) {
+            gjs_debug_lifecycle(
+                GJS_DEBUG_GOBJECT,
+                "Avoid to toggle up a wrapper for a %s object: %p (%s)",
+                m_gobj_finalized ? "finalized" : "disposed", m_ptr.get(),
+                g_type_name(gtype()));
+        } else {
+            gjs_debug_lifecycle(
+                GJS_DEBUG_GOBJECT,
+                "Avoid to toggle up a wrapper for a released %s object (%p)",
+                g_type_name(gtype()), this);
+        }
+        return;
+    }
+
     /* We need to root the JSObject associated with the passed in GObject so it
      * doesn't get garbage collected (and lose any associated javascript state
      * such as custom properties).
@@ -1266,26 +1278,8 @@ ObjectInstance::toggle_up(void)
     }
 }
 
-static void
-toggle_handler(GObject               *gobj,
-               ToggleQueue::Direction direction)
-{
-    auto* self = ObjectInstance::for_gobject(gobj);
-
-    if (G_UNLIKELY(!self)) {
-        void* disposed = g_object_get_qdata(gobj, ObjectBase::disposed_quark());
-
-        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;
-        }
-
-        // In this case the object has been disposed but its wrapper not yet
-        self = static_cast<ObjectInstance*>(disposed);
-    }
-
+static void toggle_handler(ObjectInstance* self,
+                           ToggleQueue::Direction direction) {
     switch (direction) {
         case ToggleQueue::UP:
             self->toggle_up();
@@ -1343,7 +1337,8 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
     is_main_thread = gjs->is_owner_thread();
 
     auto& toggle_queue = ToggleQueue::get_default();
-    std::tie(toggle_down_queued, toggle_up_queued) = toggle_queue.is_queued(gobj);
+    std::tie(toggle_down_queued, toggle_up_queued) =
+        toggle_queue.is_queued(self);
 
     if (is_last_ref) {
         /* We've transitions from 2 -> 1 references,
@@ -1360,7 +1355,7 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
 
             self->toggle_down();
         } else if (!toggle_down_queued) {
-            toggle_queue.enqueue(gobj, ToggleQueue::DOWN, toggle_handler);
+            toggle_queue.enqueue(self, ToggleQueue::DOWN, toggle_handler);
         }
     } else {
         /* We've transitioned from 1 -> 2 references.
@@ -1377,7 +1372,7 @@ void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject* gobj,
             }
             self->toggle_up();
         } else if (!toggle_up_queued) {
-            toggle_queue.enqueue(gobj, ToggleQueue::UP, toggle_handler);
+            toggle_queue.enqueue(self, ToggleQueue::UP, toggle_handler);
         }
     }
 }
@@ -1490,7 +1485,7 @@ ObjectInstance::weak_pointer_was_finalized(void)
 
         auto& toggle_queue = ToggleQueue::get_default();
         std::tie(toggle_down_queued, toggle_up_queued) =
-            toggle_queue.is_queued(m_ptr);
+            toggle_queue.is_queued(this);
 
         if (!toggle_down_queued && toggle_up_queued)
             return false;
@@ -1499,7 +1494,7 @@ ObjectInstance::weak_pointer_was_finalized(void)
             return false;
 
         if (toggle_down_queued)
-            toggle_queue.cancel(m_ptr);
+            toggle_queue.cancel(this);
 
         /* Ouch, the JS object is dead already. Disassociate the
          * GObject and hope the GObject dies too. (Remove it from
@@ -1608,7 +1603,7 @@ ObjectInstance::disassociate_js_gobject(void)
     bool had_toggle_down, had_toggle_up;
 
     auto& toggle_queue = ToggleQueue::get_default();
-    std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(m_ptr.get());
+    std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(this);
     if (had_toggle_up && !had_toggle_down) {
         g_error(
             "JS object wrapper for GObject %p (%s) is being released while "
@@ -1782,14 +1777,13 @@ ObjectInstance::~ObjectInstance() {
 
     invalidate_closure_list(&m_closures);
 
+    bool had_toggle_up;
+    bool had_toggle_down;
+    auto& toggle_queue = ToggleQueue::get_default();
+    std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(this);
+
     /* GObject is not already freed */
     if (m_ptr) {
-        bool had_toggle_up;
-        bool had_toggle_down;
-
-        auto& toggle_queue = ToggleQueue::get_default();
-        std::tie(had_toggle_down, had_toggle_up) = toggle_queue.cancel(m_ptr);
-
         if (!had_toggle_up && had_toggle_down) {
             g_error(
                 "Finalizing wrapper for an object that's scheduled to be "
@@ -1803,7 +1797,14 @@ ObjectInstance::~ObjectInstance() {
         if (!m_gobj_finalized)
             unset_object_qdata();
 
+        bool was_using_toggle_refs = m_uses_toggle_ref;
         release_native_object();
+
+        if (was_using_toggle_refs) {
+            // We need to cancel again, to be sure that no other thread added
+            // another toggle reference before we were removing the last one.
+            toggle_queue.cancel(this);
+        }
     }
 
     if (wrapper_is_rooted()) {
diff --git a/gi/toggle.cpp b/gi/toggle.cpp
index ca68d16b..74c4ea9c 100644
--- a/gi/toggle.cpp
+++ b/gi/toggle.cpp
@@ -1,42 +1,48 @@
 /* -*- mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
 // SPDX-License-Identifier: MIT OR LGPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017 Endless Mobile, Inc.
+// SPDX-FileCopyrightText: 2021 Canonical Ltd.
 // SPDX-FileContributor: Authored by: Philip Chimento <philip endlessm com>
 // SPDX-FileContributor: Philip Chimento <philip chimento gmail com>
+// SPDX-FileContributor: Marco Trevisan <marco trevisan canonical com>
 
 #include <algorithm>  // for find_if
 #include <deque>
 #include <mutex>
 #include <utility>  // for pair
 
-#include <glib-object.h>
-#include <glib.h>
-
+#include "gi/object.h"
 #include "gi/toggle.h"
+#include "util/log.h"
+
+/* No-op unless GJS_VERBOSE_ENABLE_LIFECYCLE is defined to 1. */
+inline void debug(const char* did GJS_USED_VERBOSE_LIFECYCLE,
+                  const ObjectInstance* object GJS_USED_VERBOSE_LIFECYCLE) {
+    gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "ToggleQueue %s %p (%s @ %p)", did,
+                        object, object ? g_type_name(object->gtype()) : "",
+                        object ? object->ptr() : nullptr);
+}
 
-std::deque<ToggleQueue::Item>::iterator
-ToggleQueue::find_operation_locked(const GObject               *gobj,
-                                   ToggleQueue::Direction direction) {
-    return std::find_if(q.begin(), q.end(),
-        [gobj, direction](const Item& item)->bool {
-            return item.gobj == gobj && item.direction == direction;
+std::deque<ToggleQueue::Item>::iterator ToggleQueue::find_operation_locked(
+    const ObjectInstance* obj, ToggleQueue::Direction direction) {
+    return std::find_if(
+        q.begin(), q.end(), [obj, direction](const Item& item) -> bool {
+            return item.object == obj && item.direction == direction;
         });
 }
 
 std::deque<ToggleQueue::Item>::const_iterator
-ToggleQueue::find_operation_locked(const GObject *gobj,
+ToggleQueue::find_operation_locked(const ObjectInstance* obj,
                                    ToggleQueue::Direction direction) const {
-    return std::find_if(q.begin(), q.end(),
-        [gobj, direction](const Item& item)->bool {
-            return item.gobj == gobj && item.direction == direction;
+    return std::find_if(
+        q.begin(), q.end(), [obj, direction](const Item& item) -> bool {
+            return item.object == obj && item.direction == direction;
         });
 }
 
-bool
-ToggleQueue::find_and_erase_operation_locked(const GObject               *gobj,
-                                             ToggleQueue::Direction direction)
-{
-    auto pos = find_operation_locked(gobj, direction);
+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);
@@ -66,21 +72,19 @@ ToggleQueue::idle_destroy_notify(void *data)
     self->m_toggle_handler = nullptr;
 }
 
-std::pair<bool, bool>
-ToggleQueue::is_queued(GObject *gobj) const
-{
+std::pair<bool, bool> ToggleQueue::is_queued(ObjectInstance* obj) const {
     std::lock_guard<std::mutex> hold(lock);
-    bool has_toggle_down = find_operation_locked(gobj, DOWN) != q.end();
-    bool has_toggle_up = find_operation_locked(gobj, UP) != q.end();
+    bool has_toggle_down = find_operation_locked(obj, DOWN) != q.end();
+    bool has_toggle_up = find_operation_locked(obj, UP) != q.end();
     return {has_toggle_down, has_toggle_up};
 }
 
-std::pair<bool, bool> ToggleQueue::cancel(GObject* gobj) {
-    debug("cancel", gobj);
+std::pair<bool, bool> ToggleQueue::cancel(ObjectInstance* obj) {
+    debug("cancel", obj);
     std::lock_guard<std::mutex> hold(lock);
-    bool had_toggle_down = find_and_erase_operation_locked(gobj, DOWN);
-    bool had_toggle_up = find_and_erase_operation_locked(gobj, UP);
-    gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "ToggleQueue: %p was %s", gobj,
+    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(),
                         had_toggle_down && had_toggle_up
                             ? "queued to toggle BOTH"
                         : had_toggle_down ? "queued to toggle DOWN"
@@ -100,10 +104,11 @@ bool ToggleQueue::handle_toggle(Handler handler) {
         q.pop_front();
     }
 
-
-    debug("handle", item.gobj);
-    handler(item.gobj, item.direction);
-
+    if (item.direction == UP)
+        debug("handle UP", item.object);
+    else
+        debug("handle DOWN", item.object);
+    handler(item.object, item.direction);
 
     return true;
 }
@@ -117,16 +122,13 @@ ToggleQueue::shutdown(void)
     m_shutdown = true;
 }
 
-void
-ToggleQueue::enqueue(GObject               *gobj,
-                     ToggleQueue::Direction direction,
-                     ToggleQueue::Handler   handler)
-{
+void ToggleQueue::enqueue(ObjectInstance* obj, ToggleQueue::Direction direction,
+                          ToggleQueue::Handler handler) {
     if (G_UNLIKELY (m_shutdown)) {
-        gjs_debug(GJS_DEBUG_GOBJECT, "Enqueuing GObject %p to toggle %s after "
-                  "shutdown, probably from another thread (%p).", gobj,
-                  direction == UP ? "UP" : "DOWN",
-                  g_thread_self());
+        gjs_debug(GJS_DEBUG_GOBJECT,
+                  "Enqueuing GObject %p to toggle %s after "
+                  "shutdown, probably from another thread (%p).",
+                  obj->ptr(), direction == UP ? "UP" : "DOWN", g_thread_self());
         return;
     }
 
@@ -139,12 +141,12 @@ ToggleQueue::enqueue(GObject               *gobj,
      * We rely on object's cancelling the queue in case an object gets
      * finalized earlier than we've processed it.
      */
-    q.emplace_back(gobj, direction);
+    q.emplace_back(obj, direction);
 
     if (direction == UP) {
-        debug("enqueue UP", gobj);
+        debug("enqueue UP", obj);
     } else {
-        debug("enqueue DOWN", gobj);
+        debug("enqueue DOWN", obj);
     }
 
     if (m_idle_id) {
diff --git a/gi/toggle.h b/gi/toggle.h
index 028dfd80..c470df67 100644
--- a/gi/toggle.h
+++ b/gi/toggle.h
@@ -7,15 +7,14 @@
 #ifndef GI_TOGGLE_H_
 #define GI_TOGGLE_H_
 
+#include <glib.h>  // for gboolean
+
 #include <atomic>
 #include <deque>
 #include <mutex>
 #include <utility>  // for pair
 
-#include <glib-object.h>
-#include <glib.h>
-
-#include "util/log.h"
+class ObjectInstance;
 
 /* Thread-safe queue for enqueueing toggle-up or toggle-down events on GObjects
  * from any thread. For more information, see object.cpp, comments near
@@ -27,13 +26,13 @@ public:
         UP
     };
 
-    typedef void (*Handler)(GObject *, Direction);
+    using Handler = void (*)(ObjectInstance*, Direction);
 
-private:
+ private:
     struct Item {
         Item() {}
-        Item(GObject* o, Direction d) : gobj(o), direction(d) {}
-        GObject *gobj;
+        Item(ObjectInstance* o, Direction d) : object(o), direction(d) {}
+        ObjectInstance* object;
         ToggleQueue::Direction direction;
     };
 
@@ -44,20 +43,14 @@ private:
     unsigned m_idle_id = 0;
     Handler m_toggle_handler = nullptr;
 
-    /* No-op unless GJS_VERBOSE_ENABLE_LIFECYCLE is defined to 1. */
-    inline void debug(const char* did GJS_USED_VERBOSE_LIFECYCLE,
-                      const void* what GJS_USED_VERBOSE_LIFECYCLE) {
-        gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "ToggleQueue %s %p", did, what);
-    }
-
     [[nodiscard]] std::deque<Item>::iterator find_operation_locked(
-        const GObject* gobj, Direction direction);
+        const ObjectInstance* obj, Direction direction);
 
     [[nodiscard]] std::deque<Item>::const_iterator find_operation_locked(
-        const GObject* gobj, Direction direction) const;
+        const ObjectInstance* obj, Direction direction) const;
 
-    [[nodiscard]] bool find_and_erase_operation_locked(const GObject* gobj,
-                                                       Direction direction);
+    [[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);
@@ -65,9 +58,9 @@ private:
  public:
     /* These two functions return a pair DOWN, UP signifying whether toggles
      * are / were queued. is_queued() just checks and does not modify. */
-    [[nodiscard]] std::pair<bool, bool> is_queued(GObject* gobj) const;
+    [[nodiscard]] std::pair<bool, bool> is_queued(ObjectInstance* obj) const;
     /* Cancels pending toggles and returns whether any were queued. */
-    std::pair<bool, bool> cancel(GObject* gobj);
+    std::pair<bool, bool> cancel(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
@@ -81,9 +74,7 @@ private:
     void shutdown(void);
 
     /* Queues a toggle to be processed in idle time. */
-    void enqueue(GObject  *gobj,
-                 Direction direction,
-                 Handler   handler);
+    void enqueue(ObjectInstance* obj, Direction direction, Handler handler);
 
     [[nodiscard]] static ToggleQueue& get_default() {
         static ToggleQueue the_singleton;


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]