I was wondering why cycle collection graph has been getting bigger, and looks like we leak some findbar related elements. (there are possibly some other leaked chrome elements too). I believe this is a regression. about:cc (Bug 726346) gave me for example the following elements: 0x7f0dfef9e820 [rc=2] root FragmentOrElement (XUL) label class='findbar-find-fast' chrome://browser/content/browser.xul 0x7f0debcd99d0 [rc=2] root FragmentOrElement (XUL) toolbarbutton class='findbar-highlight tabbable' chrome://browser/content/browser.xul 0x7f0df3a15820 [rc=2] root FragmentOrElement (XUL) label class='findbar-find-fast' chrome://browser/content/browser.xul ... ...

Looks like pretty much any findbar usage leaks ~10 elements.

Bug 537013 looks suspicious.

Can you please provide an STR? I can't reproduce any findbar leaks with about:cc.

STR: (1) load about:cc in a tab. (2) Run cycle collector for few times so that the number of objects in the graph stabilize to 200-300. (3) Search for objects containing name FragmentOrElement ("Find objects containing (name or address)") You should see perhaps just two elements, which are coming from about:cc (4) open a new tab and load http://www.yle.fi/ to it (5) press ctrl+f (6) search for mozilla (7) close the tab by clicking the [x] (8) run cycle collector in about:cc tab, you may repeat this few times (9) search for FragmentOrElement, result should be stuff like 0x7fd021647b80 [rc=2] root FragmentOrElement (XUL) label class='findbar-find-fast' chrome://browser/content/browser.xul

Thanks Olli, I can reproduce it. Some additions: (4) the new tab page (about:newtab) works as well. (6) can be skipped. You don't actually need to search for something. Just Pressing Ctrl+F is enough. (7) is important. If you close the tab via Ctrl+W nothing is leaked. Clicking the X with the mouse seems crucial.

Backing out bug 537013 helps with the findbar leaks. Looks like we're still leaking other stuff from the tab itself though: 0x7f0a81a06d30 [rc=2] root FragmentOrElement (XUL) label class='tab-text tab-label' chrome://browser/content/browser.xul 0x7f0a81a069d0 [rc=2] root FragmentOrElement (XUL) image class='tab-icon-image' chrome://browser/content/browser.xul 0x7f0a81a05e00 [rc=2] root FragmentOrElement (XUL) image class='tab-throbber' chrome://browser/content/browser.xul

The fact that we need to hover and close the tab to make it leak makes me suspect bug 585558.

Ok, it doesn't seem to be caused by bug 585558 either. The STR is basically as simple as: (1)-(3) from comment #4 (4) open a newtab (5) close it by clicking the [x] And we'll get the leak signature from comment #6. I don't think this is a leak caused by the findbar. The findbar is just leaked because we seem to leak the owning tab.

Ah, could be. I did see some elements which I thought were tab related in the graph.

The leak seems to be caused by code belonging to the close button's double-click behavior which I know nothing about: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#4335 If I remove the tabContainer.addEventListener() call, the leak disappears.

Dão, can you please take a look at this? The blame log suggests you might have worked on the close button's double click code before.

Proposed patch for dealing with the leak. about:cc does not show the leak with this patch. This reuses the existing click handler from tabbrowser-tabs. The protection is still enabled from the tab-close-button click event handler (via the _blockDblClick field). Because there's no closure involved, there's no leak possible.

review ping

...and review ping. In my current FF session this ends up adding about 10-15ms to every cycle collection.

Comment on attachment 782864 [details] [diff] [review] Handle errant clicks from tabbrowser-tabs handler >@@ -4326,39 +4345,19 @@ > this._ignoredClick = true; > return; > } > > // Reset the "ignored click" flag > this._ignoredClick = false; > > tabContainer.tabbrowser.removeTab(bindingParent, {animate: true, byMouse: true}); >+ // This enables double-click protection for the tab container >+ // (see tabbrowser-tabs 'click' handler). > tabContainer._blockDblClick = true; Spreading the _ignoredClick code across two bindings seems suboptimal. Can the remainder be moved to the tabbrowser-tabs binding as well? >- /* XXXmano hack (see bug 343628): >- * Since we're removing the event target, if the user >- * double-clicks this button, the dblclick event will be dispatched >- * with the tabbar as its event target (and explicit/originalTarget), >- * which treats that as a mouse gesture for opening a new tab. >- * In this context, we're manually blocking the dblclick event >- * (see dblclick handler). >- */ >- var clickedOnce = false; >- function enableDblClick(event) { >- var target = event.originalTarget; >- if (target.className == 'tab-close-button') >- target._ignoredClick = true; >- if (!clickedOnce) { >- clickedOnce = true; >- return; >- } >- tabContainer._blockDblClick = false; >- tabContainer.removeEventListener("click", enableDblClick, true); >- } >- tabContainer.addEventListener("click", enableDblClick, true); It seems like this event listener would eventually be removed when clicking two times on the tab bar. Is this not the case? Is the tab kept alive forever and, if so, why?

