(gdb) bt

+ Trace 231744

In the test suite, we're destroying the context forcibly, and thus collecting all objects.  This is tricky - possibly what we should do is synchronously run all pending object toggle ref notifiers.

To do that we'd need to keep a list of them.

Created attachment 240854 [details] [review]
0001-Synchronously-process-all-idle-object-toggle-referen.patch

So let me make sure I understand the problem. When a JSContext is destroyed it's global JSObject gets finalized and then all JSObjects rooted to that JSObject get reaped.  We make the assumption that the global JSObject will always be around to keep the JSObjects rooted until their toggle notify idles get dispatched.  This assumption falls over in a came where th JSContext gets destroyed.  Right?

Review of attachment 240854 [details] [review]:

man too bad bug 690126 isn't fixed...

::: gi/object.c
@@ +855,3 @@
 
+    if (source) {
+        g_atomic_int_add(&pending_idle_toggles, -1);

might make more sense to do this when the ToggleRefNotifyOperation is freed.  Since this really a count of pending notify operations.

@@ +961,3 @@
             g_assert_not_reached();
     }
+    g_atomic_int_add(&pending_idle_toggles, -1);

Then you wouldn't need to do it in two places.

@@ +1024,3 @@
     g_object_set_qdata (gobj, qdata_key, source);
     g_source_attach (source, NULL);
+    g_atomic_int_inc(&pending_idle_toggles);

likewise, maybe do this a few lines higher where the operation is created.

@@ +1116,3 @@
+           g_atomic_int_get (&pending_idle_toggles) > 0) {
+        g_main_context_iteration (NULL, FALSE);
+    }

hmm, one thought... If we're going to be keeping a global count of pending operations anyway, maybe we should just go a little further and keep a global async queue of the operations themselves.  Then we could have one idle instead of N idles, and wouldn't need to store the idle source as qdata on each object.

One downside of that idea is it would be harder to know when it's safe to invoke toggle notifies right away, i guess.

Created attachment 240889 [details] [review]
updated patch

Yeah, refcounting in the destroy is cleaner.

(In reply to comment #5)

> One downside of [the global queue] is it would be harder to know when it's safe to
> invoke toggle notifies right away, i guess.

Yeah, it's not clear to me it'd be worth it; having to rescan the list seems like it has the potential to be quite bad for performance.

Review of attachment 240889 [details] [review]:

::: gi/object.c
@@ +857,2 @@
         g_source_destroy(source);
+    }

Seems unnecessary?

::: gjs/context.c
@@ +394,3 @@
+        JS_GC(js_context->runtime);
+
+        gjs_object_process_pending_toggles();

Why twice?

Review of attachment 240889 [details] [review]:

::: gi/object.c
@@ +1107,3 @@
+/* In the case of a synchronous garbage collection from the main
+ * thread, we need to ensure we've cleared the context of any pending
+ * toggle references.

I think I would make it clear this is for the shutdown path specifically.

maybe something like,
"When the JSContext is disposed we need to synchronously finalize all JSObjects. Before a JSObject is finalized all of its pending toggle notifications need to be processed."

HI,

> Yeah, it's not clear to me it'd be worth it; having to rescan the list seems
> like it has the potential to be quite bad for performance.

So one idea would be to have a hybrid approach of the global queue, and "has-toggle-up-pending"/"has-toggle-down-pending" object data.

Anyway that optimization is sort of separate from this particular bug, so we can just leave it for another day.

Thanks, pushed with those fixes.

*** Bug 724976 has been marked as a duplicate of this bug. ***