476928 - Slow closing of tabs with the given testcase when entering Private Browsing mode

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090204020327

See bug 473843 comment 10 and 11 for an explanation. The given sessionstore.js (attachment 359929 [details]) shows this nicely at least with the following tryserver build:

https://build.mozilla.org/tryserver-builds/2009-01-30_01:[e-mail]-pbseamless/

Copying Simons comment over here:

Thanks, Henrik, with your sessionstore.js I can nicely reproduce this issue.
The problem seems to be that _endRemoveTab in tabbrowser.xml does some
performance intensive scrolling (sync'd) when the last tab is removed and we
remove the tabs starting at the end. You shouldn't be seeing this issue, if you
select the very first tab before entering PB mode.

To fix this, we could either scroll asynchronously in _endRemoveTab or just
remove the tabs starting at the first tab to be removed (which fixes this issue
for me):

>-    for (t = openTabCount - 1; t >= newTabCount; t--) {
>+    while (t <= tabbrowser.mTabs.length) {

Comment 1

[Simon Bünzli]

This should IMO be fixed in our tabbed browser implementation. The cited SessionStore change would only be a wall-paper solution for the impending 3.1 release.

Comment 2

[Nochum Sossonko [:Natch]]

How about modifying removeAllTabsBut to accept another parameter which overrides the confirmation? I think (I haven't tested this recently) that closes the tabs much more efficiently. Then sessionstore can use that to close all the tabs instead of doing it manually.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

This is something which should really be fixed for FF3.1. Otherwise this behavior could bring up a lot of annoyed users.

Comment 4

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

(In reply to comment #1)
> This should IMO be fixed in our tabbed browser implementation. The cited
> SessionStore change would only be a wall-paper solution for the impending 3.1
> release.

I filed bug 478750 for that.

This should be about the most expedient way to fix the issue at hand, be it through calling another method for closing all tabs (as per comment 2) or just hard coding that we start closing from the first tab (as per comment 1).

Either way, it doesn't block, as it won't affect all users. Wanted, though, so we'd take a patch that was tested and simple. Whipping up something like from comment 1 seems easiest.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Simon, does it mean we only need your proposed fix I mentioned again in comment 0 to fix it for FF3.1? If yes, I've created a tryserver build. It's available here: http://tinyurl.com/chht6r

I'll have a test and report back.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Simon, that doesn't work. Doing it that way we lose the content of all tabs after a restart. Feel free to test such a tryserver build.

Comment 7

[Simon Bünzli]

Attachment: cleaned up tab closing

So, my original wall-paper didn't work as well as I had envisioned. This one does: it cleans up the tab closing logic and as a side-effect also fixes this bug for good.

Dietrich: This three-liner is wanted for 3.1.

Comment 8

[Nochum Sossonko [:Natch]]

Nice, this makes it a lot smoother for me. I was also thinking of turning off smoothScroll temporarily, but I don't think it's necessary anymore and would also probably be more risky this late.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Thanks Simon. I've started a tryserver build to check it on Windows and OS X.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

That looks good. Tryserver builds are available here: http://tinyurl.com/ccwxxk

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 362619 [details] [diff] [review]
cleaned up tab closing


> 
>     // when overwriting tabs, remove all superflous ones
>-    for (t = openTabCount - 1; t >= newTabCount; t--) {
>-      tabbrowser.removeTab(tabbrowser.mTabs[t]);
>-    }
>+    if (aOverwriteTabs && newTabCount < openTabCount)
>+      Array.slice(tabbrowser.mTabs, newTabCount, openTabCount)
>+           .forEach(tabbrowser.removeTab, tabbrowser);

nit: style in this file seems to be to use brackets even for single-line if() blocks (eg, the code immediately below this change).

r=me otherwise

>     
>     if (aOverwriteTabs) {
>       this.restoreWindowFeatures(aWindow, winData);
>     }
>     if (winData.cookies) {
>       this.restoreCookies(winData.cookies);
>     }
>     if (winData.extData) {

Comment 12

[Simon Bünzli]

Attachment: for check-in (checked-in)

We're not that consistent, but sure.

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/035de2805eee

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Looks good with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090222 Minefield/3.2a1pre ID:20090222035031. Marking verified.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

The patch is still waiting for approval on 1.9.1. It's a highly visible behavior users with a lot of tabs will notice.

Due to we don't have it in beta 3 I'ld ask for a relnote.

Comment 16

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

a=beltzner for 191 landing

Don't think this is worthy of a relnote. It's UI choppiness that will be improved for the next beta.

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6f44d1dd4764

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

It's improved a lot on 1.9.1. Verfied with builds on Windows and OS X:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090309 Shiretoko/3.1b4pre

Simon, there is still some flickering visible. Means when closing all tabs the content of the first tab is visible for some milliseconds. Shall I file a follow-up bug for further optimizations?

Comment 19

[Simon Bünzli]

(In reply to comment #18)
> Shall I file a follow-up bug for further optimizations?

Sure, at least to investigate if we can improve the transition any further.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Filed bug 482580.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Sorry, but this is not fixed. Something has been changed. When I have verified this bug on 1.9.1 two days ago it seems fine but the same old behavior is present for Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090311 Shiretoko/3.1b4pre Ubiquity/0.1.5 ID:20090311030630

You only have to open about 10 tabs and select e.g. the second one. When entering the PB mode we still close all the tabs step by step. There is no change to the behavior as we had before the patch went into 1.9.1. This really affects 1.9.1 only, trunk doesn't show this behavior.

Comment 22

[(no longer active)]

Could you please try to find a regression range?  This may be a new bug which causes this to regress from the time that the patch landed on 1.9.1.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

I tried it with the same build I've mentioned in comment 18 and I'm still able to see this behavior. That's why my assumption is that the patch doesn't work on 1.9.1.

Comment 24

[Simon Bünzli]

Attachment: fix part II (never close the selected tab) (checked in)

Not closing the selected tab (which will lead to different tabs being selected while closing superfluous tabs) reduces the flickering further.

Henrik: Can you manually apply these four lines to your nsSessionStore.js and see if they help.

Comment 25

[Dietrich Ayala (:dietrich)]

Comment on attachment 367105 [details] [diff] [review]
fix part II (never close the selected tab) (checked in)

this bug is already marked fixed. if this is a further optimization, please file a new bug and ask for review there. otherwise, re-open this one.

Comment 26

[Simon Bünzli]

Reopening, since the bug as filed wasn't fixed by the original patch (but should be with both patches combined).

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 367105 [details] [diff] [review]
fix part II (never close the selected tab) (checked in)

r=me

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Simon, could you create a tryserver build for 1.9.1 so we could test and dont have to wait for checkin on 1.9.1?

Comment 29

[Simon Bünzli]

(In reply to comment #28)
I can't, I'm sorry. Then again, you can easily apply the patch yourself: Just copy the four lines to the correct place in nsSessionStore.js (and remove the initial "+" on each line).

Comment 30

[(no longer active)]

I just uploaded the patch to the try server.

Comment 31

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/3c49ef2a9f76

Comment 32

[(no longer active)]

1.9.1-based try server builds are available here:

<https://build.mozilla.org/tryserver-builds/2009-03-14_00:[e-mail]-1237015451/>

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to comment #29)
> (In reply to comment #28)
> I can't, I'm sorry. Then again, you can easily apply the patch yourself: Just
> copy the four lines to the correct place in nsSessionStore.js (and remove the
> initial "+" on each line).

I missed that. But thank you Ehsan. I've tried this 1.9.1 build and it works fine on OS X. Seems like everything will be fine now. Thanks Simon.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Verified fixed on Windows and OS X with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090322 Minefield/3.6a1pre ID:20090322035551

Comment 35

[Johnathan Nightingale [:johnath]]

Backed out changeset 6f44d1dd4764 (the first patch on 191) as part of trying to track down the Mac Ts regression being tracked by bug 486236.  I would re-open this but it's still landed on Trunk.  I would remove the fixed-191 keyword, but it doesn't have one. So I'm going to put it in the whiteboard so that we don't totally lose track of it.

Comment 36

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ce9c2499760d

Backout changeset

Comment 37

[Johnathan Nightingale [:johnath]]

That backout didn't seem to change things on 8.8.2 fast talos after the first
run, and those runs were pretty consistent at detecting the regression.

http://graphs-new.mozilla.org/graph.html#tests=[{%22test%22:16,%22branch%22:3,%22machine%22:49}]

Sorry for the annoyance.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Johnathan, will it be checked-in again or do we need a combined patch now to fix the remaining problem on this bug?

Comment 39

[Johnathan Nightingale [:johnath]]

Whimboo - probably better asked of zeniko and dietrich.  The first part was approved to land, and can reland, but if it makes more sense to bundle it into a complete fix, that works too.

Comment 40

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7d20407a4bec

Comment 41

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

Comment on attachment 367105 [details] [diff] [review]
fix part II (never close the selected tab) (checked in)

a191=beltzner

Comment 42

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/471268314eb1

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Verified fixed on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090411 Shiretoko/3.5b4pre ID:20090411030607 and on WindowsXP.