The default bug view has changed. See this FAQ.

Auto-remove iQ event listeners

RESOLVED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: dao, Assigned: dao)

Tracking

({mlk})

Trunk
Firefox 7

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 539601 [details] [diff] [review]
patch

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

6 years ago
try run: http://tbpl.mozilla.org/?tree=Try&rev=abce48f51252
(Assignee)

Comment 2

6 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 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

6 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

6 years ago
Created attachment 539765 [details] [diff] [review]
patch v2

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+
(Assignee)

Comment 7

6 years ago
Created attachment 539775 [details] [diff] [review]
patch v3
Attachment #539765 - Attachment is obsolete: true
Attachment #539775 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 10

6 years ago
Yes, I think we can count on reaching no worrisome depth.
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/mozilla-central/rev/02f8dbc08005
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7

Updated

6 years ago
Keywords: mlk

Comment 12

6 years ago
How common a leak was this, outside of automated tests?  (In combination with bug 664672?)
(Assignee)

Comment 13

6 years ago
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?
Yes, and yes.
(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.