[gjs/gnome-40: 8/30] object: Discard disposed GObject's and do not create wrappers for them

[Philip Chimento <pchimento src gnome org>]

commit 4b1a690032a880d04fb6ae68296d348a4ddb5db9
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Wed Mar 24 05:57:04 2021 +0100

    object: Discard disposed GObject's and do not create wrappers for them
    
    As per commit d37d6604 and commit c0003eb we may destroy an object
    wrapper earlier than we used to do, this has some memory benefits but
    we also need to make sure that we cleanup the object qdata so that it
    never points to the just free'd object wrapper.
    
    By doing this, however, we may end up re-associating a GObject that is
    disposed but not yet finalized to another valid wrapper. This was allowed
    before but wasn't really correct either because in such case it will be
    just useless and won't be able to monitor its disposal again.
    
    So, now during disposal notify save a qdata (that won't be removed
    until finalization) on gobject marking it as disposed, then use this
    data to prevent an object to be re-associated again to a valid wrapper.
    
    In fact we just create a new wrapper that is marked as "disposed", and
    so preventing most of actions to be performed.
    We don't warn though, because this may just happen at the moment the
    invalid object is incorrectly used.
    As per this, we need to unset the object qdata only if the object is not
    finalized, as we may still need to do it when it's finalized, or we'll
    still crash when a disposed GObject will point to a wrapper that has been
    already destroyed.
    
    Added tests simulating this.
    
    Fixes: #395
    
    (cherry-picked from commit 1532cabd)

 gi/arg.cpp                                         | 11 +++-----
 gi/function.cpp                                    |  5 ++++
 gi/object.cpp                                      | 32 ++++++++++++++++++++--
 gi/object.h                                        |  4 +++
 gi/value.cpp                                       | 15 ++--------
 installed-tests/js/testGObjectDestructionAccess.js | 14 ++++++++++
 6 files changed, 59 insertions(+), 22 deletions(-)
