GNOME Bugzilla – Bug 786668
Clean up the idle closure invalidation mess
Last modified: 2017-12-06 17:56:45 UTC
Moving the closure invalidation to an idle function has been the source of almost all the crashes in gnome-shell 3.24. However, we can't just revert it; the old way became invalid with the introduction of the moving garbage collector in SpiderMonkey 38, because you can't stop tracing a GC thing in the middle of a GC; if you run GJS in pre barrier verification mode, it will abort. We have to do something differently. By chance I discovered that JS::ExposeObjectToActiveJS() will mark its argument as reachable for the current GC which is exactly what we need to solve the problem. So we can solve the problem in a simpler way.
Created attachment 358201 [details] [review] arg: Fix memory leaks Caught with Valgrind.
Created attachment 358202 [details] [review] object: Remove unused argument gjs_object_prepare_shutdown() doesn't need to access the JSContext.
Created attachment 358203 [details] [review] Revert freeing closures in idle handler This turned out to cause a lot of problems; it has been responsible for almost all of the crashes in gnome-shell since 1.48. We revert back to the original code modulo a few improvements that have been made along the way. This state at least does not crash in the test suite, although it is definitely not correct since it breaks SpiderMonkey's garbage collector pre barrier verification mode (the reason the change was made in the first place.) That still must be fixed using a different approach. This partially reverts commits [41b78ae], [db3e387], [bace908], [9eb4a2b], [2593d3d], and [334ba96].
Created attachment 358204 [details] [review] closure: Debug message on invalidate signal We log a closure debug message in the other case where a closure is invalidated, so we should do it here as well.
Created attachment 358205 [details] [review] object: Refactor out ConnectData The ConnectData struct is not exactly needed; we can keep a list of GClosure signal closures directly. The 'obj' member was only used in the invalidate notifier, so we can pass that as the notifier callback data instead of the ConnectData struct.
Created attachment 358206 [details] [review] closure: Prevent collection of invalidated closure It's not possible to stop tracing an object in the middle of GC. However, by using JS::ExposeObjectToActiveJS(), it is possible to mark an object as reachable for the duration of one GC. This is exactly what we need for closures, to keep the closure's callable object from disappearing while GC is going on. This requires adding a method, prevent_collection(), to GjsMaybeOwned. This method is only valid in non-rooted mode. It also requires a bit of GC API magic to avoid running afoul of some debug-mode assertions. SpiderMonkey master at least has an official API for one of these bits of magic, which we can switch to in SpiderMonkey 59.
The third and sixth patches really fix the problem; the others are just minor fixes that I ran into while debugging this.
I'd like to try these patches, but I wonder whether these patches apply to 1.46.8, since Arch Linux doesn't have mozjs-52 yet...
I'm working on that and will post them on bug 785657 when they're ready.
Review of attachment 358201 [details] [review]: OK ::: gi/arg.cpp @@ +2291,1 @@ + interface_info = g_type_info_get_interface (param_info); You may want to make coding style (e.g. spaces before parens) consistent while you're at it.
Review of attachment 358202 [details] [review]: OK
Review of attachment 358204 [details] [review]: OK
Review of attachment 358203 [details] [review]: OK
Review of attachment 358205 [details] [review]: Nice
Review of attachment 358206 [details] [review]: Looks good to me.
Attachment 358201 [details] pushed as 98e8f0b - arg: Fix memory leaks Attachment 358202 [details] pushed as 291fa4e - object: Remove unused argument
I'll leave off committing the other ones until the backports have been tested.
It looks like from bug 785657 that this at least does not make the problem any worse, and it makes the existing code much easier to follow. So we'll go ahead and commit this refactor to master while we figure out what to do for 3.24. Attachment 358203 [details] pushed as f69b96d - Revert freeing closures in idle handler Attachment 358204 [details] pushed as 3067713 - closure: Debug message on invalidate signal Attachment 358205 [details] pushed as 3bad9d0 - object: Refactor out ConnectData Attachment 358206 [details] pushed as ee3c9c2 - closure: Prevent collection of invalidated closure
*** Bug 785055 has been marked as a duplicate of this bug. ***