Undo close (hidden) tab causes panorama and session restore(?) to break unrecoverably

VERIFIED FIXED in Firefox 4.0b11

Status

()

Firefox
Session Restore
P2
normal
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: vladmaniac, Assigned: ttaubert)

Tracking

({regression})

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

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [4b10][hardblocker][b11])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 506381 [details]
screenshot

Build ID: 

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

Prerequisites:
clean profile 

STR: 
1. Navigate some pages in a few tabs 
2. Go to panorama and close one of the tabs 
3. Return to normal mode 
4. Go back to panorama then reopen the previously closed tab (CMD + SHIFT + T) 

Behavior: 
The reopened tab is isolated from its native group and located in the upper left corner of the panorama content area. 
It is mandatory to close and reopen panorama to reproduce this. If not, the tab position will not hang, but you will shortly see that it appears on the left upper corner for a second, then it's moved to its place in the group. 

This works on win7, and I could reproduce it only on mac os x 10.6 

Note: see screenshot
(Reporter)

Updated

7 years ago
Keywords: regression
Whiteboard: [4b10]

Comment 1

7 years ago
This was marked as fixed in bug 624265.
Blocks: 624265
(Reporter)

Comment 2

7 years ago
yes, it works on win7, but that's it
(Reporter)

Comment 3

7 years ago
Furthermore, bug 624265 states that a new tab group is created, which isn't the case here. I am sure that somehow issues are connected that's why the "regression" keyword is there
Nominating for blocking because of a case of state loss.
blocking2.0: --- → ?
Blocks: 627096
(Assignee)

Comment 5

7 years ago
This is definitely no duplicate of bug 624265 but no regression either. There's something really weird going on. Different STR:

1. Open panorama with multiple groups
2. Close tab from a group that was not active when switching to panorama
3. Undo close tab

Result:

