597637 - Trying to restore final tab in closed tab list throws an exception

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Michael Kraft [:morac]]

In the 20100915 trunk load, after closing a bunch of tabs, all of them can be restored except for the last one which throws the following exception:

Error: uncaught exception: [Exception... "'Illegal value' when calling method: [nsISessionStore::undoCloseTab]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://navigator/content/tabbrowser.xml :: undoCloseTab :: line 1550"  data: no]

From what I can tell, the closed tab list is someone getting cleared before final closed tab can be restored.  I'm fairly certain this used to work.  I'm not sure when this broke since I haven't run the SeaMonkey trunk for a while.  I upgraded directly from the 20100722021157 (2.1a3pre) load to the 2010915012425 (2.1b1pre) load so the problem was introduced some time in that time frame (I believe).

Steps to reproduce:
1. Open a bunch of "Home" tabs by middle clicking the home button.
2. Press CTRL-W a few times (I did it three times).
3. Press CTRL-SHIFT-T the same number of times (again I did 3).

Expected results:
1. All closed should be restored.

Actual Results:
1. All tabs restored except the first tab closed (last attempted to restore) which throws an exception.

Error: uncaught exception: [Exception... "'Illegal value' when calling method: [nsISessionStore::undoCloseTab]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://navigator/content/tabbrowser.xml :: undoCloseTab :: line 1550"  data: no]

Comment 1

[Justin Wood (:Callek)]

morac, I can promise to look into this if you can get me at least a 48 hour regression window, do you think you'll have time for that?

Comment 2

[Michael Kraft [:morac]]

I'll try and narrow the window down tomorrow.

Comment 3

[Robert Kaiser]

Hmm, now, "tomorrow" has passed by a bit, any news there?

Comment 4

[Michael Kraft [:morac]]

I had issues reproducing this on my work PC which is where I tried to narrow the range down.  I then got side tracked on a bunch of other issue with SessionStore being fairly broken in Minefield.

Comment 5

[Michael Kraft [:morac]]

Okay I figured out why I couldn't reproduce the problem.  I left out a step in my steps to reproduce.  Between steps 2 and 3, restore session data.  My guess based on that is that the closed tab list in SessionStore is getting out of sync with what's displayed in the browser.  I'll do some digging into this and see if I can see what's going wrong.

Comment 6

[Michael Kraft [:morac]]

Okay, it doesn't always happen on a session restore and it can also happen without a session restore.  Using a session restore where the session doesn't have any closed tabs is the easiest way to trigger it though.  For example closing a bunch of tabs and then pasting the following into the error console:

Components.classes["@mozilla.org/suite/sessionstore;1"].getService(Components.interfaces.nsISessionStore).setBrowserState('{"windows":[{"tabs":[{"entries":[{"url":"about:blank"}],"index":1}],"selected":1}],"selectedWindow":0}');

At this point the _windows[aWindow.__SSi]._closedTabs variable is [], but the Recently Closed Tab list still shows tabs in it.  The _closedTabs variable gets set in Sessionstore:restoreWindow().

Now at this point the _closedTabs variable for the window is [], but the savedBrowsers variable for the window (in tabbrowser.xml) still has the closed tabs.  This is reflected in both the SessionStore:getClosedTabCount and SessionStore:getClosedTabData functions:

Components.classes["@mozilla.org/suite/sessionstore;1"].getService(Components.interfaces.nsISessionStore).getClosedTabCount(Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("navigator:browser"))

Components.classes["@mozilla.org/suite/sessionstore;1"].getService(Components.interfaces.nsISessionStore).getClosedTabData(Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("navigator:browser"))


The problem is that the SessionStore:undoCloseTab function will thrown an exception if the index is out of range of _closedTabs , which is always the case since _closedTabs is empty.

    var closedTabs = this._windows[aWindow.__SSi]._closedTabs;
    if (!(aIndex in closedTabs))
      throw (Components.returnCode = Components.results.NS_ERROR_INVALID_ARG);


Basically this will happen anytime that the _closedTabs array length in SessionStore does not match the savedBrowsers array length in tabbrowser.xml.  Also if the items in the array don't match restoring closed tabs can result in unexpected results.


Either savedBrowsers needs to be updated when the _closedTabs value is restored or undoClosedTabs needs to look in the returned data of getClosedTabsData to find the closed tab to restore.

Comment 7

[[e-mail]]

You can also trigger this by setting the session restore undo depth to less than the tabbrowser undo depth.

Comment 8

[[e-mail]]

Attachment: Proposed patch

* Added helper function to combine the list of sessionstore and browser tabs
  (based on getClosedTabData, which makes the diff look a little odd)
* Switched getClosedTabCount/Data to use the helper function
* undo/forgetClosedTab now use the helper function, and separately remove the
  tab from the sessionstore list.

Comment 9

[[e-mail]]

Pushed changeset bfd1320b39ac to comm-central.