624727 - Removed app tabs get overriden and reused in nsSessionStore::restoreWindow()

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Maniac Vlad Florin (:vladmaniac)]

Attachment: screenshot

Build ID:

Mozilla/5.0 (Windows NT 6.1; rv:2.0b9) Gecko/20100101 Firefox/4.0b9

Steps: 

1. Navigate some tabs
2. Enter panorama and put the tabs into two or more groups. (min is 2) 
3. Make some app tabs. (two will do) 
4. Go private browsing 
5. Enter panorama again 

Behavior: 

Groups from normal mode are saved in PB due to app tab presence 
Exiting private browsing mode will also result in destroying group organizing 

Note: see screenshot

Comment 1

[aravindm]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b9) Gecko/20100101 Firefox/4.0b9

Able to reproduce. After entering private browsing mode able to see the tabs groups and app tabs in panorama view. After exiting PB mode, tabs groups are destroyed.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre

Unable to reproduce on trunk. Please see attached video for to see the steps I followed.

If this still occurs, STR which consistently produce the problem would be helpful, and we should nominate this to be a blocker. Otherwise, though, we should close it.

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Works for me video

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: Doesn't work for me

I can reproduce this. See the video.

Comment 5

[GuillaumeC]

Using FF 4.0b10 on WinXP x64:

1. Navigate some tabs (3 mini)
2. Out of those tabs create min 2 AppTabs
3. Create one of more groups out of the remaining tabs
4. Go private browsing
5. Stop private browsing
6. Enter panorama

Expected behaviour: tab groups, tabs and app tabs are recreated as when entering Private Browsing.

Actual behavious: tab groups are recreated empty, regular tabs are all stacked up in the upper left corner, app tabs are ok. No easy way to recreate the proper settings unless stopping FF with save tabs option checked and restarting.

Comment 6

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre

Also, after issue is reproduced, new created tabs are displayed in tab view as being part of the group, but in Panorama they are displayed in the top left corner as part of a separate group. (See screenshot 1 and 2 that display the behavior of the same opened pages in both Panorama and Tab view)

Comment 7

[George Carstoiu]

Attachment: Screenshot 1

Comment 8

[George Carstoiu]

Attachment: Screenshot 2

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Reliable STR:

1.) Have two tabs.
2.) Pin them both.
3.) Enter private browsing mode.

Result:

Error: maxVisibleTabs.stop is not a function
Source File: file:///home/tim/workspace/mozilla-central/objdir-ff-release/dist/bin/components/nsSessionStore.js
Line: 2522

After that the browser works as expected but panorama is broken and produces the weird states described in former comments. So this is a rather PB/SS related bug.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Explanation:

When having two app tabs and switching to PB, restoreWindow() is called with
openTabs=2 and newTabs=1. So the first of the two app tabs will be overridden.
In the for loop the first tab gets unpinned and so immediately moved behind the
second app tab. The second loop iteration accesses this first tab again
(because gBrowser.tabs[] is accessed directly) and removes it. So the session
store is going to use a removed tab.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Pushed to try.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 507807 [details] [diff] [review]
patch v1

Thanks again for looking into this.

>+++ b/browser/components/sessionstore/src/nsSessionStore.js
>+    // unpin all tabs to ensure they are not reordered in the next loop
>+    if (aOverwriteTabs) {
>+      for (let t = tabbrowser._numPinnedTabs-1; t > -1; t--)

Nit: tabbrowser._numPinnedTabs - 1

>+++ b/browser/components/sessionstore/test/browser/browser_624727.js
>+  is(gBrowser.tabs[0].linkedBrowser, linkedBrowser, 'first tab\'s browser got re-used');

Nit: I wouldn't say anything if you didn't have to escape a quote, but please just use double quotes throughout.

Test looks good. Assuming it's failing without your patch (it should be), r=zpao.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Alright I'll fix that :) I ensured that both tests are failing without the patch applied.

Comment 14

[Ian Gilman [:iangilman]]

Comment on attachment 507807 [details] [diff] [review]
patch v1

I just reviewed the Panorama test; looks beautiful!

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Passed try!

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/a76570899a34