[gjs] object: Make ensure_toggle_reference to follow the JS API so we can throw

[Philip Chimento <pchimento src gnome org>]

commit 708af7dce9950e2381f3d11afffef48b2d4408f6
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Wed May 12 19:20:24 2021 +0200

    object: Make ensure_toggle_reference to follow the JS API so we can throw

 gi/arg-cache.cpp               |  3 ++-
 gi/object.cpp                  | 46 ++++++++++++++++++++++++++++++------------
 gi/object.h                    |  4 ++--
 test/gjs-test-toggle-queue.cpp |  3 ++-
 4 files changed, 39 insertions(+), 17 deletions(-)
---
diff --git a/gi/arg-cache.cpp b/gi/arg-cache.cpp
index 14e4cff3..53064b36 100644
--- a/gi/arg-cache.cpp
+++ b/gi/arg-cache.cpp
@@ -300,7 +300,8 @@ static bool gjs_marshal_callback_in(JSContext* cx, GjsArgumentCache* self,
                 return false;
             }
 
-            priv->associate_closure(cx, trampoline->js_function());
+            if (!priv->associate_closure(cx, trampoline->js_function()))
+                return false;
         }
         closure = trampoline->closure();
     }
diff --git a/gi/object.cpp b/gi/object.cpp
index 766f88c1..5e27c64b 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -332,7 +332,11 @@ bool ObjectInstance::add_property_impl(JSContext* cx, JS::HandleObject obj,
     if (is_custom_js_class())
         return true;
 
-    ensure_uses_toggle_ref(cx);
+    if (!ensure_uses_toggle_ref(cx)) {
+        gjs_throw(cx, "Impossible to set toggle references on %sobject %p",
+                  m_gobj_disposed ? "disposed " : "", m_ptr.get());
+        return false;
+    }
 
     return true;
 }
@@ -1530,15 +1534,12 @@ ObjectInstance::associate_js_gobject(JSContext       *context,
         g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, this);
 }
 
-// The return value here isn't intended to be JS API like boolean, as we only
-// return whether the object has a toggle reference, and if we've added one
-// and depending on this callers may need to unref the object on failure.
 bool ObjectInstance::ensure_uses_toggle_ref(JSContext* cx) {
     if (m_uses_toggle_ref)
         return true;
 
     if (!check_gobject_disposed_or_finalized("add toggle reference on"))
-        return false;
+        return true;
 
     debug_lifecycle("Switching object instance to toggle ref");
 
@@ -1678,11 +1679,22 @@ ObjectInstance::init_impl(JSContext              *context,
          * we're not actually using it, so just let it get collected. Avoiding
          * this would require a non-trivial amount of work.
          * */
-        if (!other_priv->ensure_uses_toggle_ref(context))
-            gobj = nullptr;
+        bool toggle_ref_added = false;
+        if (!m_uses_toggle_ref) {
+            if (!other_priv->ensure_uses_toggle_ref(context)) {
+                gjs_throw(context,
+                          "Impossible to set toggle references on %sobject %p",
+                          other_priv->m_gobj_disposed ? "disposed " : "", gobj);
+                return false;
+            }
+
+            toggle_ref_added = m_uses_toggle_ref;
+        }
 
         object.set(other_priv->m_wrapper);
-        g_clear_object(&gobj); /* We already own a reference */
+
+        if (toggle_ref_added)
+            g_clear_object(&gobj); /* We already own a reference */
         return true;
     }
 
@@ -1940,9 +1952,14 @@ GIFieldInfo* ObjectPrototype::lookup_cached_field_info(JSContext* cx,
     return parent->lookup_cached_field_info(cx, key);
 }
 
-void ObjectInstance::associate_closure(JSContext* cx, GClosure* closure) {
-    if (!is_prototype())
-        to_instance()->ensure_uses_toggle_ref(cx);
+bool ObjectInstance::associate_closure(JSContext* cx, GClosure* closure) {
+    if (!is_prototype()) {
+        if (!to_instance()->ensure_uses_toggle_ref(cx)) {
+            gjs_throw(cx, "Impossible to set toggle references on %sobject %p",
+                      m_gobj_disposed ? "disposed " : "", to_instance()->ptr());
+            return false;
+        }
+    }
 
     g_assert(std::find(m_closures.begin(), m_closures.end(), closure) ==
                  m_closures.end() &&
@@ -1953,6 +1970,8 @@ void ObjectInstance::associate_closure(JSContext* cx, GClosure* closure) {
     m_closures.push_front(closure);
     g_closure_add_invalidate_notifier(
         closure, this, &ObjectInstance::closure_invalidated_notify);
+
+    return true;
 }
 
 void ObjectInstance::closure_invalidated_notify(void* data, GClosure* closure) {
@@ -2021,7 +2040,8 @@ ObjectInstance::connect_impl(JSContext          *context,
         context, JS_GetObjectFunction(callback), "signal callback", signal_id);
     if (closure == NULL)
         return false;
-    associate_closure(context, closure);
+    if (!associate_closure(context, closure))
+        return false;
 
     id = g_signal_connect_closure_by_id(m_ptr, signal_id, signal_detail,
                                         closure, after);
@@ -2482,7 +2502,7 @@ bool ObjectInstance::init_custom_class_from_gobject(JSContext* cx,
 
     // Custom JS objects will most likely have visible state, so just do this
     // from the start.
-    if (!ensure_uses_toggle_ref(cx)) {
+    if (!ensure_uses_toggle_ref(cx) || !m_uses_toggle_ref) {
         gjs_throw(cx, "Impossible to set toggle references on %sobject %p",
                   m_gobj_disposed ? "disposed " : "", gobj);
         return false;
diff --git a/gi/object.h b/gi/object.h
index 18f02d5e..980aff4a 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -378,7 +378,7 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     static void closure_invalidated_notify(void* data, GClosure* closure);
 
  public:
-    void associate_closure(JSContext* cx, GClosure* closure);
+    GJS_JSAPI_RETURN_CONVENTION bool associate_closure(JSContext*, GClosure*);
 
     /* Helper methods */
 
@@ -388,7 +388,7 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     void track_gobject_finalization();
     void ignore_gobject_finalization();
     void check_js_object_finalized(void);
-    bool ensure_uses_toggle_ref(JSContext* cx);
+    GJS_JSAPI_RETURN_CONVENTION bool ensure_uses_toggle_ref(JSContext* cx);
     [[nodiscard]] bool check_gobject_disposed_or_finalized(
         const char* for_what) const;
     [[nodiscard]] bool check_gobject_finalized(const char* for_what) const;
diff --git a/test/gjs-test-toggle-queue.cpp b/test/gjs-test-toggle-queue.cpp
index 8b25b2c2..426eed7d 100644
--- a/test/gjs-test-toggle-queue.cpp
+++ b/test/gjs-test-toggle-queue.cpp
@@ -116,7 +116,8 @@ static ::ObjectInstance* new_test_gobject(GjsUnitTestFixture* fx) {
     GjsAutoUnref<GObject> gobject(
         G_OBJECT(g_object_new(G_TYPE_OBJECT, nullptr)));
     auto* object = ObjectInstance::new_for_gobject(fx->cx, gobject);
-    static_cast<ObjectInstance*>(object)->ensure_uses_toggle_ref(fx->cx);
+    g_assert_true(
+        static_cast<ObjectInstance*>(object)->ensure_uses_toggle_ref(fx->cx));
     return object;
 }