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

VERIFIED FIXED in Firefox 4.0b12

Status

()

defect
P3
normal
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: morac, Assigned: ttaubert)

Tracking

Trunk
Firefox 4.0b12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [approved-patches-landed])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Attachment #467029 - Attachment description: install this in profile → sessionstore.js file that demonstrates issue
(Reporter)

Updated

9 years ago
Blocks: 585689
Thanks for posting the sessionstore to test this with!
Priority: -- → P3
Let's try to investigate this by beta 11.
Blocks: 627096
No longer blocks: 585689
Keywords: qawanted
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]
Assignee: nobody → tim.taubert
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);
Posted patch patch v1, WIP (test only) (obsolete) — Splinter Review
Tiny test the reproduces the bug.
*that
Requesting blocker status because this bug seems to break the whole browser when trying to restore hidden tabs.
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Component: TabCandy → Session Restore
QA Contact: tabcandy → session.restore
Summary: If deselect all tabs in a tab group in crash prompt, no tabs display in tab bar and page from another tab displays → Restoring multiple solely hidden tabs (after crash) breaks session restore
Keywords: qawanted
(Assignee)

Updated

8 years ago
Blocks: 626527
Posted patch patch v2 (obsolete) — Splinter Review
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! :)
Attachment #510544 - Attachment is obsolete: true
Attachment #510553 - Flags: review?(paul)
Attachment #510553 - Flags: feedback?(ian)
Posted patch patch v3 (obsolete) — Splinter Review
Updated patch because a test failed.
Attachment #510553 - Attachment is obsolete: true
Attachment #510632 - Flags: review?(paul)
Attachment #510632 - Flags: feedback?(ian)
Attachment #510553 - Flags: review?(paul)
Attachment #510553 - Flags: feedback?(ian)
(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. ;)
(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).
Can someone clarify exactly what the STR here are?
(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
(Reporter)

Comment 15

8 years ago
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.
This is pretty edgy, won't block final.
blocking2.0: ? → -
Posted patch patch v4 (obsolete) — Splinter Review
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.
Attachment #510632 - Attachment is obsolete: true
Attachment #510963 - Flags: review?(paul)
Attachment #510963 - Flags: feedback?(ian)
Attachment #510632 - Flags: review?(paul)
Attachment #510632 - Flags: feedback?(ian)
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.
Attachment #510963 - Flags: review?(paul) → review-
Posted patch patch v5 (obsolete) — Splinter Review
(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.
Attachment #510963 - Attachment is obsolete: true
Attachment #511811 - Flags: review?(paul)
Attachment #510963 - Flags: feedback?(ian)
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.
Attachment #511811 - Flags: review?(paul) → review+
(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=tim.taubert@gmx.de&rev=ac019fba7c40
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).
Attachment #511811 - Flags: approval2.0?
(Reporter)

Comment 24

8 years ago
(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.
Attachment #511811 - Flags: approval2.0? → approval2.0+
Attachment #511811 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/36e155375cd9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12

Comment 27

8 years ago
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.
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
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.
(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.
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)
Should we reopen to investigate this bug further?

Comment 33

8 years ago
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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).
If this is the expected behaviour, can we re-close this bug?
Whiteboard: [approved-patches-landed]

Comment 38

8 years ago
Regardless of open or closed, it shouldn't be on our betaN list... moving
Blocks: 585689
No longer blocks: 627096
Cannot reproduce comment #28. Comment #33 is expected behavior.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
I can no longer reproduce what I was seeing in comment 28 with Firefox 4.0rc1 -- marking verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.