Closed
Bug 664519
Opened 13 years ago
Closed 13 years ago
Auto-remove iQ event listeners
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 7
People
(Reporter: dao, Assigned: dao)
References
Details
(Keywords: memory-leak)
Attachments
(1 file, 2 obsolete files)
2.68 KB,
patch
|
iangilman
:
review+
|
Details | Diff | Splinter Review |
The tabview code seems to generally not remove ("unbind") event listeners added through the iQ API (foo.click(...) etc.). Since nodes can hold references to other stuff (e.g. tabs), this can unnecessarily keep things alive. The attached patch lets iQ automatically remove all listeners when removing and emptying nodes.
Attachment #539601 -
Flags: feedback?(tim.taubert)
Assignee | ||
Comment 1•13 years ago
|
||
try run: http://tbpl.mozilla.org/?tree=Try&rev=abce48f51252
Assignee | ||
Comment 2•13 years ago
|
||
In particular, take a look at how search.js handles tabs from other browser windows: onOther: function(tab, index){ let item = iQ("<div/>") .addClass("inlineMatch") .click(function(event){ hideSearch(event); TabUtils.focus(tab); }); ... item.appendTo("#results"); Later: // Remove any previous other-window search results and // hide the display area. iQ("#results").empty();
Comment 3•13 years ago
|
||
Comment on attachment 539601 [details] [diff] [review] patch Review of attachment 539601 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabview/iq.js @@ +336,5 @@ > // ---------- > // Function: remove > // Removes the receiver from the DOM. > remove: function iQClass_remove() { > + this.unbindAll(); Unbinding all event listeners when calling $element.remove() is a bit unexpected. We might remove the element from the DOM just to re-insert it in another place, as we do when sorting app tab icons. https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#1204 Here we would lose our app tab icon's onclick event handler. That is the only place I could find so far... maybe we could add an option like "dontUnbind"? @@ +353,5 @@ > empty: function iQClass_empty() { > for (let i = 0; this[i] != null; i++) { > let elem = this[i]; > while (elem.firstChild) { > + iQ(elem.firstChild).unbindAll(); This seems fair because I don't expect anyone to re-insert those elements in another place when emptying an element.
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to comment #3) > Unbinding all event listeners when calling $element.remove() is a bit > unexpected. We might remove the element from the DOM just to re-insert it in > another place, as we do when sorting app tab icons. > > https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/ > groupitems.js#1204 > > Here we would lose our app tab icon's onclick event handler. That is the > only place I could find so far... maybe we could add an option like > "dontUnbind"? Yep, while I was aware that such a case could exist in theory, I didn't think it existed in actual tabview code. Will add the parameter.
Assignee | ||
Comment 5•13 years ago
|
||
I didn't like the triple negation in !dontUnbind, so I came up with a different name...
Attachment #539601 -
Attachment is obsolete: true
Attachment #539601 -
Flags: feedback?(tim.taubert)
Attachment #539765 -
Flags: feedback?(tim.taubert)
Comment 6•13 years ago
|
||
Comment on attachment 539765 [details] [diff] [review] patch v2 Review of attachment 539765 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabview/groupitems.js @@ +1200,5 @@ > return true; > > let targetIndex = xulTab._tPos; > > + $icon.remove(true); $icon.remove({preserveEventHandlers: true}) is more explicit and would save me a look up to find out what this parameter does. It's easier to add some more options in the future and also more consistent with the existing Panorama code.
Attachment #539765 -
Flags: feedback?(tim.taubert) → feedback+
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #539765 -
Attachment is obsolete: true
Attachment #539775 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #539775 -
Flags: review?(gavin.sharp) → review?(ian)
Comment 8•13 years ago
|
||
Comment on attachment 539775 [details] [diff] [review] patch v3 Review of attachment 539775 [details] [diff] [review]: ----------------------------------------------------------------- Good catch, Dao... thank you for taking care of this. ::: browser/base/content/tabview/iq.js @@ +758,5 @@ > + unbindAll: function iQClass_unbindAll() { > + for (let i = 0; this[i] != null; i++) { > + let elem = this[i]; > + > + for (let i = 0; i < elem.childElementCount; i++) I guess it's perfectly legal to nest two loop variables with the same name, but I think it's better practice to give them different names (i, j or a, b, for instance); it makes the code clearer to read, and it's ready should future code changes require the inner loop access to the outer loop variable.
Attachment #539775 -
Flags: review?(ian) → review+
Comment 9•13 years ago
|
||
I'm slightly concerned about the recursive behavior of removeAll(). Can we just rely on the fact that it won't get called for elements with deep children subtrees?
Assignee | ||
Comment 10•13 years ago
|
||
Yes, I think we can count on reaching no worrisome depth.
Assignee | ||
Comment 11•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/02f8dbc08005
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Comment 12•13 years ago
|
||
How common a leak was this, outside of automated tests? (In combination with bug 664672?)
Assignee | ||
Comment 13•13 years ago
|
||
The case in comment 2 would occur whenever you search across multiple windows in Panorama.
Comment 14•13 years ago
|
||
And how big were the leaks? Comment 0 makes it sound like it could leak whole tabs? Is Panorama the only user of iQ?
Comment 15•13 years ago
|
||
Yes, and yes.
Comment 16•13 years ago
|
||
(In reply to comment #0) > Since nodes can hold references to other stuff (e.g. tabs), this can > unnecessarily keep things alive. But only if something holds a reference to the node, no? In the example from comment 2, what's the root that keeps the node (and thus the tab) alive?
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•