Created attachment 268710 [details] [review]
object: Correct assertion about toggle up/down on unref

It's normal and possible for there to be *both* a TOGGLE_UP and a
TOGGLE_DOWN queued.  In that case, they would cancel, and we'd be in a
correct state.

This was consistently triggering in the ostree gjs-based
test-sysroot.js.

Created attachment 268711 [details] [review]
Further fixes for assertions during context dispose

The current toggle ref code has and assertion that we don't have a
DOWN notification pending while the object is supposed to be live -
and during normal operation, this is true.

Except at context shutdown, we force the JS object to be destroyed,
which can cause recursion into a dispose handler of a gobject, which
can then queue a toggle down.  Then we later force dispose that
object, and find the DOWN pending.

Now, the next patch will simply avoid all of this insanity by default
and allow the kernel to clean up our state much faster, in a more
power efficient way by simply not doing any of this shit =)

However, there are valid use cases for wanting "clean" exits, such as
valgrind.  So this patch fixes things up for the dispose by iterating
over the entire live object list before we drop into the JS context
destruction, breaking the JS <-> C association.  This safely clears
the toggle refs, so the JS side is just JS cleanup, we don't reenter
GObject land.

This will make several of the OSTree tests written using gjs start
passing in Continuous again.  \o/

Review of attachment 268710 [details] [review]:

Can you introduce a test for this in the gjs testsuite? I'm not sure how it would trigger...

Review of attachment 268711 [details] [review]:

"the next patch"?

I think you also forgot to git add context-private.h. To be honest, I've been putting that sort of stuff in jsapi-util.h, which is where most of it exists already, but if you want to add a context-private.h, go for it. I might move some other stuff over.

::: gi/object.cpp
@@ +1110,3 @@
+     */
+    gjs_keep_alive_iterator_init (&kiter, keep_alive);
+    while (gjs_keep_alive_iterator_next (&kiter, 

trailing whitespace?

Created attachment 268731 [details] [review]
Fix assertions during context dispose

Well, the nice thing about the InstalledTests model is that the other
tests also act as gjs tests...it'd be really quite painful to manually
replicate the chain of objects in gjs.

I don't really need it anyways, since it appears to only happen during
context dispose anyways, so let's just go with the second patch.

Review of attachment 268731 [details] [review]:

::: gi/keep-alive.cpp
@@ +334,2 @@
 JSObject*
+gjs_keep_alive_get_global_if_exists (JSContext *context)

I know it's annoying, but the gjs style has no spaces between name and ().

::: gi/keep-alive.h
@@ +77,3 @@
+struct GjsKeepAliveIter {
+    gpointer dummy[4];
+    guint v;

What's the motivation for this? We don't have a stable public API to care about.

(In reply to comment #6)

> ::: gi/keep-alive.h
> @@ +77,3 @@
> +struct GjsKeepAliveIter {
> +    gpointer dummy[4];
> +    guint v;
> 
> What's the motivation for this? We don't have a stable public API to care
> about.

Just best practice not to leak implementation details.

Attachment 268731 [details] pushed as be5c12c - Fix assertions during context dispose