Closed Bug 664519 Opened 13 years ago Closed 13 years ago

Auto-remove iQ event listeners

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 7

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: memory-leak)

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — 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)
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 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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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 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+
Attached patch patch v3Splinter Review
Attachment #539765 - Attachment is obsolete: true
Attachment #539775 - Flags: review?(gavin.sharp)
Attachment #539775 - Flags: review?(gavin.sharp) → review?(ian)
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+
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?
Yes, I think we can count on reaching no worrisome depth.
http://hg.mozilla.org/mozilla-central/rev/02f8dbc08005
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Keywords: mlk
How common a leak was this, outside of automated tests?  (In combination with bug 664672?)
The case in comment 2 would occur whenever you search across multiple windows in Panorama.
And how big were the leaks?  Comment 0 makes it sound like it could leak whole tabs?  Is Panorama the only user of iQ?
(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?
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: