Just reproduced this 5 minutes ago, with a debug build of libmozjs you get

Assertion failure: !rt->isHeapCollecting(), at /home/giovanni/gnome/js24-24.2.0/js/src/jsapi.cpp:206

because StBin.grid-search-result is being destroyed for the first time, so it destroys its child, causing a 'destroy' signal that calls back into JS code. And that's not allowed during GC sweep.

Created attachment 270079 [details] [review]
search: destroy result actors before forgetting about them

We can't let live (ie, never destroyed) actors undergo GC, because
they will emit :destroy signals during finalization and assert/crash
libmozjs. Properly destroy all actors before letting the GC
free them.

Review of attachment 270079 [details] [review]:

This seems like a super terrible bug. We can't just let GC crash the Shell and hack around it everywhere. Plenty of extensions will likely crash the Shell as well. I know you have a gjs bug open for this.

We need to dispose GObjects in the main thread. Ryan has long wanted a "GCleanup" that would make it safe to do g_object_unref from any thread and have it get disposed on the thread the object was created in. It might be possible to do something like that here: through toggle refs, we know if we're holding onto the last reference of an object, so we know if the object will be finalized in the main thread or not.

We should queue finalization of objects to be handled in the main thread, since we're quite certain that it was created it in the main thread, at least until we get GCleanup.

::: js/ui/search.js
@@ +337,3 @@
     _ensureResultActors: function(results, callback) {
         let metasNeeded = results.filter(Lang.bind(this, function(resultId) {
+            return !(resultId in this._resultDisplays);

No, the code before should work fine.

(In reply to comment #3)
> Review of attachment 270079 [details] [review]:
> 
> This seems like a super terrible bug. We can't just let GC crash the Shell and
> hack around it everywhere. Plenty of extensions will likely crash the Shell as
> well. I know you have a gjs bug open for this.
> 
> We need to dispose GObjects in the main thread. Ryan has long wanted a
> "GCleanup" that would make it safe to do g_object_unref from any thread and
> have it get disposed on the thread the object was created in. It might be
> possible to do something like that here: through toggle refs, we know if we're
> holding onto the last reference of an object, so we know if the object will be
> finalized in the main thread or not.
> 
> We should queue finalization of objects to be handled in the main thread, since
> we're quite certain that it was created it in the main thread, at least until
> we get GCleanup.

You didn't read the gjs bug, did you? (Or the stack trace, fwiw)
This is totally running off the main thread. It's deep inside a JS_GC call.
It's the only proper place to finalize GObject stuff, and unless you insist with JSCLASS flags, all JS object finalization happens in the main thread.
You can queue all that you want, the JS object is dead and you get a crash when you invoke the queued idle next time. Or the JS function handling the signal is dead, and you still get a crash. Or you get a new proxy, and the JS function, now bound to something completely different than it expects, fails to do anything useful because the signal IDs set on the expando properties are undefined.

> ::: js/ui/search.js
> @@ +337,3 @@
>      _ensureResultActors: function(results, callback) {
>          let metasNeeded = results.filter(Lang.bind(this, function(resultId) {
> +            return !(resultId in this._resultDisplays);
> 
> No, the code before should work fine.

It's a little different in case of non-enumerable properties (say, a nice search provider returning "watch" as a result id), that's why I changed it to match the for..in in clear()

(In reply to comment #4)
> (In reply to comment #3)
> > Review of attachment 270079 [details] [review] [details]:
> 
> You didn't read the gjs bug, did you? (Or the stack trace, fwiw)

Ops, I just noticed the original stack trace does not show the bug is not threading related (because without JS_DEBUG, the bug manifests as a crash when the resurrected object is double-freed in the background thread)

With a "simple" test case such as:
#!/usr/bin/env gjs

const GLib = imports.gi.GLib;
const GObject = imports.gi.GObject;
const Gtk = imports.gi.Gtk;
const System = imports.system;

function test() {
    let w = new Gtk.Button();
    w._foo = true;
    w.connect('destroy', function() {
        log('destroy invoked');
        log('w is ' + w);
    });

    let c = new Gtk.Box();
    c.add(w);
}

function main() {
    Gtk.init(null);

    // Create an object
    // Make sure the object uses toggle references
    // Create a reference cycle
    // Install a callback that will be invoked
    // when the object is finalized
    // Get the object out of scope
    // Run the mainloop for a while, to make sure gjs
    // deferred keep-alive callbacks are run
    // Force a GC
    //
    // The test passes if there is a critical telling
    // you that the JS callback is not invoked;
    // it fails if gjs aborts in libmozjs or if
    // the callback is invoked

    test();

    GLib.timeout_add(GLib.PRIORITY_DEFAULT, 500, Gtk.main_quit);
    Gtk.main();

    System.gc();
    return 0;
}
main();

you get a more clear picture:

+ Trace 233228

(In reply to comment #4)

Ah, thanks for the explanation. I guess I got this confused with another finalization bug. It seems the only thing we can truly do in this case is to prevent objects with signals from being finalized, which is extremely poor. Or we can just prevent signals from being emitted after being finalized, which is also poor. Hm.

> It's a little different in case of non-enumerable properties (say, a nice
> search provider returning "watch" as a result id), that's why I changed it to
> match the for..in in clear()

js> "watch" in {}
true

The "in" operator doesn't care about enumerability at all. You probably want foo.hasOwnProperty("watch");. But that's a separate fix.

Pushed after Jasper acknowledged it was a real bug in the last comment,
with the problematic part (in vs === undefined) removed.