GNOME Bugzilla – Bug 640781
memory leaks
Last modified: 2011-03-21 21:40:09 UTC
Created attachment 179477 [details] [review] [1/4]remove st_container_remove_all & rewrite st_container_destroy_children 1. they leak list's nodes 2. st_container_remove_all leak reference to first_child and last_child 3. st_container_remove_all is useless
Created attachment 179478 [details] [review] [2/4]workspace: disconnect from 'size-changed' in _onDestroy MutterWindow has greater live time than Workspace
Created attachment 179479 [details] [review] [3/4] workspacesView: remove duplicate connection to same signal
Created attachment 179480 [details] [review] [4/4] workspacesView: diconnect signal from _scrollAdjustment in _onDestroy
Review of attachment 179479 [details] [review]: I have his embedded in other patches on the workspace-thumbnails branch, but it's fine to land patch and I'll merge up.
Review of attachment 179480 [details] [review]: ::: js/ui/workspacesView.js @@ +478,2 @@ _onDestroy: function() { + this._scrollAdjustment.disconnect(this._scrollAdjustmentNotifyValueId); I'd rather you did: this._scrollAdjustment.run_dispose() In general, it's best if every GObject is disposed at teardown (disposal automatically disconnects all signals)
Review of attachment 179478 [details] [review]: Putting a property onto clone from outside like this is ugly. Can you make WindowClone forward the size-changed event to a Signals.js signal and disconnect from that? We shouldn't need to disconnect from the signals.js signal because signals.js are transparent to the Javascript garbage collector and won't cause leaks.
Review of attachment 179477 [details] [review]: ::: js/ui/dash.js @@ +207,3 @@ _redisplay: function () { this._box.hide(); + this._box.destroy_children(); Looks fine, but to me seems to point out a bug in the code - it looks like if redisplay() occurred during a drag (which is possible), we'd get in a confused state with regards to this._dragPlaceholder and this._favRemoveTarget.actor. Maybe Florian can double check my reading. ::: js/ui/modalDialog.js @@ +85,2 @@ setButtons: function(buttons) { + this._buttonLayout.destroy_children(); Looks fine, but another bug here that I'd appreciate if you could file is that actionKeys isn't cleared. ::: src/st/st-container.c @@ -96,3 @@ - * @container: An #StContainer - * - * Removes all child actors from @container. Hmm, radical, but yeah, it's' almost always wrong, and if someone really needs it they can just code it. @@ +105,1 @@ + container->priv->children = NULL; Don't like this, This puts the container into an inconsistent state - where Clutter thinks that the children are children but we don't. I think it's bad if the: priv->children = g_list_remove (priv->children, actor); path in st_container_remove() is a no-op. If we are destroying the children front to back in the list, then the removes are fast. Just remove the: priv->children = priv->children->next; line from the existing code and it will be correct.
Oh, and just wanted to say good work tracking these leaks down!
Created attachment 179484 [details] [review] workspacesView: dispose _scrollAdjustment in _onDestroy
Created attachment 179485 [details] [review] workspace: make WindowClone forward the 'size-changed' event
Review of attachment 179484 [details] [review]: Yep
Review of attachment 179485 [details] [review]: ::: js/ui/workspace.js @@ +106,3 @@ + this.realWindow.connect('size-changed', Lang.bind(this, function() { + this.emit('size-changed'); But you still need to disconnect this, right?
Created attachment 179660 [details] [review] workspace: make WindowClone forward the 'size-changed' event
Created attachment 179661 [details] [review] modalDialog: remove old actions from _actionKeys in setButtons
Created attachment 179663 [details] [review] dnd: If needed, destroy _dragActor in _cancelDrag > it looks like if > redisplay() occurred during a drag (which is possible), we'd get in a confused > state with regards to this._dragPlaceholder and this._favRemoveTarget.actor. fixed by this patch
Created attachment 179664 [details] [review] remove st_container_remove_all & rewrite st_container_destroy_children
Review of attachment 179664 [details] [review]: Looks good except for a few tweaks to the commit message * Capital letter to start the subject, always * Then the rest I'd write as: ===== 1. Both functions leaked the nodes in priv->children 2. st_container_remove_all wasn't properly updating first_child and last_child 3. remove_all() is almost never right since it won't cause signal handlers on the children to be removed. In the rare cases where it might be needed the caller can simply use clutter_container_remove(). ===
Review of attachment 179660 [details] [review]: OK
Review of attachment 179661 [details] [review]: OK
Review of attachment 179663 [details] [review]: Doesn't make sense to me - it looks like dragActor will be destroyed (when appropriate) either in _dragActorDropped or in _onAnimationComplete
Created attachment 179754 [details] [review] main: correct pushModal/popModal mechanism 1. disconnect destroy signals in popModal (actors can have great lifetime and then pushed again) 2. incorrect pushModal/popModal pair in overview
(In reply to comment #20) > Doesn't make sense to me - it looks like dragActor will be destroyed (when > appropriate) either in _dragActorDropped or in _onAnimationComplete No. _dragActorDropped or in _onAnimationComplete will not be called (It execute only if draggable.actor destroyed during drag)
Created attachment 179755 [details] [review] Remove st_container_remove_all & rewrite st_container_destroy_children correct commit message
Created attachment 179857 [details] [review] ShellGlobal: don't use JS_MaybeGC spidermonkey doesn't know about object size (Bug 640790) => JS_MaybeGC never start GC.
Review of attachment 179857 [details] [review]: I don't like the idea of doing a full JS garbage collection every time. I've followed up in bug 640790 with what I think is a better approach.
Created attachment 180163 [details] [review] shell_global_maybe_gc: Call gjs_context_maybe_gc instead of JS_MaybeGC This handles native glibc memory allocations better see: https://bugzilla.gnome.org/show_bug.cgi?id=640790 Also bump the minimum gjs version to 0.7.11
Review of attachment 180163 [details] [review]: looks good
Comment on attachment 180163 [details] [review] shell_global_maybe_gc: Call gjs_context_maybe_gc instead of JS_MaybeGC Attachment 180163 [details] pushed as 32d5986 - shell_global_maybe_gc: Call gjs_context_maybe_gc instead of JS_MaybeGC
(In reply to comment #22) > (In reply to comment #20) > > Doesn't make sense to me - it looks like dragActor will be destroyed (when > > appropriate) either in _dragActorDropped or in _onAnimationComplete > > No. _dragActorDropped or in _onAnimationComplete will not be called (It execute > only if draggable.actor destroyed during drag) Unfortunately, I can't quite follow what you are saying here. Can you provide a sequence of events when things go wrong?
(In reply to comment #29) > Unfortunately, I can't quite follow what you are saying here. Can you provide a > sequence of events when things go wrong? Drag actor does not always have to be the same as original actor. If original actor destroyed during drag, _dragActor will not be destoyed.
Created attachment 181776 [details] [review] main: correct pushModal/popModal mechanism merge with HEAD
Created attachment 181777 [details] [review] messageTray: correct destroy notification
Comment on attachment 181776 [details] [review] main: correct pushModal/popModal mechanism >+ let record = modalActorFocusStack[focusIndex]; >+ record.actor.disconnect(record.destroyId); >+ >+ if (focusIndex == modalActorFocusStack.length - 1) { >+ if (record.focus) >+ record.focus.disconnect(record.focusDestroyId); >+ global.stage.set_key_focus(modalActorFocusStack[focusIndex].focus); "modalActorFocusStack[focusIndex].focus" is just "record.focus" isn't it? >+ } else { >+ if (modalActorFocusStack[modalActorFocusStack.length - 1].focus) >+ modalActorFocusStack[modalActorFocusStack.length - 1].focus.disconnect(modalActorFocusStack[modalActorFocusStack.length - 1].focusDestroyId); and that could really use a temporary variable >+ // Remove from the middle, shift the focus chain up >+ for (let i = modalActorFocusStack.length - 1; i > focusIndex; i--) { this looks right, but it's not totally obvious, because you swapped the direction of the loop. any reason for that? >- global.stage.set_key_focus(null); Why did you remove this? This could result in some modal thingies continuing to get keyboard events they weren't expecting after they popModal. >- if (Main.pushModal(this.dash.actor)) >+ if (Main.pushModal(this.viewSelector.actor)) as of a few hours ago this is out of date; you should be pushing/popping this._group now, not this.viewSelector.actor.
Comment on attachment 181777 [details] [review] messageTray: correct destroy notification >Subject: [PATCH 2/2] messageTray: correct destroy notification why? what was wrong with it?
(In reply to comment #34) > why? what was wrong with it? Notification is leak. Apply follow patch: --- a/js/ui/messageTray.js +++ b/js/ui/messageTray.js @@ -412,6 +412,7 @@ Notification.prototype = { this.actor = new St.Table({ name: 'notification', reactive: true }); + this.actor.connect('destroy', function (){print("destroy");}); this.actor.connect('style-changed', Lang.bind(this, this._styleChanged)); this.actor.connect('button-release-event', Lang.bind(this, function (actor, event) { And try before and after my patch:)
Comment on attachment 181777 [details] [review] messageTray: correct destroy notification >Subject: [PATCH 2/2] messageTray: correct destroy notification ah... I parsed that as being about a problem with notification of destruction, whereas you meant a problem with destruction of notifications. :-} How about "messageTray: don't leak Notifications" >@@ -801,6 +803,7 @@ Notification.prototype = { > if (!reason) > reason = NotificationDestroyedReason.DISMISSED; > this.emit('destroy', reason); >+ this.actor.destroy(); A long long time ago, we agreed that the proper style for this was to have the JS object's destroy() method just call "this.actor.destroy()", and for all the actual cleanup to happen in an _onDestroy() handler. (That way if the actor gets destroyed for some other reason, it still does the right thing.) So if we're going to fix this, let's do it like that. > let index = this._notificationQueue.indexOf(notification); >+ notification.destroy(); > if (index != -1) > this._notificationQueue.splice(index, 1); It looks like removeNotification() is only ever called as a notification 'destroy' signal handler. So this change should be a no-op. (And we should probably rename removeNotification to _onNotificationDestroyed.)
Created attachment 181875 [details] [review] main: correct pushModal/popModal mechanism > "modalActorFocusStack[focusIndex].focus" is just "record.focus" isn't it? right > and that could really use a temporary variable done > as of a few hours ago this is out of date; you should be pushing/popping > this._group now, not this.viewSelector.actor. updated > Why did you remove this? This could result in some modal thingies continuing to > get keyboard events they weren't expecting after they popModal. It is useless(and incorrect). I store previously focused actor(which can be not null) in .focus. > this looks right, but it's not totally obvious, because you swapped the > direction of the loop. any reason for that? yes, original is incorrect, Since modalActorFocusStack[i][1] will be assigned to elements from i + 1 to end of the array.
Created attachment 181879 [details] [review] messageTray: don't leak Notifications
Created attachment 181904 [details] [review] WindowAttentionHandler: Fix notify callback leak Keep track of the connected handlers and disconnect them on when destroying the source.
Review of attachment 181904 [details] [review]: ::: js/ui/windowAttentionHandler.js @@ +49,3 @@ + _onSourceDestroyed: function(source) { + for (let i = 0; i < source.__windowSignalIDs.length; i++) { + Mainloop.source_remove(source.__windowSignalIDs[i]); should be source._window.disconnect(source.__windowSignalIDs[i]) @@ +87,3 @@ + source.__windowSignalIDs.push(window.connect('notify::demands-attention', Lang.bind(this, function() { source.destroy(); }))); + source.__windowSignalIDs.push(window.connect('focus', Lang.bind(this, function() { source.destroy(); }))); + source.__windowSignalIDs.push(window.connect('unmanaged', Lang.bind(this, function() { source.destroy(); }))); It is better to move this to Source._init
Comment on attachment 181879 [details] [review] messageTray: don't leak Notifications good to go, except: > source.connect('destroy', Lang.bind(this, > function () { >- this.removeSource(source); >+ this._onSourceDestroy(source); > })); source.connect('destroy', Lang.bind(this, this._onSourceDestroy));
Created attachment 182366 [details] [review] WindowAttentionHandler: Fix notify callback leak Keep track of the connected handlers and disconnect them on when destroying the source.
Created attachment 182386 [details] [review] WindowAttentionHandler: Fix notify callback leak Keep track of the connected handlers and disconnect them on when destroying the source. --- Don't import Mainloop
Review of attachment 182386 [details] [review]: looks good
Created attachment 182434 [details] [review] messageTray: correct _expandedSummaryItem in _onSourceDestroy
(In reply to comment #45) > Created an attachment (id=182434) [details] [review] > messageTray: correct _expandedSummaryItem in _onSourceDestroy It fix follow problem: JS ERROR: !!! Exception was: TypeError: this._sourceTitle.clutter_text is null JS ERROR: !!! lineNumber = '943' JS ERROR: !!! fileName = '/home/zaspire/gnome-shell/source/gnome-shell/js/ui/messageTray.js' JS ERROR: !!! stack = '(0)@/home/zaspire/gnome-shell/source/gnome-shell/js/ui/messageTray.js:943 ([object Object])@/home/zaspire/gnome-shell/source/gnome-shell/js/ui/messageTray.js:1284 ([object Object])@/home/zaspire/gnome-shell/source/gnome-shell/js/ui/messageTray.js:1267 ([object _private_St_Button],[object _private_GLib_ParamSpec])@/home/zaspire/gnome-shell/source/gnome-shell/js/ui/messageTray.js:1138 ([object _private_St_Button],[object _private_GLib_ParamSpec])@/home/zaspire/gnome-shell/install/share/gjs-1.0/lang.js:110
Review of attachment 182434 [details] [review]: Separately fixed: commit 4282748483d2a74fbad90dbe15035224531a6577 Author: Marina Zhurakhinskaya <marinaz@redhat.com> Date: Sun Mar 6 16:29:11 2011 -0500 Fix removing summary items
Created attachment 184009 [details] [review] dnd: If needed, destroy _dragActor in _cancelDrag I'm going to push this slightly different version. The difference is that it replaces + if (this._dragActor != this.actor) With: + if (!this._dragOrigParent) So it's more clearly like this._dragAnimationComplete. If this._dragOrigParent is not null, then we know this._dragActor == this.actor. If this._dragOrigParent is null, then it's just barely conceivable that this.actor._delegate.getDragActor() returned the actor itself, but calling destroy() on an actor twice is safe.
Attachment 184009 [details] pushed as d38f41a - dnd: If needed, destroy _dragActor in _cancelDrag