Closed Bug 476928 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

3.5 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: whimboo, Assigned: zeniko)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 1 obsolete file)

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:53-ehsan.akhgari@gmail.com-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) {
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.
Component: Session Restore → Tabbed Browser
QA Contact: session.restore → tabbed.browser
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.
This is something which should really be fixed for FF3.1. Otherwise this behavior could bring up a lot of annoyed users.
Flags: blocking-firefox3.1?
(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.
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
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.
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.
Attached patch cleaned up tab closing (obsolete) — Splinter Review
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.
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #362619 - Flags: review?(dietrich)
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.
Thanks Simon. I've started a tryserver build to check it on Windows and OS X.
That looks good. Tryserver builds are available here: http://tinyurl.com/ccwxxk
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) {
Attachment #362619 - Flags: review?(dietrich) → review+
We're not that consistent, but sure.
Attachment #362619 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/035de2805eee
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Attachment #363517 - Flags: approval1.9.1?
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.
Status: RESOLVED → VERIFIED
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.
Keywords: relnote
Attachment #363517 - Flags: approval1.9.1? → approval1.9.1+
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.
Keywords: relnote
Keywords: checkin-needed
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?
(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.
Blocks: 482580
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.
Keywords: verified1.9.1
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.
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.
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.
Attachment #367105 - Flags: review?(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.
Reopening, since the bug as filed wasn't fixed by the original patch (but should be with both patches combined).
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Attachment #367105 - Flags: review?(dietrich) → review+
Comment on attachment 367105 [details] [diff] [review]
fix part II (never close the selected tab) (checked in)

r=me
Keywords: checkin-needed
Whiteboard: [checkin needed for part II]
Attachment #363517 - Attachment description: for check-in → for check-in (checked-in)
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?
(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 just uploaded the patch to the try server.
http://hg.mozilla.org/mozilla-central/rev/3c49ef2a9f76
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [checkin needed for part II]
(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.
Attachment #367105 - Flags: approval1.9.1?
Whiteboard: [part II needs 1.9.1 approval and landing]
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
Status: RESOLVED → VERIFIED
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.
Whiteboard: [part II needs 1.9.1 approval and landing] → [part II needs 1.9.1 approval and landing] [backed out on 191 branch - see comment 35]
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.
Johnathan, will it be checked-in again or do we need a combined patch now to fix the remaining problem on this bug?
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.
Keywords: checkin-needed
Whiteboard: [part II needs 1.9.1 approval and landing] [backed out on 191 branch - see comment 35] → [1.9.1 landing needed for part I][part II needs 1.9.1 approval and landing]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7d20407a4bec
Keywords: checkin-needed
Whiteboard: [1.9.1 landing needed for part I][part II needs 1.9.1 approval and landing] → [part II needs 1.9.1 approval]
Comment on attachment 367105 [details] [diff] [review]
fix part II (never close the selected tab) (checked in)

a191=beltzner
Attachment #367105 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/471268314eb1
Keywords: fixed1.9.1
Whiteboard: [part II needs 1.9.1 approval]
Attachment #367105 - Attachment description: fix part II (never close the selected tab) → fix part II (never close the selected tab) (checked in)
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.
No longer blocks: 482580
You need to log in before you can comment on or make changes to this bug.