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

1 files changed, 38 insertions, 37 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()) {