object: don't use toggle references unless necessary

Many GObjects (such as widgets and actors) don't have JS state, but
they're kept alive by C code. We can therefore save some memory
by GCing these objects, and creating new wrappers when needed.
If state is ever set, we transparently switch to toggle refs, so
no change should be visible at the JS level.

https://bugzilla.gnome.org/show_bug.cgi?id=679688

1 files changed, 55 insertions, 18 deletions

diff --git a/gi/object.cpp b/gi/object.cpp
index b20d8b90..6e807084 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -70,6 +70,11 @@ struct ObjectInstance {
 
     unsigned js_object_finalized : 1;
     unsigned g_object_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
+     * hard ref on the underlying GObject, and may be finalized at will. */
+    bool uses_toggle_ref : 1;
 };
 
 static std::stack<JS::PersistentRootedObject> object_init_list;
@@ -85,6 +90,7 @@ extern struct JSClass gjs_object_instance_class;
 GJS_DEFINE_PRIV_FROM_JS(ObjectInstance, gjs_object_instance_class)
 
 static void            disassociate_js_gobject (GObject *gobj);
+static void ensure_uses_toggle_ref(JSContext *cx, ObjectInstance *priv);
 
 typedef enum {
     SOME_ERROR_OCCURRED = false,
@@ -430,6 +436,9 @@ set_g_param_from_prop(JSContext          *context,
     case SOME_ERROR_OCCURRED:
         return false;
     case NO_SUCH_G_PROPERTY:
+        /* We need to keep the wrapper alive in order not to lose custom
+         * "expando" properties */
+        ensure_uses_toggle_ref(context, priv);
         return result.succeed();
     case VALUE_WAS_SET:
     default:
@@ -496,14 +505,7 @@ object_instance_set_prop(JSContext              *context,
     bool ret = true;
     bool g_param_was_set = false;
 
-    if (!gjs_get_string_id(context, id, &name))
-        return result.succeed();  /* not resolved, but no error */
-
     priv = priv_from_js(context, obj);
-    gjs_debug_jsprop(GJS_DEBUG_GOBJECT,
-                     "Set prop '%s' hook obj %p priv %p",
-                     name.get(), obj.get(), priv);
-
     if (priv == nullptr)
         /* see the comment in object_instance_get_prop() on this */
         return result.succeed();
@@ -521,6 +523,18 @@ object_instance_set_prop(JSContext              *context,
         return result.succeed();
     }
 
+    if (!gjs_get_string_id(context, id, &name)) {
+        /* We need to keep the wrapper alive in order not to lose custom
+         * "expando" properties. In this case if gjs_get_string_id() is false
+         * then a number or symbol property was probably set. */
+        ensure_uses_toggle_ref(context, priv);
+        return result.succeed();  /* not resolved, but no error */
+    }
+
+    gjs_debug_jsprop(GJS_DEBUG_GOBJECT,
+                     "Set prop '%s' hook obj %p priv %p",
+                     name.get(), obj.get(), priv);
+
     ret = set_g_param_from_prop(context, priv, name, g_param_was_set, value_p, result);
     if (g_param_was_set || !ret)
         return ret;
@@ -1126,7 +1140,10 @@ static void
 release_native_object (ObjectInstance *priv)
 {
     priv->keep_alive.reset();
-    g_object_remove_toggle_ref(priv->gobj, wrapped_gobj_toggle_notify, NULL);
+    if (priv->uses_toggle_ref)
+        g_object_remove_toggle_ref(priv->gobj, wrapped_gobj_toggle_notify, nullptr);
+    else
+        g_object_unref(priv->gobj);
     priv->gobj = NULL;
 }
 
@@ -1256,16 +1273,28 @@ associate_js_gobject (JSContext       *context,
     ObjectInstance *priv;
 
     priv = priv_from_js(context, object);
+    priv->uses_toggle_ref = false;
     priv->gobj = gobj;
 
     g_assert(!priv->keep_alive.rooted());
 
     set_object_qdata(gobj, priv);
 
+    priv->keep_alive = object;
     ensure_weak_pointer_callback(context);
     wrapped_gobject_list.insert(priv);
 
     g_object_weak_ref(gobj, wrapped_gobj_dispose_notify, priv);
+}
+
+static void
+ensure_uses_toggle_ref(JSContext      *cx,
+                       ObjectInstance *priv)
+{
+    if (priv->uses_toggle_ref)
+        return;
+
+    g_assert(!priv->keep_alive.rooted());
 
     /* OK, here is where things get complicated. We want the
      * wrapped gobj to keep the JSObject* wrapper alive, because
@@ -1278,8 +1307,14 @@ associate_js_gobject (JSContext       *context,
      * the wrapper to be garbage collected (and thus unref the
      * wrappee).
      */
-    priv->keep_alive.root(context, object, gobj_no_longer_kept_alive_func, priv);
-    g_object_add_toggle_ref(gobj, wrapped_gobj_toggle_notify, NULL);
+    priv->uses_toggle_ref = true;
+    priv->keep_alive.switch_to_rooted(cx, gobj_no_longer_kept_alive_func, priv);
+    g_object_add_toggle_ref(priv->gobj, wrapped_gobj_toggle_notify, nullptr);
+
+    /* We now have both a ref and a toggle ref, we only want the toggle ref.
+     * This may immediately remove the GC root we just added, since refcount
+     * may drop to 1. */
+    g_object_unref(priv->gobj);
 }
 
 static void
@@ -1383,6 +1418,7 @@ G_GNUC_END_IGNORE_DEPRECATIONS
          * we're not actually using it, so just let it get collected. Avoiding
          * this would require a non-trivial amount of work.
          * */
+        ensure_uses_toggle_ref(context, other_priv);
         object.set(other_priv->keep_alive);
         g_object_unref(gobj); /* We already own a reference */
         gobj = NULL;
@@ -1410,11 +1446,6 @@ G_GNUC_END_IGNORE_DEPRECATIONS
 
     if (priv->gobj == NULL)
         associate_js_gobject(context, object, gobj);
-    /* We now have both a ref and a toggle ref, we only want the
-     * toggle ref. This may immediately remove the GC root
-     * we just added, since refcount may drop to 1.
-     */
-    g_object_unref(gobj);
 
     gjs_debug_lifecycle(GJS_DEBUG_GOBJECT, "JSObject created with GObject %p (%s)",
                         priv->gobj, G_OBJECT_TYPE_NAME(priv->gobj));
@@ -1683,6 +1714,8 @@ real_connect_func(JSContext *context,
         return true;
     }
 
+    ensure_uses_toggle_ref(context, priv);
+
     if (argc != 2 || !argv[0].isString() || !JS::IsCallable(&argv[1].toObject())) {
         gjs_throw(context, "connect() takes two args, the signal name and the callback");
         return false;
@@ -2123,9 +2156,6 @@ gjs_object_from_g_object(JSContext    *context,
         g_object_ref_sink(gobj);
         associate_js_gobject(context, obj, gobj);
 
-        /* see the comment in init_object_instance() for this */
-        g_object_unref(gobj);
-
         g_assert(priv->keep_alive == obj.get());
     }
 
@@ -2657,6 +2687,10 @@ gjs_object_custom_init(GTypeInstance *instance,
 
     associate_js_gobject(context, object, G_OBJECT (instance));
 
+    /* Custom JS objects will most likely have visible state, so
+     * just do this from the start */
+    ensure_uses_toggle_ref(context, priv);
+
     JS::RootedValue v(context);
     if (!gjs_object_get_property(context, object,
                                  GJS_STRING_INSTANCE_INIT, &v)) {
@@ -3109,6 +3143,9 @@ gjs_object_associate_closure(JSContext       *cx,
     if (!priv)
         return false;
 
+    if (priv->gobj)
+        ensure_uses_toggle_ref(cx, priv);
+
     do_associate_closure(priv, closure);
     return true;
 }