GNOME Bugzilla – Bug 725024
Some GC fixes, improvements and refactors
Last modified: 2017-01-06 07:47:44 UTC
This started as a way to prevent a crash in cases like bug 724798, and ended with me diving deep into libmozjs code and refactoring gjs heavily. Depends on bug 724797 and bug 679688 (in particular, without bug 679688 we log a critical for the weak singleton case, even in the safe case of no expando properties)
Created attachment 270072 [details] [review] Check and prevent reentrancy to the JSAPI during finalization Keep track of whether the runtime is currently doing GC sweeping, and prevent calling JS code at that time. This could happen with dangling signal connections or widgets/actors that are not properly destroyed, and causes an assert failure with debug libmozjs (and a crash otherwise)
Created attachment 270073 [details] [review] object: remove bogus comment and code about GC in a secondary thread handle_toggle_up() is only ever called from the main thread, and that's where GC marking happens. The background GC thread only deals with sweeping, and not even of JS objects!
Created attachment 270074 [details] [review] object toggle notify: do more work without deferring to an idle The only case we really need an idle is when called from a secondary thread (and even there it's arguable the idle is fixing anything). Inside the main thread we're in full control of what the GC is doing, and we know when it's safe and when it's not to touch the JSAPI. Deferring to an idle while the GC is already in the sweeping phase is late, by the time the idle runs the JS object is already dead and we're accessing freed memory.
Created attachment 270075 [details] [review] util: remove unused and semi-bogus API Was removed in the last commit.
Created attachment 270076 [details] [review] Use background finalizing for thread-safe stuff For wrapper objects that hold thread-safe or unique C structures, set the appropriate flag to send the finalizer to the background thread, so that we don't block the mainloop more than necessary.
Created attachment 270077 [details] [review] system: add maybegc Like we have system.gc(), add system.maybegc(), that can be called from paint clock after-paint handlers or idle sources.
Review of attachment 270072 [details] [review]: Very nice. Only one minor thing: ::: gi/function.cpp @@ +190,3 @@ + "widget with ::destroy signals connected, but can also be caused by " + "using the destroy() or dispose() vfuncs. Because it would crash the " + "application, it has been blocked and the JS callback not invoked."); Why not deduplicate this error message with the one from value.cpp?
Review of attachment 270073 [details] [review]: Mmmm.... ::: gi/object.cpp @@ -897,3 @@ - * - * Note it's possible that the garbage collector is running in a secondary - * thread right now. If it is, we need to wait for it to finish, then block Er...remember we may be getting the toggle notification from say the GDBus thread. The GC may be running in the *main* thread. I don't see how you can be sure that we'll never get toggle up from another thread.
Review of attachment 270077 [details] [review]: You can Cosimo are doing some duplicate work here; see my comment to him in https://bugzilla.gnome.org/show_bug.cgi?id=725099 Do we need this patch if his lands? It'd be nice to have this be as automagic as possible.
(In reply to comment #9) > Review of attachment 270077 [details] [review]: > > You can Cosimo are doing some duplicate work here; see my comment to him in > https://bugzilla.gnome.org/show_bug.cgi?id=725099 > > Do we need this patch if his lands? It'd be nice to have this be as automagic > as possible. Yes, it's nice if everything works automagically, but you never know what the app needs, and offering the API leaves the ultimate choice to the app developer. (In reply to comment #8) > Review of attachment 270073 [details] [review]: > > Mmmm.... > > ::: gi/object.cpp > @@ -897,3 @@ > - * > - * Note it's possible that the garbage collector is running in a secondary > - * thread right now. If it is, we need to wait for it to finish, then > block > > Er...remember we may be getting the toggle notification from say the GDBus > thread. The GC may be running in the *main* thread. > > I don't see how you can be sure that we'll never get toggle up from another > thread. Indeed, the patch is losing the thread check. Good catch.
Created attachment 270435 [details] [review] object toggle notify: do more work without deferring to an idle The only case we really need an idle is when called from a secondary thread (and even there it's arguable the idle is fixing anything). Inside the main thread we're in full control of what the GC is doing, and we know when it's safe and when it's not to touch the JSAPI. Deferring to an idle while the GC is already in the sweeping phase is late, by the time the idle runs the JS object is already dead and we're accessing freed memory.
Review of attachment 270076 [details] [review]: I'm OK with this if you are after reading my concerns below: ::: gjs/byteArray.cpp @@ +58,3 @@ JSCLASS_HAS_PRIVATE | + JSCLASS_IMPLEMENTS_BARRIERS | + JSCLASS_BACKGROUND_FINALIZE, The issues here are so subtle...imagine GBytes instances with GDestroyNotify that we'll now be running in a new thread. ::: gjs/mem.cpp @@ +75,3 @@ int i; int n_counters; + int total_objects; Looks like an unrelated bugfix, feel free to push. ::: modules/cairo-surface.cpp @@ +37,3 @@ } GjsCairoSurface; +GJS_DEFINE_PROTO_ABSTRACT_WITH_GTYPE("CairoSurface", cairo_surface, CAIRO_GOBJECT_TYPE_SURFACE, JSCLASS_BACKGROUND_FINALIZE) Are all cairo backends really going to be safe against being created in one thread and destroyed in another?
Review of attachment 270075 [details] [review]: Sure (maybe squash?)
*** Bug 727858 has been marked as a duplicate of this bug. ***
Attachment 270072 [details] pushed as dc31b7c - Check and prevent reentrancy to the JSAPI during finalization
Review of attachment 270435 [details] [review]: I'll be honest, while your explanation is good I can't claim I fully have this in my head. I can't immediately come up with a case where not blocking the GC would break, so let's go with this. ::: gi/object.cpp @@ +1236,3 @@ + + /* Use -1 to mark that a JS object once existed, but it doesn't any more */ + set_js_obj(gobj, (JSObject*)(-1)); Some hesitation at using -1 as a magic pointer, but eh, it's OK.
Attachment 270073 [details] pushed as 54ff90d - object: remove bogus comment and code about GC in a secondary thread Attachment 270075 [details] pushed as 675ce62 - util: remove unused and semi-bogus API Attachment 270076 [details] pushed as ef33fbe - Use background finalizing for thread-safe stuff Attachment 270435 [details] pushed as ae34ec4 - object toggle notify: do more work without deferring to an idle
*** Bug 725109 has been marked as a duplicate of this bug. ***