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

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Maniac Vlad Florin (:vladmaniac)]

Attachment: 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

Comment 1

[Kevin Brosnan]

This was marked as fixed in bug 624265.

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

yes, it works on win7, but that's it

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 4

[juan becerra [:juanb]]

Nominating for blocking because of a case of state loss.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 6

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

From a quick look, it seems like something in sessionstore will need to be fixed...

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 8

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

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.

Comment 9

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

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.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 11

[Ian Gilman [:iangilman]]

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).

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2, WIP

Comment 13

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

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).

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Pushed to try.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Passed try.

Comment 16

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

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.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(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? :)

Comment 19

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

(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).

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

Comment 21

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

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

r+ Thanks again!

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

Test cleaned up because it intermittently failed.

Comment 23

[Ian Gilman [:iangilman]]

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

Again just looking at the test (as Paul knows the other area). Looks good.

Comment 24

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

Pushed http://hg.mozilla.org/mozilla-central/rev/1c4fcf164f18