897751 - Tab close button's DoubleClick protection handler leaks

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Bug 537013 looks suspicious.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 12

[Adam]

Attachment: Handle errant clicks from tabbrowser-tabs handler

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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

review ping

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 15

[Dão Gottwald [:dao]]

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?

Comment 16

[Adam]

Attachment: Separate handler out and move remaining ignoreClick code

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

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 18

[Asaf Romano (gone)]

I will get to this sometime today.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Mano: review ping?

Comment 20

[Asaf Romano (gone)]

Comment on attachment 799271 [details] [diff] [review]
Separate handler out and move remaining ignoreClick code

># HG changeset patch
># User Adam Dane [:hobophobe] <[e-mail]>
># 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).

Comment 21

[Adam]

Attachment: Addressed review requests and prepared for checkin

Thanks Mano!

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

Comment 22

[Asaf Romano (gone)]

_clickedOnce should also be renamed.

Comment 23

[Asaf Romano (gone)]

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)

Comment 24

[Adam]

Attachment: Renamed field to clickedTabBarOnce

(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 25

[Asaf Romano (gone)]

Comment on attachment 815255 [details] [diff] [review]
Renamed field to clickedTabBarOnce

Yep, got confused. Sorry.

r=mano

Comment 26

[Adam]

Attachment: Refresh for checkin

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/855adbbb8604

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/855adbbb8604

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Thank you!

Comment 30

[tabmix.onemen]

> 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' ?

Comment 31

[Philip Chee]

(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

Comment 32

[tabmix.onemen]

(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

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(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.