588426 - Restoring multiple solely hidden tabs (after crash) breaks session restore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P3)

Description

[Michael Kraft [:morac]]

If tabs are group and the browser crashes such that the crash prompt displays and the user deselects all tabs in the currently displayed group, then there will be no tabs in the tab bar but the page from a tab in a different group will display.

To fix this, simply press the new tab button or open Tab Candy and reselect the group.

I'm not sure what should occur in this case, but what's happening isn't correct.  Either a blank tab should display or a different group should display.

Steps to recreate:
1. Quit Firefox and place the attached sessionstore.js file in the profile folder.
2. Run Firefox and when the crash prompt displays, unselect Google and click restore.

Comment 1

[Michael Kraft [:morac]]

Attachment: sessionstore.js file that demonstrates issue

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Thanks for posting the sessionstore to test this with!

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Let's try to investigate this by beta 11.

Comment 4

[Raymond Lee [:raymondlee]]

I've followed the STR with the sessionstore.js.  After clicking on the restore, the window is resized but the Restore Session page remains and the "restore" button is disabled.

There is an error in the Console.
Error: uncaught exception: [Exception... "'[JavaScript Error: "aTabs[ex - 1] is undefined" {file: "file:///Users/raymondlee/Documents/glaxstar/mozilla-central/obj-ff-dbg/dist/Minefield.app/Contents/MacOS/components/nsSessionStore.js" line: 2481}]' when calling method: [nsISessionStore::setWindowState]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/aboutSessionRestore.js :: restoreSession :: line 144"  data: yes]

Comment 5

[Raymond Lee [:raymondlee]]

In this STR, user decides not to restore tabs in the current group so the unhiddenTabs is 0  which causes an error.

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2505

2504       // Determine if we can optimize & load visible tabs first
2505       let maxVisibleTabs = Math.ceil(tabbrowser.tabContainer.mTabstrip.scrollClientSize /
2506                                      aTabs[unhiddenTabs - 1].getBoundingClientRect().width);

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1, WIP (test only)

Tiny test the reproduces the bug.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

*that

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Requesting blocker status because this bug seems to break the whole browser when trying to restore hidden tabs.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Fix: optimizing to load visible tabs (the ones appearing on tab strip) first, only if there are at least 2 visible (non-hidden) tabs. If there's only one visible tab we don't need to reorder.

So when restoring only hidden tabs the first of these gets unhidden and is displayed (which is better than doing nothing and staying broken ;)

AFAIK the only way to hide tabs is using panorama (but of course with other add-ons in the near future). So it's not clear what session store should do when it finds that there are solely hidden tabs to be restored. Any feedback/ideas appreciated! :)

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Updated patch because a test failed.

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #9)
> If there's only one
> visible tab we don't need to reorder.
> 
> So when restoring only hidden tabs the first of these gets unhidden and is
> displayed (which is better than doing nothing and staying broken ;)
> 
> AFAIK the only way to hide tabs is using panorama (but of course with other
> add-ons in the near future). So it's not clear what session store should do
> when it finds that there are solely hidden tabs to be restored. Any
> feedback/ideas appreciated! :)

IMHO, this strikes me as yet another good reason to actually make group membership (group ID property, say) a first-class citizen of tabbrowser + sessionstore, not just hidden status. Post-fx4, of course. ;)

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #11)
> IMHO, this strikes me as yet another good reason to actually make group
> membership (group ID property, say) a first-class citizen of tabbrowser +
> sessionstore, not just hidden status. Post-fx4, of course. ;)

Indeed. If using panorama it would also be helpful to divide restorable tabs into groups not just windows (in the restore dialog after a crash).

Comment 13

[Dietrich Ayala (:dietrich)]

Can someone clarify exactly what the STR here are?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #13)
> Can someone clarify exactly what the STR here are?

Sure. The exact STR is:

1.) Have at least two panorama tab groups with some tabs and activate one of them
2.) Crash/kill Firefox
3.) Open Firefox
4.) Deselect all tabs of the last active group (in restore dialog)
5.) Click to restore tabs

Comment 15

[Michael Kraft [:morac]]

You'll have to crash Firefox multiple times to get the crash prompt.  That or set the browser.sessionstore.max_resumed_crashes preference to 0.

Comment 16

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

This is pretty edgy, won't block final.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #10)
> Created attachment 510632 [details] [diff] [review]
> patch v3

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=2b539b9c86c7

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

Little fix: 

