Created attachment 207147 [details] [review]
union: Clean up old, broken log statement

Created attachment 207148 [details] [review]
object: Properly manage memory for GObjects

Created attachment 207150 [details] [review]
object: Make sure that we properly update the counters for custom objects

Since we initialize GObjects through construct properties, we need to
make sure that we don't get fooled and think that g_object_newv returned
a singleton.

Created attachment 207151 [details] [review]
function: Fix a few memory leaks

Review of attachment 207148 [details] [review]:

Uhm uhm uhm...
I didn't know GObject memory management was so screwed up - we used to valgrind gjs tests, rigth?

::: gi/object.c
@@ +891,3 @@
+        /* we should already have a ref */
+    }
+

I think it is wrong to have this special treatment here: it only applies to g_object_newv (which cannot be correctly marked (transfer none) or (transfer full) depending on the underlying constructor), not to other functions returning gobjects.

@@ -975,3 @@
-     * we just added, since refcount may drop to 1.
-     */
-    g_object_unref(gobj);

g_object_add_toggle_ref does g_object_ref internally, so you really need to pair the reference returned by g_object_newv (or the hacky workaround for a (transfer none) of a non floating object with a g_object_unref().

@@ -1786,3 @@
 
-        /* see the comment in init_object_instance() for this */
-        g_object_unref(gobj);

Same here, g_object_unref goes on pair with g_object_ref_sink, which ensures that we only ever handle non-floating objects.
(For floating objects, this brings down the ref_count to 1, which is correct since the only reference is the toggle reference. For non-floating objects, this simply pairs the above g_object_ref_sink, and actual refcounting correctness is handled by argument cleanup)

Review of attachment 207147 [details] [review]:

I think this is fine.
(Does anyone make a real use of gjs_debug_lifecycle?)

Review of attachment 207150 [details] [review]:

Uh! Good catch!

Review of attachment 207151 [details] [review]:

Sure!

Review of attachment 207148 [details] [review]:

::: gi/object.c
@@ +891,3 @@
+        /* we should already have a ref */
+    }
+

Ok, I see what you did now: if you inherit from GInitiallyUnowned or GtkWindow, you want the extra refs already when JS property code is called, so you can pass your object to C API freely.
The problem is that this adds an extra unbalanced ref for normal method calls. You may want to add a flag in associate_js_object (and a big big comment too)

Review of attachment 207148 [details] [review]:

::: gi/object.c
@@ +891,3 @@
+        /* we should already have a ref */
+    }
+

By "normal method calls" do you mean a C API that returns a new GtkWindow?

In that case, we should be creating a new wrapper object through gjs_object_from_g_object, where we want to sink and ref the object so that we can properly set up a toggle ref, right?

@@ -975,3 @@
-     * we just added, since refcount may drop to 1.
-     */
-    g_object_unref(gobj);

Aha, I didn't realize g_object_add_toggle_ref added a ref internally. (But the existing code was still broken - if associate_js_gobject wasn't called, we'd be unreffing a ref we never added)

(In reply to comment #10)
> Review of attachment 207148 [details] [review]:
> 
> ::: gi/object.c
> @@ +891,3 @@
> +        /* we should already have a ref */
> +    }
> +
> 
> By "normal method calls" do you mean a C API that returns a new GtkWindow?

A non-floating GtkWindow, or a floating GInitiallyUnowned, yes.
(It mostly happens with explicit constructor calls, but to the gjs eyes, those are just functions)

> In that case, we should be creating a new wrapper object through
> gjs_object_from_g_object, where we want to sink and ref the object so that we
> can properly set up a toggle ref, right?

Yes, but unless i misread it your patch removes that ref_sink. Also, gjs_object_from_g_object() calls associate_js_object(), so the latter must not mess with ref counts unless it's impossible to do otherwise.

> @@ -975,3 @@
> -     * we just added, since refcount may drop to 1.
> -     */
> -    g_object_unref(gobj);
> 
> Aha, I didn't realize g_object_add_toggle_ref added a ref internally. (But the
> existing code was still broken - if associate_js_gobject wasn't called, we'd be
> unreffing a ref we never added)

How is that possible? associate_js_gobject() is called right before that code (unless priv->gobj is not NULL, which means that something, somewhere, called it)

(In reply to comment #11)
> (In reply to comment #10)
> > Review of attachment 207148 [details] [review] [details]:
> > 
> > ::: gi/object.c
> > @@ +891,3 @@
> > +        /* we should already have a ref */
> > +    }
> > +
> > 
> > By "normal method calls" do you mean a C API that returns a new GtkWindow?
> 
> A non-floating GtkWindow, or a floating GInitiallyUnowned, yes.
> (It mostly happens with explicit constructor calls, but to the gjs eyes, those
> are just functions)
> 
> > In that case, we should be creating a new wrapper object through
> > gjs_object_from_g_object, where we want to sink and ref the object so that we
> > can properly set up a toggle ref, right?
> 
> Yes, but unless i misread it your patch removes that ref_sink.

associate_g_object sinks the ref if it's floating.

> Also,
> gjs_object_from_g_object() calls associate_js_object(), so the latter must not
> mess with ref counts unless it's impossible to do otherwise.

The only time gjs_object_from_g_object is called is when we create a new wrapper, so we should sink any existing floating ref, and add a toggle ref, right?

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Review of attachment 207148 [details] [review] [details] [details]:
> > > 
> > > ::: gi/object.c
> > > @@ +891,3 @@
> > > +        /* we should already have a ref */
> > > +    }
> > > +
> > > 
> > > By "normal method calls" do you mean a C API that returns a new GtkWindow?
> > 
> > A non-floating GtkWindow, or a floating GInitiallyUnowned, yes.
> > (It mostly happens with explicit constructor calls, but to the gjs eyes, those
> > are just functions)
> > 
> > > In that case, we should be creating a new wrapper object through
> > > gjs_object_from_g_object, where we want to sink and ref the object so that we
> > > can properly set up a toggle ref, right?
> > 
> > Yes, but unless i misread it your patch removes that ref_sink.
> 
> associate_g_object sinks the ref if it's floating.

True, but then it also adds an extra ref for GInitiallyUnowned that are not floating, which is wrong.

> > Also,
> > gjs_object_from_g_object() calls associate_js_object(), so the latter must not
> > mess with ref counts unless it's impossible to do otherwise.
> 
> The only time gjs_object_from_g_object is called is when we create a new
> wrapper, so we should sink any existing floating ref, and add a toggle ref,
> right?

Yes, but after sinking you need to unref, since you have two refs (one from ref_sink, the other from add_toggle_ref) and only want one. Problem is, after ref_sinking you don't know if the object was indeed floating or not, so you don't known if you need to unref.
At that point, ref_sink + unref always is simpler (for gjs_object_from_g_object()).

Attachment 207147 [details] pushed as 6e179e9 - union: Clean up old, broken log statement
Attachment 207151 [details] pushed as 09c9964 - function: Fix a few memory leaks


I opted not to push the "Properly manage memory" commit -- it needs reworking, but I repurposed the "update the counters" commit to solve the crash with custom gobjects.