[gjs: 4/5] context: Shut down toggle queue before dispose notify

[Cosimo Cecchi <cosimoc src gnome org>]

commit 3fbdb3733e4e554e00b5ff71bd303ffb50a61c6e
Author: Philip Chimento <philip chimento gmail com>
Date:   Sat Feb 17 23:30:48 2018 -0800

    context: Shut down toggle queue before dispose notify
    
    This adds a method to shut down the toggle queue so that it doesn't
    accept any more toggles. This is intended to stop toggles happening that
    are queued from other threads while the GjsContext dispose sequence has
    already entered its final garbage collection.
    
    We clear pending toggles and shut down the toggle queue before chaining
    up in GjsContext's dispose function. Sending out dispose notifications
    causes all GjsMaybeOwned instances to unroot themselves, which means that
    toggles don't have much meaning after that point.
    
    Closes #26.

 gi/object.cpp                              | 15 +++++++++------
 gi/object.h                                |  1 +
 gi/toggle.cpp                              | 17 +++++++++++++++++
 gi/toggle.h                                | 10 +++++++++-
 gjs/context.cpp                            |  9 ++++++++-
 installed-tests/scripts/testCommandLine.sh |  4 ++++
 6 files changed, 48 insertions(+), 8 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 3567ff5..c78a0a4 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1143,16 +1143,19 @@ gjs_object_clear_toggles(void)
 }
 
 void
-gjs_object_prepare_shutdown(void)
+gjs_object_shutdown_toggle_queue(void)
 {
-    /* First, get rid of anything left over on the main context */
-    gjs_object_clear_toggles();
+    auto& toggle_queue = ToggleQueue::get_default();
+    toggle_queue.shutdown();
+}
 
-    /* Now, we iterate over all of the objects, breaking the JS <-> C
+void
+gjs_object_prepare_shutdown(void)
+{
+    /* We iterate over all of the objects, breaking the JS <-> C
      * association.  We avoid the potential recursion implied in:
      *   toggle ref removal -> gobj dispose -> toggle ref notify
-     * by simply ignoring toggle ref notifications during this process.
-     */
+     * by emptying the toggle queue earlier in the shutdown sequence. */
     std::vector<ObjectInstance *> to_be_released;
     for (auto iter = wrapped_gobject_list.begin(); iter != wrapped_gobject_list.end(); ) {
         ObjectInstance *priv = *iter;
diff --git a/gi/object.h b/gi/object.h
index 8cb1adc..63aeb37 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -60,6 +60,7 @@ bool      gjs_typecheck_is_object(JSContext       *context,
 
 void gjs_object_prepare_shutdown(void);
 void gjs_object_clear_toggles(void);
+void gjs_object_shutdown_toggle_queue(void);
 
 void gjs_object_define_static_methods(JSContext       *context,
                                       JS::HandleObject constructor,
diff --git a/gi/toggle.cpp b/gi/toggle.cpp
index 5010716..6d84d87 100644
--- a/gi/toggle.cpp
+++ b/gi/toggle.cpp
@@ -110,11 +110,28 @@ ToggleQueue::handle_toggle(Handler handler)
     return true;
 }
 
+void
+ToggleQueue::shutdown(void)
+{
+    debug("shutdown", nullptr);
+    g_assert(((void)"Queue should have been emptied before shutting down",
+              q.empty()));
+    m_shutdown = true;
+}
+
 void
 ToggleQueue::enqueue(GObject               *gobj,
                      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());
+        return;
+    }
+
     Item item{gobj, direction};
     /* If we're toggling up we take a reference to the object now,
      * so it won't toggle down before we process it. This ensures we
diff --git a/gi/toggle.h b/gi/toggle.h
index b32714c..f03b6ea 100644
--- a/gi/toggle.h
+++ b/gi/toggle.h
@@ -26,6 +26,7 @@
 #ifndef GJS_TOGGLE_H
 #define GJS_TOGGLE_H
 
+#include <atomic>
 #include <deque>
 #include <mutex>
 #include <glib-object.h>
@@ -53,6 +54,8 @@ private:
 
     std::mutex lock;
     std::deque<Item> q;
+    std::atomic_bool m_shutdown = ATOMIC_VAR_INIT(false);
+
     unsigned m_idle_id;
     Handler m_toggle_handler;
 
@@ -79,7 +82,12 @@ public:
      * want to wait for it to be processed in idle time. Returns false if queue
      * is empty. */
     bool handle_toggle(Handler handler);
-    
+
+    /* 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. */
+    void shutdown(void);
+
     /* Queues a toggle to be processed in idle time. */
     void enqueue(GObject  *gobj,
                  Direction direction,
diff --git a/gjs/context.cpp b/gjs/context.cpp
index 22a0638..433ecc7 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -268,7 +268,14 @@ gjs_context_dispose(GObject *object)
     if (js_context->profiler)
         g_clear_pointer(&js_context->profiler, _gjs_profiler_free);
 
-    /* Run dispose notifications first, so that anything releasing
+    /* Stop accepting entries in the toggle queue before running dispose
+     * notifications, which causes all GjsMaybeOwned instances to unroot.
+     * We don't want any objects to toggle down after that. */
+    gjs_debug(GJS_DEBUG_CONTEXT, "Shutting down toggle queue");
+    gjs_object_clear_toggles();
+    gjs_object_shutdown_toggle_queue();
+
+    /* Run dispose notifications next, so that anything releasing
      * references in response to this can still get garbage collected */
     gjs_debug(GJS_DEBUG_CONTEXT,
               "Notifying reference holders of GjsContext dispose");
diff --git a/installed-tests/scripts/testCommandLine.sh b/installed-tests/scripts/testCommandLine.sh
index 7df3f3d..8dbb2bc 100755
--- a/installed-tests/scripts/testCommandLine.sh
+++ b/installed-tests/scripts/testCommandLine.sh
@@ -198,6 +198,10 @@ report "unhandled promise rejection should be reported"
 test -z "$($gjs awaitcatch.js)"
 report "catching an await expression should not cause unhandled rejection"
 
+# https://gitlab.gnome.org/GNOME/gjs/issues/26
+$gjs -c 'new imports.gi.Gio.Subprocess({argv: ["true"]}).init(null);'
+report "object unref from other thread after shutdown should not race"
+
 rm -f exit.js help.js promise.js awaitcatch.js
 
 echo "1..$total"