GNOME Bugzilla – Bug 693676
test GObject Class failure
Last modified: 2017-04-01 20:14:01 UTC
gjs make test is leaking 2 closures in 'testDerived' of testGObjectclass. JS LOG: running test testDerived JS CTX: Script evaluation succeeded JS CTX: Script returned integer code 0 JS MEMORY: Memory report: before destroying context JS MEMORY: 116 objects currently alive JS MEMORY: boxed = 4 JS MEMORY: gerror = 0 JS MEMORY: closure = 9 JS MEMORY: database = 0 JS MEMORY: function = 71 JS MEMORY: importer = 2 JS MEMORY: ns = 4 JS MEMORY: object = 19 JS MEMORY: param = 5 JS MEMORY: repo = 1 JS MEMORY: resultset = 0 JS MEMORY: weakhash = 0 JS MEMORY: interface = 1 JS CTX: Destroying JS context JS MEMORY: Memory report: after destroying context JS MEMORY: 2 objects currently alive JS MEMORY: boxed = 0 JS MEMORY: gerror = 0 JS MEMORY: closure = 2 JS MEMORY: database = 0 JS MEMORY: function = 0 JS MEMORY: importer = 0 JS MEMORY: ns = 0 JS MEMORY: object = 0 JS MEMORY: param = 0 JS MEMORY: repo = 0 JS MEMORY: resultset = 0 JS MEMORY: weakhash = 0 JS MEMORY: interface = 0
*** Bug 693695 has been marked as a duplicate of this bug. ***
This used to work fine, because the closures are destroyed in the class_finalize function. My bet is some change in glib that broke dynamic types, following the trend of keeping all GTypeModules resident.
Created attachment 235884 [details] [review] closure: Fix reference counting of closures, unbreak tests Glib no longer finalizes type classes, plugin or not. So essentially gjs will 'leak' the classes that it registers in gjs_register_type(). GClosures that have been associated with such leaked classes (eg: through g_signal_override_class_closure()) will leak too. However the javascript context, object and runtime associated with the Closure do not leak. Make our memory reference tracking for closures to track the javascript related contents of the Closure and not the GClosure itself. https://bugzilla.gnome.org/show_bug.cgi?id=693695
Created attachment 235885 [details] [review] object: Register auto generated GType classes as static types Glib no longer allows unloading/finalizing of type classes regardless of whether they're static or plugin. So just use static classes here, as they'll 'leak' anyway.
Created attachment 235887 [details] [review] closure: Fix reference counting of closures, unbreak tests Glib no longer finalizes type classes, plugin or not. So essentially gjs will 'leak' the classes that it registers in gjs_register_type(). GClosures that have been associated with such leaked classes (eg: through g_signal_override_class_closure()) will leak too. However the javascript context, object and runtime associated with the Closure do not leak. Make our memory reference tracking for closures to track the javascript related contents of the Closure and not the GClosure itself.
Review of attachment 235884 [details] [review]: ::: gi/closure.c @@ +245,1 @@ GJS_DEC_COUNTER(closure); Aren't we decrementing the counter twice here?
(In reply to comment #6) > Review of attachment 235884 [details] [review]: > > ::: gi/closure.c > @@ +245,1 @@ > GJS_DEC_COUNTER(closure); > > Aren't we decrementing the counter twice here? No, there are two invalidation paths. See g_closure_add_invalidate_notifier() calls in gjs_closure_new(). This is probably ripe for some cleanup.
Review of attachment 235885 [details] [review]: Ok
Review of attachment 235887 [details] [review]: Oh ugh...
FWIW, on IRC Ryan was suggesting holding off applying the static types patch, since he's not sure if this change in glib is going to 'stick'.
(In reply to comment #10) > FWIW, on IRC Ryan was suggesting holding off applying the static types patch, > since he's not sure if this change in glib is going to 'stick'. Even then, the dynamic type was just a hack, because types were not registered inside the load() virtual, so you could not really unregister a type and register a new one with the same name. So, once the closure count is fixed, we can go back to static types. Or even better, we can stop using GClosures inside our tests, and override the virtual function instead...
(In reply to comment #11) > (In reply to comment #10) > > FWIW, on IRC Ryan was suggesting holding off applying the static types patch, > > since he's not sure if this change in glib is going to 'stick'. > > Even then, the dynamic type was just a hack, because types were not registered > inside the load() virtual, so you could not really unregister a type and > register a new one with the same name. > So, once the closure count is fixed, we can go back to static types. So should I merge both patches? > Or even better, we can stop using GClosures inside our tests, and override the > virtual function instead... The actual cause was providing default implementations of these signals in the testGObjectClass.js file: on_run_last: function() { this._run_last_callback(); }, on_empty: function() { this.empty_called = true; }
Comment on attachment 235887 [details] [review] closure: Fix reference counting of closures, unbreak tests Attachment 235887 [details] pushed as 03ac265 - closure: Fix reference counting of closures, unbreak tests
(In reply to comment #12) > [...] > > The actual cause was providing default implementations of these signals in the > testGObjectClass.js file: > > on_run_last: function() { > this._run_last_callback(); > }, > > on_empty: function() { > this.empty_called = true; > } Oh right, and these signals are created by gjs so they don't have matching vfuncs. Ok, ignore my comment then.
Cosimo and I discussed this and we think the patch is an improvement, regardless of whether the change in GLib went through or not. The type module code could never be unloaded anyway, since it called g_assert_not_reached() in its unload function. This doesn't seem to break anything. It's early in the cycle so if it turns out otherwise, there is plenty of time to revert it. Attachment 235885 [details] pushed as 0fe780c - object: Register auto generated GType classes as static types