GNOME Bugzilla – Bug 775374
ShellGlobal singleton should be destroyed before gnome-shell exits
Last modified: 2016-12-12 23:04:56 UTC
I'm testing out gnome-shell with the "mozjs31" branch of GJS. With SpiderMonkey 31, you need to destroy all your JSContexts before destroying the JSRuntime, and you need to destroy your JSRuntime before calling JS_ShutDown(), and it's recommended to call JS_ShutDown() before exiting. GJS will now take care of all of this for you, except that you need to call g_object_unref() on any GjsContext before exiting. (Otherwise you fail an assert upon exit such as this:) JS API usage error: found live context at 0x2820cd0 JS API usage error: 1 context left in runtime upon JS_DestroyRuntime. Assertion failure: m_pools.empty(), at /home/philip/gnome/checkout/mozjs-31.5.0/js/src/assembler/jit/ExecutableAllocator.h:231 Segmentation fault (core dumped) I'm attaching a patch that cleans up the GjsContexts in the extension preferences and the portal helper, but I don't know how to handle the one in ShellGlobal. Is there any time we can explicitly destroy the ShellGlobal singleton before gnome-shell exits?
Created attachment 341009 [details] [review] helpers: Destroy GjsContext before exit This will be required in the upcoming version of GJS.
Unreferencing a GObject before exit is not harmful at all even for now. It looks reasonable.
Created attachment 341443 [details] [review] main: Destroy GjsContext before exit This will be required in the upcoming version of GJS. The reference count on ShellGlobal is 2 at this point, because JS holds a reference due to the "window.global = Shell.Global.get()" line in ui/environment.js. Therefore, destroy the GjsContext first, then unref the ShellGlobal object. Cleaning up ShellGlobal was previously only enabled behind a debug environment variable, but it should be required now.
This second patch prevents the segfault for me. Not sure if this is the only point at which we should destroy ShellGlobal, but maybe a reviewer can tell?
Review of attachment 341009 [details] [review]: This is clearly correct.
Review of attachment 341443 [details] [review]: Looks good to me, and it would be great to get it in this week, so that we can release a new mozjs31 GJS Monday. Florian or others, any opinions on this?
The code which refers to window.global is removed in the patch (environment: move more init stuff here from main.js, commit 7921954a3187ae). What about just remove "window.global = Shell.Global.get()" and apply the first patch only?
(In reply to Hyungwon Hwang from comment #7) > What about just remove "window.global = Shell.Global.get()" and apply the > first patch only? No, that would result in massive code breakage. "window" here isn't a random object, but a weird way to define global properties. For accessing those properties, the "window" prefix is redundant, which is why it is omitted in the code. But that doesn't mean that "window.global" is unused, as you can easily confirm if you grep for something like "\<global\."
Review of attachment 341443 [details] [review]: ::: src/shell-global.c @@ +573,3 @@ + * Destroys the GjsContext held by ShellGlobal, in order to break reference + * counting cycles. (The GjsContext holds a reference to ShellGlobal because + * it's available as window.global inside JS.) An alternative would be to avoid the reference cycle in the first place and use a separate singleton for the GjsContext ("the_context"). But then having to explain how code is split in an odd way to avoid a reference cycle is probably not better than explaining the existence of a reference cycle and how to break it, so fine by me ...
OK, thanks!
Attachment 341009 [details] pushed as 56b20ef - helpers: Destroy GjsContext before exit Attachment 341443 [details] pushed as 83005e2 - main: Destroy GjsContext before exit
Created attachment 341702 [details] [review] tests: Destroy GjsContext before exit This will be required in the upcoming version of GJS.
Found another place where this is needed!
Review of attachment 341702 [details] [review]: Whoops feel free to push!
Comment on attachment 341702 [details] [review] tests: Destroy GjsContext before exit Pushed.