[gjs/gnome-3-28] object: don't use toggle references unless necessary

[Philip Chimento <pchimento src gnome org>]

commit 29579aadae6b009e002787b23b220de0f7e3ce81
Author: Giovanni Campagna <gcampagna src gnome org>
Date:   Sun Sep 17 15:09:47 2017 -0700

    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
    
    (cherry picked from commit 0cc23474f815fc71cad911cef16c1200de27981c)

 gi/object.cpp | 73 ++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 18 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index e7d63b0a..a023fe63 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -131,6 +131,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;
@@ -146,6 +151,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,
@@ -491,6 +497,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:
@@ -557,14 +566,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();
@@ -582,6 +584,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;
@@ -1197,7 +1211,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;
 }
 
@@ -1331,16 +1348,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);
     object_instance_link(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
@@ -1353,8 +1382,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
@@ -1458,6 +1493,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;
@@ -1485,11 +1521,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));
@@ -1749,6 +1780,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;
@@ -2189,9 +2222,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());
     }
 
@@ -2724,6 +2754,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)) {
@@ -3176,6 +3210,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;
 }