[gjs/gnome-40: 7/30] object: Catch finalized objects that are still under gjs scope

[Philip Chimento <pchimento src gnome org>]

commit b3324a72c64f3ec4dd919fc4f975e253b4c3a4a6
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Wed Mar 24 07:40:47 2021 +0100

    object: Catch finalized objects that are still under gjs scope
    
    When an object is finalized its qdata is finally removed, so if while we
    track the object in the wrapper the qdata is removed (after a disposal)
    then we can be sure that the object is finalized.
    
    We can so track this state by using a GDestroyNotify function as it will
    be called with the wrapper pointer and so we can easily mark the object
    as finalized in such case.
    
    While the object is in the disposed state, waiting for finalization we
    have to temporary give the role of monitoring the qdata to the disposed
    quark so that during this phase the object has still the private qdata
    unset, but still monitored for finalization, until we stop caring about
    it.
    If instead the object is created as a wrapper for a disposed object,
    the private qdata still serves us to monitor its finalization without
    having to override the disposed one, that may still be used by another
    wrapper that is still living waiting for being garbage collected.
    
    If this happens we can finally write proper critical messages and avoid
    accessing to the object again.
    
    Added a test to cover this case
    
    (cherry-picked from commit 3303948f)

 gi/object.cpp                                      | 64 ++++++++++++++++++++--
 gi/object.h                                        |  4 ++
 installed-tests/js/testGObjectDestructionAccess.js | 32 ++++++++++-
 3 files changed, 92 insertions(+), 8 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 45ce94bd..74001f92 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -79,9 +79,12 @@ static_assert(sizeof(ObjectInstance) <= 88,
 bool ObjectInstance::s_weak_pointer_callback = false;
 ObjectInstance *ObjectInstance::wrapped_gobject_list = nullptr;
 
+static const auto DISPOSED_OBJECT = std::numeric_limits<uintptr_t>::max();
+
 // clang-format off
 G_DEFINE_QUARK(gjs::custom-type, ObjectBase::custom_type)
 G_DEFINE_QUARK(gjs::custom-property, ObjectBase::custom_property)
+G_DEFINE_QUARK(gjs::disposed, ObjectBase::disposed)
 // clang-format on
 
 [[nodiscard]] static GQuark gjs_object_priv_quark() {
@@ -237,13 +240,27 @@ void ObjectPrototype::set_type_qdata(void) {
 void
 ObjectInstance::set_object_qdata(void)
 {
-    g_object_set_qdata(m_ptr, gjs_object_priv_quark(), this);
+    g_object_set_qdata_full(
+        m_ptr, gjs_object_priv_quark(), this, [](void* object) {
+            auto* self = static_cast<ObjectInstance*>(object);
+            if (G_UNLIKELY(!self->m_gobj_disposed)) {
+                g_warning(
+                    "Object %p (a %s) was finalized but we didn't track "
+                    "its disposal",
+                    self->m_ptr.get(), g_type_name(self->gtype()));
+                self->m_gobj_disposed = true;
+            }
+            self->m_gobj_finalized = true;
+            gjs_debug_lifecycle(GJS_DEBUG_GOBJECT,
+                                "Wrapped GObject %p finalized",
+                                self->m_ptr.get());
+        });
 }
 
 void
 ObjectInstance::unset_object_qdata(void)
 {
-    g_object_set_qdata(m_ptr, gjs_object_priv_quark(), nullptr);
+    g_object_steal_qdata(m_ptr, gjs_object_priv_quark());
 }
 
 GParamSpec* ObjectPrototype::find_param_spec_from_id(JSContext* cx,
@@ -1084,11 +1101,32 @@ static void wrapped_gobj_dispose_notify(
                         where_the_object_was);
 }
 
+void ObjectInstance::track_gobject_finalization() {
+    auto quark = ObjectBase::disposed_quark();
+    g_object_steal_qdata(m_ptr, quark);
+    g_object_set_qdata_full(m_ptr, quark, this, [](void* data) {
+        auto* self = static_cast<ObjectInstance*>(data);
+        self->m_gobj_finalized = true;
+        gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "Wrapped GObject %p finalized",
+                            self->m_ptr.get());
+    });
+}
+
+void ObjectInstance::ignore_gobject_finalization() {
+    auto quark = ObjectBase::disposed_quark();
+    if (g_object_get_qdata(m_ptr, quark) == this) {
+        g_object_steal_qdata(m_ptr, quark);
+        g_object_set_qdata(m_ptr, quark, gjs_int_to_pointer(DISPOSED_OBJECT));
+    }
+}
+
 void
 ObjectInstance::gobj_dispose_notify(void)
 {
     m_gobj_disposed = true;
 
+    track_gobject_finalization();
+
     if (m_uses_toggle_ref) {
         g_object_ref(m_ptr.get());
         g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this);
@@ -1307,6 +1345,19 @@ void
 ObjectInstance::release_native_object(void)
 {
     discard_wrapper();
+
+    if (m_gobj_finalized) {
+        g_critical(
+            "Object %p of type %s has been finalized while it was still "
+            "owned by gjs, this is due to invalid memory management.",
+            m_ptr.get(), g_type_name(gtype()));
+        m_ptr.release();
+        return;
+    }
+
+    if (m_gobj_disposed)
+        ignore_gobject_finalization();
+
     if (m_uses_toggle_ref && !m_gobj_disposed)
         g_object_remove_toggle_ref(m_ptr.release(), wrapped_gobj_toggle_notify,
                                    this);
@@ -1352,6 +1403,7 @@ ObjectInstance::ObjectInstance(JSContext* cx, JS::HandleObject object)
     : GIWrapperInstance(cx, object),
       m_wrapper_finalized(false),
       m_gobj_disposed(false),
+      m_gobj_finalized(false),
       m_uses_toggle_ref(false) {
     GTypeQuery query;
     type_query_dynamic_safe(&query);
@@ -1667,11 +1719,11 @@ ObjectInstance::~ObjectInstance() {
         bool had_toggle_up;
         bool had_toggle_down;
 
-        if (G_UNLIKELY(m_ptr->ref_count <= 0)) {
+        if (m_gobj_finalized || G_UNLIKELY(m_ptr->ref_count <= 0)) {
             g_error(
                 "Finalizing wrapper for an already freed object of type: "
-                "%s.%s\n",
-                ns(), name());
+                "%s.%s: %p\n",
+                ns(), name(), m_ptr.get());
         }
 
         auto& toggle_queue = ToggleQueue::get_default();
@@ -2239,6 +2291,8 @@ bool ObjectBase::to_string(JSContext* cx, unsigned argc, JS::Value* vp) {
  * wrapped GObject has already been disposed.
  */
 const char* ObjectInstance::to_string_kind(void) const {
+    if (m_gobj_finalized)
+        return "object (FINALIZED)";
     return m_gobj_disposed ? "object (DISPOSED)" : "object";
 }
 
diff --git a/gi/object.h b/gi/object.h
index 9e10b676..ef875391 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -177,6 +177,7 @@ class ObjectBase
 
     [[nodiscard]] static GQuark custom_type_quark();
     [[nodiscard]] static GQuark custom_property_quark();
+    [[nodiscard]] static GQuark disposed_quark();
 };
 
 // See https://bugzilla.mozilla.org/show_bug.cgi?id=1614220
@@ -304,6 +305,7 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
 
     bool m_wrapper_finalized : 1;
     bool m_gobj_disposed : 1;
+    bool m_gobj_finalized : 1;
 
     /* True if this object has visible JS state, and thus its lifecycle is
      * managed using toggle references. False if this object just keeps a
@@ -373,6 +375,8 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
  private:
     void set_object_qdata(void);
     void unset_object_qdata(void);
+    void track_gobject_finalization();
+    void ignore_gobject_finalization();
     void check_js_object_finalized(void);
     void ensure_uses_toggle_ref(JSContext* cx);
     [[nodiscard]] bool check_gobject_disposed(const char* for_what) const;
diff --git a/installed-tests/js/testGObjectDestructionAccess.js 
b/installed-tests/js/testGObjectDestructionAccess.js
index 2391aacb..9a59ac0d 100644
--- a/installed-tests/js/testGObjectDestructionAccess.js
+++ b/installed-tests/js/testGObjectDestructionAccess.js
@@ -4,9 +4,8 @@
 
 imports.gi.versions.Gtk = '3.0';
 
-const GLib = imports.gi.GLib;
-const GObject = imports.gi.GObject;
-const Gtk = imports.gi.Gtk;
+const {GLib, Gio, GObject, Gtk} = imports.gi;
+const {system: System} = imports;
 
 describe('Access to destroyed GObject', function () {
     let destroyedWindow;
@@ -141,3 +140,30 @@ describe('Access to destroyed GObject', function () {
             /\[object \(DISPOSED\) instance wrapper GIName:Gtk.Window jsobj@0x[a-f0-9]+ 
native@0x[a-f0-9]+\]/);
     });
 });
+
+describe('Disposed or finalized GObject', function () {
+    it('generates a warn on object garbage collection', function () {
+        Gio.File.new_for_path('/').unref();
+
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            '*Object 0x* has been finalized *');
+        System.gc();
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'generates a warn on object garbage collection');
+    });
+
+    it('generates a warn on object garbage collection if has expando property', function () {
+        let file = Gio.File.new_for_path('/');
+        file.toggleReferenced = true;
+        file.unref();
+        expect(file.toString()).toMatch(
+            /\[object \(FINALIZED\) instance wrapper GType:GLocalFile jsobj@0x[a-f0-9]+ 
native@0x[a-f0-9]+\]/);
+        file = null;
+
+        GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_CRITICAL,
+            '*Object 0x* has been finalized *');
+        System.gc();
+        GLib.test_assert_expected_messages_internal('Gjs', 'testGObjectDestructionAccess.js', 0,
+            'generates a warn on object garbage collection');
+    });
+});