Last Comment Bug 664519 - Auto-remove iQ event listeners
: Auto-remove iQ event listeners
Status: RESOLVED FIXED
: mlk
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 7
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks: bc-leaks
  Show dependency treegraph
 
Reported: 2011-06-15 11:35 PDT by Dão Gottwald [:dao]
Modified: 2016-04-12 14:00 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.78 KB, patch)
2011-06-15 11:35 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v2 (2.65 KB, patch)
2011-06-16 03:42 PDT, Dão Gottwald [:dao]
ttaubert: feedback+
Details | Diff | Splinter Review
patch v3 (2.68 KB, patch)
2011-06-16 06:07 PDT, Dão Gottwald [:dao]
ian: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2011-06-15 11:35:32 PDT
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.
Comment 1 Dão Gottwald [:dao] 2011-06-15 11:40:45 PDT
try run: http://tbpl.mozilla.org/?tree=Try&rev=abce48f51252
Comment 2 Dão Gottwald [:dao] 2011-06-15 21:59:12 PDT
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 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-16 02:30:13 PDT
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.
Comment 4 Dão Gottwald [:dao] 2011-06-16 03:21:01 PDT
(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.
Comment 5 Dão Gottwald [:dao] 2011-06-16 03:42:22 PDT
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...
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-16 05:29:22 PDT
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.
Comment 7 Dão Gottwald [:dao] 2011-06-16 06:07:54 PDT
Created attachment 539775 [details] [diff] [review]
patch v3
Comment 8 Ian Gilman [:iangilman] 2011-06-21 09:47:46 PDT
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.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-21 09:51:44 PDT
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?
Comment 10 Dão Gottwald [:dao] 2011-06-21 11:11:43 PDT
Yes, I think we can count on reaching no worrisome depth.
Comment 11 Dão Gottwald [:dao] 2011-06-22 04:59:51 PDT
http://hg.mozilla.org/mozilla-central/rev/02f8dbc08005
Comment 12 Jesse Ruderman 2011-06-26 05:37:22 PDT
How common a leak was this, outside of automated tests?  (In combination with bug 664672?)
Comment 13 Dão Gottwald [:dao] 2011-06-26 05:39:56 PDT
The case in comment 2 would occur whenever you search across multiple windows in Panorama.
Comment 14 Nicholas Nethercote [:njn] 2011-06-26 13:26:20 PDT
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 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-26 13:27:42 PDT
Yes, and yes.
Comment 16 Markus Stange [:mstange] 2011-06-29 02:03:15 PDT
(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?

Note You need to log in before you can comment on or make changes to this bug.