> Spreading the _ignoredClick code across two bindings > seems suboptimal. Can the remainder be moved to the > tabbrowser-tabs binding as well? This moves it and the other moved code to a separate handler. This is so it runs before the close button handler/during the capturing phase. > It seems like this event listener would eventually be > removed when clicking two times on the tab bar. Is > this not the case? Is the tab kept alive forever and, > if so, why? You are correct, it is removed if it is triggered. The leak is only when (and as long as) it doesn't get triggered.

Any chance to get this fixed. This kind of leak is really bad for responsiveness.

I will get to this sometime today.

Mano: review ping?

Comment on attachment 799271 [details] [diff] [review] Separate handler out and move remaining ignoreClick code ># HG changeset patch ># User Adam Dane [:hobophobe] <unusualtears@gmail.com> ># Date 1378270059 18000 ># Tue Sep 03 23:47:39 2013 -0500 ># Node ID 440ff099f2395aaa4ab540ce7d85413033297eb4 ># Parent 47baeadc005b7868e4e387b9656d3f9d19d1e6c2 >Bug 897751 - Tab close button's DoubleClick protection handler leaks > >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >--- a/browser/base/content/tabbrowser.xml >+++ b/browser/base/content/tabbrowser.xml >@@ -4041,16 +4041,66 @@ > > // See hack note in the tabbrowser-close-tab-button binding > if (!this._blockDblClick) > BrowserOpenTab(); > > event.preventDefault(); > ]]></handler> > >+ <handler event="click" button="0" phase="capturing"><![CDATA[ Please add a comment here, referencing this bug, that explains why don't we just do it in the close-button binding. >+ let target = event.originalTarget; >+ if (target.classList.contains('tab-close-button')) { Just compare the className. >+ target._ignoredClick = true; Please rename this so that it reflects the fact the click is only ignored for the close button (maybe _ignoreCloseButtonClicks).

Thanks Mano! Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7018bb915589

_clickedOnce should also be renamed.

Actually, thinking about this a little bit more, wouldn't it make sense to set these fields on the close-tab-button element rather than on that tab? As it is, I could imagine this becoming unexpectedly buggy when tabs are switched (I'm saying "imagine," because I cannot think of concrete use cases)

(In reply to Mano from comment #23) > Actually, thinking about this a little bit more, wouldn't it make sense to > set these fields on the close-tab-button element rather than on that tab? As > it is, I could imagine this becoming unexpectedly buggy when tabs are > switched (I'm saying "imagine," because I cannot think of concrete use cases) I think you misread the binding parent? In the handler |this| is the tab bar. That part of the handler concerns blocking false double-clicks on the tab bar itself after the close button is destroyed (the first part of the handler is about the close button clicks). While I was looking over that, I noticed in the upper part of the handler: > // We preemptively set this to allow the closing-multiple-tabs- > // in-a-row case. > if (this._blockDblClick) { > target._ignoredCloseButtonClicks = true; > } else if (event.detail > 1 && !target._ignoredCloseButtonClicks) { > [...] > } > // Reset the "ignored click" flag > target._ignoredCloseButtonClicks = false; The reset at the end needs to be in an else block, so that the first branch actually has an effect. It's fixed in this patch.

Comment on attachment 815255 [details] [diff] [review] Renamed field to clickedTabBarOnce Yep, got confused. Sorry. r=mano

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ccef3ecfb113

Thank you!

> let target = event.originalTarget; > if (target.className == 'tab-close-button') { some extensions may add more classes to the class list, can you use target.classList.contains('tab-close-button') instead of target.className == 'tab-close-button' ?

(In reply to onemen.one from comment #30) > > let target = event.originalTarget; > > if (target.className == 'tab-close-button') { > > some extensions may add more classes to the class list, can you use > target.classList.contains('tab-close-button') instead of target.className == > 'tab-close-button' ? This was rejected in Comment 20

(In reply to Philip Chee from comment #31) > This was rejected in Comment 20 why ? Mano did not provide any reason. why not to use unique identifier, maybe anonid

(In reply to onemen.one from comment #30) > > let target = event.originalTarget; > > if (target.className == 'tab-close-button') { > > some extensions may add more classes to the class list, can you use > target.classList.contains('tab-close-button') instead of target.className == > 'tab-close-button' ? This was now fixed by pushing the patch in bug 880277 to fx-team.