---
diff --git a/gi/arg.cpp b/gi/arg.cpp
index 4574f98c..4f245c5a 100644
--- a/gi/arg.cpp
+++ b/gi/arg.cpp
@@ -2657,13 +2657,10 @@ gjs_value_from_g_argument (JSContext             *context,
             }
 
             if (g_type_is_a(gtype, G_TYPE_OBJECT)) {
-                // Null arg is already handled above
-                JSObject* obj = ObjectInstance::wrapper_from_gobject(
-                    context, G_OBJECT(gjs_arg_get<GObject*>(arg)));
-                if (!obj)
-                    return false;
-                value_p.setObject(*obj);
-                return true;
+                g_assert(gjs_arg_get<void*>(arg) &&
+                         "Null arg is already handled above");
+                return ObjectInstance::set_value_from_gobject(
+                    context, gjs_arg_get<GObject*>(arg), value_p);
             }
 
             if (g_type_is_a(gtype, G_TYPE_BOXED) ||
diff --git a/gi/function.cpp b/gi/function.cpp
index c8d336de..e378f72b 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -365,6 +365,11 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) {
         if (gobj) {
             this_object = ObjectInstance::wrapper_from_gobject(context, gobj);
             if (!this_object) {
+                if (g_object_get_qdata(gobj, ObjectBase::disposed_quark())) {
+                    warn_about_illegal_js_callback(
+                        "on disposed object",
+                        "using the destroy(), dispose(), or remove() vfuncs");
+                }
                 gjs_log_exception(context);
                 return;
             }
diff --git a/gi/object.cpp b/gi/object.cpp
index 74001f92..a7757770 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -10,6 +10,7 @@
 
 #include <algorithm>  // for find
 #include <functional>  // for mem_fn
+#include <limits>
 #include <string>
 #include <tuple>        // for tie
 #include <utility>      // for move
@@ -46,6 +47,7 @@
 #include "gi/object.h"
 #include "gi/repo.h"
 #include "gi/toggle.h"
+#include "gi/utils-inl.h"  // for gjs_int_to_pointer
 #include "gi/value.h"
 #include "gi/wrapperutils.h"
 #include "gjs/atoms.h"
@@ -260,7 +262,9 @@ ObjectInstance::set_object_qdata(void)
 void
 ObjectInstance::unset_object_qdata(void)
 {
-    g_object_steal_qdata(m_ptr, gjs_object_priv_quark());
+    auto priv_quark = gjs_object_priv_quark();
+    if (g_object_get_qdata(m_ptr, priv_quark) == this)
+        g_object_steal_qdata(m_ptr, priv_quark);
 }
 
 GParamSpec* ObjectPrototype::find_param_spec_from_id(JSContext* cx,
@@ -1125,6 +1129,7 @@ ObjectInstance::gobj_dispose_notify(void)
 {
     m_gobj_disposed = true;
 
+    unset_object_qdata();
     track_gobject_finalization();
 
     if (m_uses_toggle_ref) {
@@ -1479,11 +1484,13 @@ ObjectInstance::associate_js_gobject(JSContext       *context,
     m_ptr = gobj;
     set_object_qdata();
     m_wrapper = object;
+    m_gobj_disposed = !!g_object_get_qdata(gobj, ObjectBase::disposed_quark());
 
     ensure_weak_pointer_callback(context);
     link();
 
-    g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, this);
+    if (!G_UNLIKELY(m_gobj_disposed))
+        g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, this);
 }
 
 void
@@ -1550,9 +1557,10 @@ ObjectInstance::disassociate_js_gobject(void)
             m_ptr.get(), type_name());
     }
 
-    if (!m_gobj_disposed) {
+    if (!m_gobj_disposed)
         g_object_weak_unref(m_ptr.get(), wrapped_gobj_dispose_notify, this);
 
+    if (!m_gobj_finalized) {
         /* Fist, remove the wrapper pointer from the wrapped GObject */
         unset_object_qdata();
     }
@@ -2497,6 +2505,24 @@ JSObject* ObjectInstance::wrapper_from_gobject(JSContext* cx, GObject* gobj) {
     return priv->wrapper();
 }
 
+bool ObjectInstance::set_value_from_gobject(JSContext* cx, GObject* gobj,
+                                            JS::MutableHandleValue value_p) {
+    if (!gobj) {
+        value_p.setNull();
+        return true;
+    }
+
+    auto* wrapper = ObjectInstance::wrapper_from_gobject(cx, gobj);
+    if (wrapper) {
+        value_p.setObject(*wrapper);
+        return true;
+    }
+
+    gjs_throw(cx, "Failed to find JS object for GObject %p of type %s", gobj,
+              g_type_name(G_TYPE_FROM_INSTANCE(gobj)));
+    return false;
+}
+
 // Replaces GIWrapperBase::to_c_ptr(). The GIWrapperBase version is deleted.
 bool ObjectBase::to_c_ptr(JSContext* cx, JS::HandleObject obj, GObject** ptr) {
     g_assert(ptr);
diff --git a/gi/object.h b/gi/object.h
index ef875391..8c1d255e 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -362,6 +362,10 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     GJS_JSAPI_RETURN_CONVENTION
     static JSObject* wrapper_from_gobject(JSContext* cx, GObject* ptr);
 
+    GJS_JSAPI_RETURN_CONVENTION
+    static bool set_value_from_gobject(JSContext* cx, GObject*,
+                                       JS::MutableHandleValue);
+
     /* Methods to manipulate the list of closures */
 
  private:
diff --git a/gi/value.cpp b/gi/value.cpp
index 5447dc96..b54dea66 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -828,18 +828,9 @@ gjs_value_from_g_value_internal(JSContext             *context,
         v = g_value_get_boolean(gvalue);
         value_p.setBoolean(!!v);
     } else if (g_type_is_a(gtype, G_TYPE_OBJECT) || g_type_is_a(gtype, G_TYPE_INTERFACE)) {
-        GObject *gobj;
-
-        gobj = (GObject*) g_value_get_object(gvalue);
-
-        if (gobj) {
-            JSObject* obj = ObjectInstance::wrapper_from_gobject(context, gobj);
-            if (!obj)
-                return false;
-            value_p.setObject(*obj);
-        } else {
-            value_p.setNull();
-        }
+        return ObjectInstance::set_value_from_gobject(
+            context, static_cast<GObject*>(g_value_get_object(gvalue)),
+            value_p);
     } else if (gtype == G_TYPE_STRV) {
         if (!gjs_array_from_strv (context,
                                   value_p,
diff --git a/installed-tests/js/testGObjectDestructionAccess.js 
b/installed-tests/js/testGObjectDestructionAccess.js
index 9a59ac0d..28901460 100644
--- a/installed-tests/js/testGObjectDestructionAccess.js
+++ b/installed-tests/js/testGObjectDestructionAccess.js
@@ -142,6 +142,20 @@ describe('Access to destroyed GObject', function () {
 });
 
 describe('Disposed or finalized GObject', function () {
+    it('is marked as disposed when it is a manually disposed property', function () {
+        const emblem = new Gio.EmblemedIcon({
+            gicon: new Gio.ThemedIcon({ name: 'alarm' }),
+        });
+
+        let { gicon } = emblem;
+        gicon.run_dispose();
+        gicon = null;
+        System.gc();
+
+        expect(emblem.gicon.toString()).toMatch(
+            /\[object \(DISPOSED\) instance wrapper .* jsobj@0x[a-f0-9]+ native@0x[a-f0-9]+\]/);
+    });
+
     it('generates a warn on object garbage collection', function () {
         Gio.File.new_for_path('/').unref();