+    // if all tabs to be restored are hidden, make the first one visible
+    if (unhiddenTabs == 0) {
+      aTabData[0].hidden = false;
+    } else if (unhiddenTabs > 1) {
       // Load hidden tabs last, by pushing them to the end of the list

Changed to:

+    // if all tabs to be restored are hidden, make the first one visible
+    if (unhiddenTabs == 0) {
+      aTabData[0].hidden = false;
+    } else if (aTabs.length > 1) {
       // Load hidden tabs last, by pushing them to the end of the list

Reason:

We also want to reorder when there is at least one hidden tab and at least two tabs altogether to ensure visible tabs are restored first.

Comment 19

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

Comment on attachment 510963 [details] [diff] [review]
patch v4

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js
>-    if (aTabs.length > 1) {
>+    let filterHidden = function (aTabData) !aTabData.hidden;
>+    let unhiddenTabs = aTabData.filter(filterHidden).length;

Just do this on 1 line
> let unhiddenTabs = aTabData.filter(function(aData) !aData.hidden).length

>+
>+    // if all tabs to be restored are hidden, make the first one visible
>+    if (unhiddenTabs == 0) {
>+      aTabData[0].hidden = false;
>+    } else if (aTabs.length > 1) {

That seems fine to me.

>       // Load hidden tabs last, by pushing them to the end of the list
>-      let unhiddenTabs = aTabs.length;
>-      for (let t = 0; t < unhiddenTabs; ) {
>+      for (let t = 0, n = aTabs.length - unhiddenTabs; 0 < n; ++t) {
>         if (aTabData[t].hidden) {
>           aTabs = aTabs.concat(aTabs.splice(t, 1));
>           aTabData = aTabData.concat(aTabData.splice(t, 1));
>           if (aSelectTab > t)
>             --aSelectTab;
>-          --unhiddenTabs;
>-          continue;
>+          --n;
>         }
>-        ++t;
>       }

2 things here.
1. I don't think this is going to work if you have hidden, hidden, visible tabs. ++t every time is going to skip tabs immediately following a hidden tab (since the arrays get modified in the loop). I thought browser_480148.js tested that, but I guess not. Please add a test case there to make sure we don't miss that again. So let's keep the continue and ++t where they were.
2. can we call this something besides n? tabsToReorder makes sense to me and then make the condition tabsToReorder > 0

>diff --git a/browser/components/sessionstore/test/browser/browser_588426.js b/browser/components/sessionstore/test/browser/browser_588426.js
>+function test() {
>+  ss.setBrowserState(JSON.stringify({ windows: [{ tabs: [
>+      {entries: [{url: "about:home"}], hidden: true},
>+      {entries: [{url: "about:blank"}], hidden: true}

let's avoid about:home & about:blank. Use about:mozilla & about:robots

>+  whenTabsRestored(2, function () {

I wrote waitForBrowserState (in head.js) for this very purpose. So please use that.
> waitForBrowserState(stateObject, callback)

>+    is(gBrowser.tabs.length, 2, "two tabs were restored");
>+    is(gBrowser.visibleTabs.length, 1, "one tab is visible");
>+
>+    let tab = gBrowser.tabs[0];
>+    whenTabIsLoaded(tab, function () {

Is it always the case that we'll get another load event? I think SSTabRestored fires after the load for the document.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

(In reply to comment #19)
> Just do this on 1 line
> > let unhiddenTabs = aTabData.filter(function(aData) !aData.hidden).length

Fixed.

> 1. I don't think this is going to work if you have hidden, hidden, visible
> tabs. ++t every time is going to skip tabs immediately following a hidden tab
> (since the arrays get modified in the loop). I thought browser_480148.js tested
> that, but I guess not. Please add a test case there to make sure we don't miss
> that again. So let's keep the continue and ++t where they were.

Fixed and new test case added in browser_480148.js.

> 2. can we call this something besides n? tabsToReorder makes sense to me and
> then make the condition tabsToReorder > 0

Fixed.

> let's avoid about:home & about:blank. Use about:mozilla & about:robots

Fixed.

> I wrote waitForBrowserState (in head.js) for this very purpose. So please use
> that.

Fixed.

> Is it always the case that we'll get another load event? I think SSTabRestored
> fires after the load for the document.

Seemed to work for about:blank only? Removed that completely.

Comment 21

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

Comment on attachment 511811 [details] [diff] [review]
patch v5

Look much better. Thanks! r=me. This should go green on try, but please push there first to make sure.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #21)
> Look much better. Thanks! r=me. This should go green on try, but please push
> there first to make sure.

Thanks! Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=ac019fba7c40

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 511811 [details] [diff] [review]
patch v5

Passed try with some known intermittent oranges. Requesting approval. This is a small patch with a test and this bug could lead to session restore being broken (though the STR is indeed pretty edgy).

Comment 24

[Michael Kraft [:morac]]

(In reply to comment #23)
> Passed try with some known intermittent oranges. Requesting approval. This is a
> small patch with a test and this bug could lead to session restore being broken
> (though the STR is indeed pretty edgy).

There are other ways to reproduce this, but they involve using an add-on that calls the SessionStore setBrowserState or setWindowState API functions.  That's actually how I found the problem originally.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 26

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/36e155375cd9

Comment 27

[AndreiD[QA]]

Tested the issue with the STRs in comment #14 on:
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre

After a restoring the session, the two groups appear in panorama (one blank and one with the other tabs).
But only one tab (from the previously inactive group) is loaded after session restore.

Comment 28

[u279076]

Ok, I've tested this on Firefox 4.0b12pre 20110218 and I'm not sure if what I'm seeing is expected.

STR:
1. Open 6 tabs to different sites
2. Open Panorama and drag last 3 tabs to create a 2nd group
3. Click one of the tabs in the new group to exit Panorama
4. Crash Firefox
5. Restart Firefox
*** On restore, all 6 tabs are loaded in the main window.  If I go to Panorama view I see Tabgroup 1 with 3 tabs, Tabgroup 2 with 2 tabs, and the 6th tab on its own ***
6. Fix the groupings and open a tab in the 2nd grouping again
7. Crash Firefox and Restart again
8. Uncheck the last three tabs from about:sessionrestore and click Restore
*** On restore, I get the 3 selected tabs in the main window. If I go to Panorama view I see Tabgroup 1 with 2 tabs and Tabgroup 2 with the 3rd tab

Simplified STR:
1. Load 6 tabs: ABCDEF
2. Split into 2 groups: ABC DEF
3. Display the 2nd group: DEF
4. Crash: ABCDEF in main window, ABC DE F in panorama
5. Crash, uncheck DEF: ABC in main window, AB C in panorama

Comment 29

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

Hmm, so I didn't reproduce that :( (brand spanking new profile)
At step 5 all 6 tabs were visible, but entering/exiting tabview hid the other groups (and I only had 2 groups as expected). I saved my session files at each point so I'll see if there's anything in there.

Comment 30

[u279076]

(In reply to comment #29)
> Hmm, so I didn't reproduce that :( (brand spanking new profile)
> At step 5 all 6 tabs were visible, but entering/exiting tabview hid the other
> groups (and I only had 2 groups as expected). I saved my session files at each
> point so I'll see if there's anything in there.

Yeah, I tested with a new profile too.  For the record, on an Ubuntu 10.10 32-bit VM.

Comment 31

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

Hmm I think the all tabs visible at step 5 is something different. All of my tabs thought they were visible (hidden: false in sessionstore.js). This is probably because we didn't do anything to trigger a save... TabHide & TabShow don't trigger one... (why not? sigh)

Comment 32

[u279076]

Should we reopen to investigate this bug further?

Comment 33

[AndreiD[QA]]

Tested the issue with the STRs in comment #14 on:
Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110221 Firefox/4.0b12pre

Restoring Tabs does not break the window, but only one is visible tab (from the previously inactive group) after session restore.

Comment 34

[u279076]

I'm reopening this bug so we can investigate whether or not what I am seeing in comment 28 is an issue with this bug.

Comment 35

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

I filed bug 635418 for the issue I mentioned in comment 31.

(In reply to comment #33)
> Restoring Tabs does not break the window, but only one is visible tab (from the
> previously inactive group) after session restore.

I think that's what we're expecting here.

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #35)
> (In reply to comment #33)
> > Restoring Tabs does not break the window, but only one is visible tab (from the
> > previously inactive group) after session restore.
> 
> I think that's what we're expecting here.

Indeed, session restore doesn't know anything about tab group structures at the moment so it just shows the first hidden tab (instead of a whole group).

Comment 37

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

If this is the expected behaviour, can we re-close this bug?

Comment 38

[Kevin Hanes]

Regardless of open or closed, it shouldn't be on our betaN list... moving

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

Cannot reproduce comment #28. Comment #33 is expected behavior.

Comment 40

[u279076]

I can no longer reproduce what I was seeing in comment 28 with Firefox 4.0rc1 -- marking verified.