Error: uncaught exception: [Exception... "'[JavaScript Error: "aTabs[ex - 1] is undefined" {file: "file:///home/tim/workspace/mozilla-central/objdir-ff-release/dist/bin/components/nsSessionStore.js" line: 2459}]' when calling method: [nsISessionStore::undoCloseTab]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/browser.js :: undoCloseTab :: line 11989"  data: yes]

Also nominating for blocking because the browser state is really borked after that and does not recover until restarted.
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Updated

7 years ago
Summary: Undo close tab isolates the reopened tab from its group if navigating in and out of panorama → Undo close tab from inactive group causes browser state to be unrecoverably broken
From a quick look, it seems like something in sessionstore will need to be fixed...
(Assignee)

Comment 7

7 years ago
Created attachment 506630 [details] [diff] [review]
patch v1
Attachment #506630 - Flags: review?(paul)
Comment on attachment 506630 [details] [diff] [review]
patch v1

#1, Thanks a lot for looking into this and diving into the crazy world of session restore
#2, You made me actually look closely at this code again and there are consequences ;) (and so it follows...)
#3, You can ignore my first rambling block down here
#4, I've poked Ian to look at the panorama bits

>+++ b/browser/components/sessionstore/src/nsSessionStore.js

So there's a line up here
>     if (aTabs.length > 0) {
that I think we can just change to if > 1. All that block is doing is reordering tabs, but if there's a single tab then there isn't anything to reorder. I guess it also sets gBrowser.selectedTab. But in the case of a hidden tab we don't want that selected do we? But we do in the unhidden case. Actually, let's not mess with that

Most of that method is a no-op if aTabs.length == 0 so I'm having trouble validating why this code is like this (even though I reviewed the patch which put the if in there...)

We could "simplify" this by moving if (!this._isWindowLoaded(aWindow)) (and return early) up before if (aTabs.length > 0), make that > 1, and move the part that sets aSelectTab out of that aTabs.length block...

</ramble>

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

This does actually fix the problem, and I would take this. It just bothered me that we do a lot of unnecessary things. Adding this in makes us do more unnecessary things because we're doing unnecessary things in the first place, but it's the safest thing to do this late in the game. If you're still with me, r+ on this part :)

But we really should have a sessionstore test for this. Especially since it's entirely possible to do this outside of panorama right now. So r- until that exists. It should be a pretty straightforward test.
Attachment #506630 - Flags: review?(paul)
Attachment #506630 - Flags: review?(ian)
Attachment #506630 - Flags: review-
I would call this a blocker as well, and a hardblocker at that. Too much potential for dataloss. And with a patch in progress, it doesn't seem to be a long pole.
Summary: Undo close tab from inactive group causes browser state to be unrecoverably broken → Undo close (hidden) tab causes panorama and session restore(?) to break unrecoverably
Whiteboard: [4b10] → [4b10][needs review ian][needs sessionstore test]
(Assignee)

Comment 10

7 years ago
Try failed: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_480148.js | Test #2: 'visible' tabs restored first - Got 12 0 1 2 3 4 5 6 7 8 9 10 11, expected 12 7 8 9 10 11 0 1 2 3 4 5 6
blocking2.0: ? → betaN+
Keywords: regression
Whiteboard: [4b10][needs review ian][needs sessionstore test] → [4b10][needs review ian][needs sessionstore test][hardblocker]
Comment on attachment 506630 [details] [diff] [review]
patch v1

The Panorama portion and the test look good to me (I didn't look at the sessionstore portion).
Attachment #506630 - Flags: review?(ian) → review+
Priority: -- → P2
(Assignee)

Comment 12

7 years ago
Created attachment 507975 [details] [diff] [review]
patch v2, WIP
Attachment #506630 - Attachment is obsolete: true
Comment on attachment 507975 [details] [diff] [review]
patch v2, WIP

I know I mentioned this on IRC, but those comments get lost, so...

>-    if (aTabs.length > 0) {
>+    if (!this._isWindowLoaded(aWindow)) {
>+      // from now on, the data will come from the actual window
>+      delete this._statesToRestore[aWindow.__SS_restoreID];
>+      delete aWindow.__SS_restoreID;
>+      delete this._windows[aWindow.__SSi]._restoring;
>+      return;
>+    }

Do the return outside of this if, explicitly checking for aTabs.length == 0 first. The first part could apply even if aTabs.length > 0, so we still want to run it & the rest of the method in that case.

And then also, since we won't make it into restoreHistory, we won't ever be making the call to this._sendWindowStateEvent(aWindow, "Ready"), so we should do that in the aTabs.length == 0 case. I'll file a followup to make sure we test that case (since if tests were passing without that, then we don't have enough coverage there).
Attachment #507975 - Flags: feedback+
(Assignee)

Comment 14

7 years ago
Created attachment 508181 [details] [diff] [review]
patch v3

Pushed to try.
Attachment #507975 - Attachment is obsolete: true
Attachment #508181 - Flags: review?(paul)
(Assignee)

Updated

7 years ago
Attachment #508181 - Flags: review?(ian)
(Assignee)

Updated

7 years ago
Component: TabCandy → Session Restore
QA Contact: tabcandy → session.restore
(Assignee)

Updated

7 years ago
Blocks: 626527
(Assignee)

Comment 15

7 years ago
Passed try.

Updated

7 years ago
Whiteboard: [4b10][needs review ian][needs sessionstore test][hardblocker] → [4b10][needs review ian][needs sessionstore test][hardblocker][has patch]
Comment on attachment 508181 [details] [diff] [review]
patch v3

>+  // clean up after ourselves if something goes wrong
>+  registerCleanupFunction(function () {
>+    if (gPrefService.prefHasUserValue("browser.sessionstore.interval"))
>+      gPrefService.clearUserPref("browser.sessionstore.interval");
>+
>+    let tab = gBrowser.tabs[1];
>+    if (tab)
>+      gBrowser.removeTab(tab);
>+  });

If something is going wrong the test is failing. Do we need to register this cleanup function? Do one of the tests actually fail before we get in here?

>+  waitForSaveState(function () {
>+    gBrowser.removeTab(tab);
>+
>+    tab = ss.undoCloseTab(window, 0);
>+    gBrowser.removeTab(tab);
>+
>+    waitForSaveState(finish);
>+    gPrefService.setIntPref("browser.sessionstore.interval", 0);
>+  });

Using waitForSaveState isn't really necessary here, so let's not do that. I really don't like to use that method unless we really need to. You should be able to just remove, undoClose, and then remove. If triggering getting session state is needed (it shouldn't be here) then you can use ss.getBrowserState.

Otherwise it looks good.
(Assignee)

Comment 17

7 years ago
(In reply to comment #16)
> If something is going wrong the test is failing. Do we need to register this
> cleanup function? Do one of the tests actually fail before we get in here?

The cleanup function is only necessary because I'm using waitForSaveState() later. It's possible that the callback is never fired and so it's better to cleanup to prevent cascading failures?!

> Using waitForSaveState isn't really necessary here, so let's not do that. I
> really don't like to use that method unless we really need to. You should be
> able to just remove, undoClose, and then remove. If triggering getting session
> state is needed (it shouldn't be here) then you can use ss.getBrowserState.

I'm using that function because I was rather clueless. When removing that function call/wrap I get the following error:

Exception thrown - [Exception... "'Illegal value' when calling method: [nsISessionStore::undoCloseTab]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_628270.js :: test :: line 46"  data: no]

So it seems adding, removing, unclosing is so quick that it doesn't make it into session store? Even calling ss.getBrowserState() before restoring the tab does not help. Do you have any idea? :)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 630151
(In reply to comment #17)
> (In reply to comment #16)
> > If something is going wrong the test is failing. Do we need to register this
> > cleanup function? Do one of the tests actually fail before we get in here?
> 
> The cleanup function is only necessary because I'm using waitForSaveState()
> later. It's possible that the callback is never fired and so it's better to
> cleanup to prevent cascading failures?!

My point was just that if the callback was failing, it's a bad test. We'd end up with false failures due to an unreliable method of testing, AKA [orange].

> > Using waitForSaveState isn't really necessary here, so let's not do that. I
> > really don't like to use that method unless we really need to. You should be
> > able to just remove, undoClose, and then remove. If triggering getting session
> > state is needed (it shouldn't be here) then you can use ss.getBrowserState.
> 
> I'm using that function because I was rather clueless. When removing that
> function call/wrap I get the following error:

I can excuse cluelessness :) I don't blame you since it certainly seems like it would be useful.

> Exception thrown - [Exception... "'Illegal value' when calling method:
> [nsISessionStore::undoCloseTab]"  nsresult: "0x80070057
> (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_628270.js
> :: test :: line 46"  data: no]
> 
> So it seems adding, removing, unclosing is so quick that it doesn't make it
> into session store? Even calling ss.getBrowserState() before restoring the tab
> does not help. Do you have any idea? :)

So this is because you're closing a tab with only about:blank in it, which we don't remember in recently closed tabs (we were, but that was a bug: bug 581937). Load something like about:robots, listen for the load event on the linkedBrowser, then continue. Closing, then undoClose, then close *should* work synchronously).

I think you'll also want to check that after undoCloseTab the tab has the correct url (you'd want to wait for a load event again). I would say also test for hidden/visible, but undoCloseTab selects the tab immediately, so that's sort of pointless since it becomes visible. The tests you have will pass regardless of this fix (a timeout would indicate a failure, which is fine).
(Assignee)

Updated

7 years ago
Attachment #508181 - Flags: review?(paul)
Attachment #508181 - Flags: review?(ian)
(Assignee)

Comment 20

7 years ago
Created attachment 508571 [details] [diff] [review]
patch v4
Attachment #508181 - Attachment is obsolete: true
Attachment #508571 - Flags: review?(paul)
(Assignee)

Updated

7 years ago
Attachment #508571 - Flags: review?(ian)
Comment on attachment 508571 [details] [diff] [review]
patch v4

r+ Thanks again!
Attachment #508571 - Flags: review?(paul) → review+
Whiteboard: [4b10][needs review ian][needs sessionstore test][hardblocker][has patch] → [4b10][needs review ian][hardblocker][has patch]
(Assignee)

Comment 22

7 years ago
Created attachment 508752 [details] [diff] [review]
patch v5

Test cleaned up because it intermittently failed.
Attachment #508571 - Attachment is obsolete: true
Attachment #508752 - Flags: review?(ian)
Attachment #508571 - Flags: review?(ian)
Comment on attachment 508752 [details] [diff] [review]
patch v5

Again just looking at the test (as Paul knows the other area). Looks good.
Attachment #508752 - Flags: review?(ian) → review+
Whiteboard: [4b10][needs review ian][hardblocker][has patch] → [4b10][needs review ian][hardblocker][has patch][b11]
Whiteboard: [4b10][needs review ian][hardblocker][has patch][b11] → [4b10][hardblocker][has patch][b11][can land]
Pushed http://hg.mozilla.org/mozilla-central/rev/1c4fcf164f18
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [4b10][hardblocker][has patch][b11][can land] → [4b10][hardblocker][b11]
Target Milestone: --- → Firefox 4.0b11
Status: RESOLVED → VERIFIED

Updated

7 years ago
Blocks: 639843
You need to log in before you can comment on or make changes to this bug.