587922 - Tabs are not actually closed after using TabCandy interface

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Matheus Kerschbaum]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b4pre) Gecko/20100816 Minefield/4.0b4pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b4pre) Gecko/20100816 Minefield/4.0b4pre  Built from http://hg.mozilla.org/mozilla-central/rev/62d9ac429278

Pretty nasty bug if it actually keeps tabs on memory. Looks like a regression from bug 586693.

Reproducible: Always

Steps to Reproduce:
1.Open two tabs
2.Go to tabcandy interface
3.Go back to one of the tabs
4.Close one of the tabs (notice that the close button continues to appear on the last tab even tho it shouldn't)
5.Attempt to close the window (it will ask you if you wish to save your session even tho there's only one tab)
6.Restart the browser
7.Re-open tabcandy interface and move the group containing the last tab to somewhere else.
Actual Results:  
The second tab opened in step 1 appears beneath the first tab group, which explains why when closing the entire browser it asked you if you wished to save the session.

Expected Results:  
The second tab should be closed properly.

Should block beta4.

Comment 1

[Matheus Kerschbaum]

Simpler STR on Win7:
1-Go to tabcandy interface
2-Open several tabs
3-Close those tabs
4-Look at tab previews

Results: It will show the tabs that you already closed, and if you attempt to close by clicking on the little "x" icon on the previews it will freeze the browser and after a while a message appears saying: "A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.

Script: chrome://browser/content/tabbrowser.xml:1461"

Comment 3

[Alice0775 White]

I can confirm str in comment#0.
Works:
http://hg.mozilla.org/mozilla-central/rev/639e221ffc55
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100816 Minefield/4.0b4pre ID:20100816105355
Fails;
http://hg.mozilla.org/mozilla-central/rev/a15f73b8d431
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100816 Minefield/4.0b4pre ID:20100816111312
Regression Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=639e221ffc55&tochange=a15f73b8d431

Comment 4

[Ed Lee :Mardak]

Do you see anything in the error console when you close tabs?

Comment 5

[Alice0775 White]

(In reply to comment #4)
> Do you see anything in the error console when you close tabs?
No error.

However, warning and message are as follows.
[After Step 2]
Warning: useless expression
Source file: chrome://browser/content/tabview.js
Line: 5007

    //   checkItemStatus    - (boolean) make sure this is a valid item which should be snapped
    snapBounds: function Drag_snapBounds(bounds, stationaryCorner, assumeConstantSize, keepProportional, checkItemStatus) {
      if (!stationaryCorner)
>>      stationaryCorner || 'topleft';
      var update = false; // need to update
      var updateX = false;


[Open TabCandy(extra step) after step 4]
tabview assert: must already be linked([object XULElement])@chrome://browser/content/tabview.js:4575
()@chrome://browser/content/tabview.js:4673
()@chrome://browser/content/tabview.js:4711
()@chrome://browser/content/tabview.js:4406
(-2)@chrome://browser/content/tabview.js:600
@:0

Comment 6

[Ed Lee :Mardak]

It seems like on closing a tab, TabSelect on the new tab results in showOnlyTheseTabs being called on an array of tabs no longer including the tab being closed.

This results in the animating closing tab to be hidden. And hidden tabs don't get transitionend events.

:(

Don't hide tabs if they're in the _removingTabs array?

Comment 7

[Ed Lee :Mardak]

Or immediately remove the tab if it's being hidden?

Comment 8

[Ed Lee :Mardak]

Attachment: v1

At first I forced the remove to be immediate when hiding, but that would just result in the animation being skipped. So I switched to just ignoring the tab being removed when hide is called.

Comment 9

[Ed Lee :Mardak]

Attachment: v2

I'm not entirely sure how it's getting broken anymore. But in the process of writing the test, it seems like doing..

t = addTab(noanimate)
removeTab(t, animate)
hideTab(t)

That doesn't trigger the problem -- transitionend still happens. Also calling hideTab from TabClose doesn't trigger the problem.

Only calling hideTab from TabSelect seems to prevent transitionend. This code path would be removeTab -> blurTab -> selectTab -> hideTab -> remove fadein so perhaps the tab is hidden and no transition happens.

But I don't see how that's different from removeTab -> hideTab -> blurTab -> remove fadein.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 466605 [details] [diff] [review]
v2

Which part of bug 586693 caused this in the first place?

>-      <property name="visibleTabs" readonly="true"
>-                onget="return Array.filter(this.tabs, function(tab) !tab.hidden);"/>
>+      <property name="visibleTabs" readonly="true">
>+        <getter><![CDATA[
>+          return Array.filter(this.tabs, function(tab) {
>+            return !tab.hidden && this._removingTabs.indexOf(tab) == -1;
>+          }, this);
>+        ]]></getter>
>+      </property>

If you do this you can (should, really) skip some work in _blurTab.

>       <method name="hideTab">
>         <parameter name="aTab"/>
>         <body>
>         <![CDATA[
>-          if (!aTab.hidden && !aTab.pinned && !aTab.selected) {
>+          // Tabs being removed will disappear soon, so skip them
>+          let removing = this._removingTabs.indexOf(aTab) > -1;
>+          if (!aTab.hidden && !aTab.pinned && !aTab.selected && !removing) {

indexOf is slightly more expensive than the other stuff, so it shouldn't be done eagerly. Probably best to get rid of the local variable.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

If the tabs remained in memory wouldn't this have triggered all kinds of leak tests?

Comment 0 and comment 1 are unclear: are you closing tabs using any mechanism (ctrl-w, closebutton on tabstrip) or does this only happen for tabs that you close within the tabcandy UI?

I've sent beta 4 to build already, but this might require a respin.

Comment 12

[Alice0775 White]

for comment #0,
Step 2-1 Gather all tabs in a group
Step 4. Closing by x button or Middle click on the tab in tabstrip

I can not try comment #1 because str is not clear.

Comment 13

[Matheus Kerschbaum]

(In reply to comment #11)
> If the tabs remained in memory wouldn't this have triggered all kinds of leak
> tests?
> 
> Comment 0 and comment 1 are unclear: are you closing tabs using any mechanism
> (ctrl-w, closebutton on tabstrip) or does this only happen for tabs that you
> close within the tabcandy UI?
> 
> I've sent beta 4 to build already, but this might require a respin.

Basically after using the TabCandy interface and trying to close tabs using the close button on tabstrip the tabs are not actually closing but just hidden away. Some things are probably still flushed out of memory, but others are definitely kept, as memory usage with just one tab after closing about 15 others was at 350mb.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, that blocks b4

Comment 15

[Ed Lee :Mardak]

Attachment: v3

(In reply to comment #10)
> Which part of bug 586693 caused this in the first place?
When closing the tab, TabSelect is fired on the new tab, and TabCandy will showOnlyTheseTabs not including the one that just got removed and that somehow prevents transitionend from happening. Before the setTimeouts would allow removeTab to finish its whole function including blurTab then remove fadein attribute. But because TabCandy handles TabSelect synchronously, it's able to hide it in a bad way.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 466699 [details] [diff] [review]
v3

>+            // Switch to a visible tab unless there aren't any remaining
>+            let remainingTabs = this.visibleTabs;
>+            if (remainingTabs.length == 1 && remainingTabs[0] == aTab) {

This also needs to take into account that remainingTabs can be empty.

>+  // Make sure the end remove tab still gets called even after calling hide
>+  let origEnd = gBrowser._endRemoveTab;
>+  gBrowser._endRemoveTab = function() {
>+    // Restore the original function and finish removing
>+    gBrowser._endRemoveTab = origEnd;
>+    gBrowser._endRemoveTab.apply(gBrowser, arguments);
>+
>+    is(numVisBeforeHide, 1, "animated remove has in 1 tab left");
>+    is(numVisAfterHide, 1, "hiding a removing tab is also has 1 tab");
>+    finish();
>+  };

If possible I'd rather not mess with _endRemoveTab like this in a test. Can you poll gBrowser.tabs.length instead?

Comment 17

[Ed Lee :Mardak]

Attachment: v4

Comment 18

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/40fa4ebeacbb
Skip hiding tabs that are being removed to let it finish animating instead of hiding it to prevent the animation from finishing. 

GECKO20b4_20100817_RELBRANCH commit:
http://hg.mozilla.org/mozilla-central/rev/96aa1d8d1f70

Comment 20

[Matheus Kerschbaum]

FWIW, closing a tab through the TabCandy interface in a different window still triggers this bug. Should I file a follow-up for that or would bug 578512 fix this?

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Please file a follow up, nominate for blocking, cc the list. If bug 578512 fixes that, more's the better, but let's make sure the specific case you're talking about it on file.