If the last browser window is closed and then reopened, SessionStore's session data get's into a weird state where there can be open (or closed) windows with no tabs. I suspect this is caused by the new TabCandy implementation, but I'm not sure about that. Here's the easiest way to reproduce the problem that I've found. After each step from #2 and on you can type the following into the error console to get the session state: Components.classes["@mozilla.org/browser/sessionstore;1"].getService(Components.interfaces.nsISessionStore).getBrowserState() 1. Open Minefield (I used Gecko/20100820) and load a page into a tab. 2. enable javascript.options.showInConsole preference to see errors. 3. Open the error console and check session state. 4. Close the browser window. Session state changes slightly, but is still okay. 5. In the error console window type "window.open()" to open a new browser window. Session state has changed and _closedWindows is now corrupted. 6. Open the History -> Recently Closed Windows menu. This will fail and the following error will appear in the error console: Error: selectedTab is undefined Source file: chrome://browser/content/browser.js Line: 2109 Here's the before and after session states when I ran my test in a brand new profile with Minefield 20100820 nightly: Before: {"windows":[{"tabs":[{"entries":[{"url":"http://www.mozilla.org/projects/minefield/","title":"Minefield Start Page","ID":0,"formdata":{},"scroll":"0,0"}],"index":1,"hidden":false,"attributes":{"image":"http://www.mozilla.org/favicon.ico"}}],"selected":1,"_closedTabs":[],"width":1080,"height":1156,"screenX":425,"screenY":9,"sizemode":"normal","cookies":[{"host":".mozilla.org","value":"150903082","path":"/","name":"__utmc"}]}],"selectedWindow":1,"_closedWindows":[]} After closing (still good): {"windows":[{"tabs":[{"entries":[{"url":"http://www.mozilla.org/projects/minefield/","title":"Minefield Start Page","ID":0,"formdata":{},"scroll":"0,0"}],"index":1,"hidden":false,"attributes":{"image":"http://www.mozilla.org/favicon.ico"}}],"selected":1,"_closedTabs":[],"width":1080,"height":1156,"screenX":425,"screenY":9,"sizemode":"normal","cookies":[{"host":".mozilla.org","value":"150903082","path":"/","name":"__utmc"}],"title":"Minefield Start Page"}],"selectedWindow":0,"_closedWindows":[]} After reopening (bad - lost the tabs): {"windows":[{"tabs":[{"entries":[],"hidden":false,"attributes":{}}],"selected":1,"_closedTabs":[],"width":1080,"height":1156,"screenX":425,"screenY":9,"sizemode":"normal"}],"selectedWindow":1,"_closedWindows":[{"tabs":[],"selected":1,"_closedTabs":[],"width":1080,"height":1156,"screenX":425,"screenY":9,"sizemode":"normal","cookies":[{"host":".mozilla.org","value":"150903082","path":"/","name":"__utmc"}],"title":"Minefield Start Page"}]} As you can see at this point the _closedWindows object is corrupted since the tab array is empty. I've also seen a corrupted session state when calling getBrowserState() from my add-on during quit-application granted notification stage. The difference is that instead of the _closedWindow object having an empty tab array, it's the windows[0].tabs object that's empty.

Are you on Windows? This is WFM on OS X. On OS X, my "after closing" step has _closedWindows populated. I think that should be the case on every OS...

Yes I'm on Windows XP. Maybe it's specific to Windows? And yes, _closedWindows should be populated since it is with Firefox 3.6.8. I think something is actually getting messed up with the actual windows[0].tab array and then it's getting moved over to the _closedWindows list. The reason I say that is becaue, like I said, the Session Manager add-on calls the getSessionState function at shutdown and starting recently (I think the b5pre builds), the state data shows the same issue when the last browser window is closed (shutting down the browser). I worked around it, but it seemed odd. For example, here's a session that got saved at shutdown. Apparently SessionStore now moves the last closed window to the _closedWindow list prior to quit-application-granted when the last browser window is closed. It didn't used to do this. As you can see both "tabs" arrays are empty. {"windows":[{"tabs":[],"selected":1,"_closedTabs":[],"extData":{"__SessionManagerWindowId":"window1282326774775"},"width":1341,"height":1166,"screenX":260,"screenY":0,"sizemode":"normal","title":"Add-ons Manager"}],"selectedWindow":0,"_closedWindows":[{"tabs":[],"selected":2,"_closedTabs":[],"extData":{"__SessionManagerWindowId":"window1282326716624"},"width":1341,"height":1166,"screenX":260,"screenY":0,"sizemode":"normal","title":"Mozilla Firefox Start Page"}]}

Could this have something to do with the switch to using all JSON? Though that when in in June and I'm fairly certain things broken very recently. There's been a lot of changes to SessionStore recently, in addition to the Tab Candy stuff so I'm not sure where to even begin with this. My guess is one of the recent changes regarding tab collection http://hg.mozilla.org/mozilla-central/log/2d6ead22831c/browser/components/sessionstore/src/nsSessionStore.js

Very unlikely it's the JSON change - that didn't actually change how we handled any data. Tab Candy is possible, though most of the sessionstore changes were just handling of hidden attribute (and the iterations it took to get there). My bet is that it has something to do with app tabs actually (bug 580512 and then the followup bug 587299). I know that these definitely did some work to the actual data with some app tab filtering. There were some quit-application/last-window-closed changes. Would you be willing to test a couple of the nightly builds? 8/12 should have all the tab candy stuff and would be before the bugs I just mentioned. 8/11 would be before tab candy, and then anything between 8-13 & now should have my suspected changes.

Actually on second though I think the issue with comment #2 isn't related. That's caused by That's caused by the new aPinnedOnly parameter in the call to _saveWindowHistory. Basically once you hit quit-application-granted, the session data won't be valid since the tabs can get stripped out becuase of the new aPinnedOnly. I'm guessing this is working as designed, even if it is annoying. I did notice that if I have the "When Minefield starts" setting set to "Show my Windows and Tabs from Last Time", the steps in comment #0 won't reproduce the problem. Not sure why exactly. From what I can tell it looks like the closing part works since the closed window data displays correctly when I call Components.classes["@mozilla.org/browser/sessionstore;1"].getService(Components.interfaces.nsISessionStore).getClosedWindowData() after closing the window. It gets corrupted when a new window is opened. For example, here's the returned data from getClosedWindowData() after a new window is opened in step 5. [{"tabs":[],"selected":4,"_closedTabs":[],"width":1080,"height":1156,"screenX":425,"screenY":9,"sizemode":"normal","cookies":[{"host":".mozilla.org","value":"150903082","path":"/","name":"__utmc"}],"title":"Minefield Start Page"}] I think I found the problem. There's the following code in the onLoad function: else if (this._restoreLastWindow && aWindow.toolbar.visible && this._closedWindows.length && !this._inPrivateBrowsing) { // default to the most-recently closed window // don't use popup windows let state = null; let newClosedWindows = this._closedWindows.filter(function(aWinState) { if (!state && !aWinState.isPopup) { state = aWinState; return false; } return true; }); if (state) { delete state.hidden; //@line 646 "e:\builds\moz2_slave\mozilla-central-win32-nightly\build\browser\components\sessionstore\src\nsSessionStore.js" if (!this._doResumeSession()) //@line 648 "e:\builds\moz2_slave\mozilla-central-win32-nightly\build\browser\components\sessionstore\src\nsSessionStore.js" state.tabs = state.tabs.filter(function (tab) tab.pinned); if (state.tabs.length > 0) { this._closedWindows = newClosedWindows; this._restoreCount = 1; state = { windows: [state] }; this.restoreWindow(aWindow, state, this._isCmdLineEmpty(aWindow, state)); } } // we actually restored the session just now. this._prefBranch.setBoolPref("sessionstore.resume_session_once", false); } The "_restoreLastWindow" variable gets set to true when the last browser window is closed, so opening a new window goes into the code above. I put some dump statements in there and the this._closedWindows gets corrupted right after this line: state.tabs = state.tabs.filter(function (tab) tab.pinned); This is what's stripping out the tabs from the this._closedWindows variable because state and this._closedWindows are pointing to the same object because of this code: let newClosedWindows = this._closedWindows.filter(function(aWinState) { if (!state && !aWinState.isPopup) { state = aWinState; return false; } return true; }); I'm not sure what the code is supposed to be doing, but the end result is it ends up simply stripping the tabs out of the last closed window in the closed window list.

I see that code was added in bug 580512 so that would be it.

I'll also mention that it's probably not a good idea to simply strip out the tab data in general. I see that SessionStore is now doing so if tabs are "pinned" (not sure what that means). The problem you run into is the case where there are no pinned tabs. You end up with session data which looks like: {"windows":[{"tabs":[],"selected":1, ... That is bad because you basically now have a window with 0 tabs in it and the first tab selected. That's what happened to the _closedWindows data here as well. Unless Firefox 4.0 is now allow 0 tabs in a window. Is that the case?

Thanks a lot for tracking this down. This definitely isn't supposed to happen. Idea (probably easier for you since you're on Windows)... Change > let newClosedWindows = this._closedWindows.filter(function(aWinState) { to > let newClosedWindows = this._closedWindows.slice().filter(function(aWinState) { And then if that doesn't work... > state = aWinState; to > state = JSON.parse(JSON.stringify(aWinState)); I'm hoping the first one works, but slice makes a shallow copy, so I bet the objects are still the same. I don't want to have to stringify & parse. (In reply to comment #7) > I'll also mention that it's probably not a good idea to simply strip out the > tab data in general. Well so the stripping is so that if you had app tabs, they would be restored when you open that window, which we decided was expected. > I see that SessionStore is now doing so if tabs are "pinned" (not sure what > that means). The problem you run into is the case where there are no pinned > tabs. You end up with session data which looks like: "pinned" is just how we are denoting "app tabs" > Unless Firefox 4.0 is now allow 0 tabs in a window. Is that the case? No, that is not the case

(In reply to comment #8) > > let newClosedWindows = this._closedWindows.slice().filter(function(aWinState) { Nope this didn't fix it. > > state = JSON.parse(JSON.stringify(aWinState)); This one works. > Well so the stripping is so that if you had app tabs, they would be restored > when you open that window, which we decided was expected. I don't have a problem with that. The problem is what if there are no app tabs? You end up with a session with all the tabs stripped out of the window. The same thing can happen in the _saveWindowHistory function: var tabbrowser = aWindow.gBrowser; var tabs = tabbrowser.tabs; var tabsData = this._windows[aWindow.__SSi].tabs = []; for (var i = 0; i < tabs.length; i++) { if (aPinnedOnly && !tabs[i].pinned) break; tabsData.push(this._collectTabData(tabs[i])); } this._windows[aWindow.__SSi].selected = tabbrowser.mTabBox.selectedIndex + 1; If there's no pinned tabs, then the tabs variable remains [] while the selected tab index is set to whatever tab is currently selected. At that point it's basically a corrupted session since it has a window and no tabs. I've actually notice an additional peculiarities with how this works. It appears the "When Minefield starts" setting now also applies to the case where there are no browser window, but the browser isn't shut down. I'm assuming this was done purposely, but there's something not quite correct. Currently if the "When Minefield starts" is set to "Show my Windows and Tabs from Last Time", then the closed window is restored when a new browser window is open. The old closed window state is loaded into the new window. Different than it worked under 3.6.8, but okay I guess. If "Show my Windows and Tabs from Last Time" is not set, then things work differently depending on whether there are app tabs or not. If there are, then the closed window is restored with only the app tabs. All other tabs are lost and the closed window does not appear in the recently closed windows list. If on the other hand, there are no app tabs, the a new window is open and the original closed window shows up under the recently closed window list (with the fix from comment #8). Without the fix from comment #8, the Recently Closed Window list appears, but generates errors as mentioned in comment #0. In order to fix this I have a few questions on how this is supposed to work: 1. Does the "When Minefield starts" now also apply when Minefield isn't technically starting? Meaning does it apply when a new window is opened after the last browser window is closed? 2. Assuming it does, should the browser remove the last closed window from the recently closed window list if the "Show my Windows and Tabs from Last Time" option isn't set? The way the code currently works is to strip out all non-app tabs unless the "Show my Windows and Tabs from Last Time" is set. If the answer to questions #1 and #2 are both yes, then the fix to this would be to simply set this._closedWindows to newClosedWindows if tab.length is 0 and be done with it. However, if the answer to #2 is no, then not only do we need to fix it to prevent the tabs from being stripped out of the original this._closedWindows, but also handle the case where there are app tabs in the closed window since that causes the tabs in the original closed window to be lost. My guess, simply based on how it's coded is that the answers to both questions are "yes". If so the fix would simply be to add an else clause to the end of the if state on line 644 as such: if (state.tabs.length > 0) { this._closedWindows = newClosedWindows; this._restoreCount = 1; state = { windows: [state] }; this.restoreWindow(aWindow, state, this._isCmdLineEmpty(aWindow, state)); } else this._closedWindows = newClosedWindows; I wouldn't particularly like that change, in fact I think it's a very bad idea since the result can be lost tabs, but at least it would be consistent (tabs are already lost unless they are app tabs).

In an attempt to help avoid headaches on your end, I would actually avoid doing extension work based on the session format right now. It's very likely going to change in a non-trivial way in the next couple weeks. I appreciate the work you put into this so far & hope you don't take it personally when I say I'm punting on this for now. Things with App Tabs and Tab Candy are changing and I'm not completely sure how that's all going to play out. You might want to follow along in bug 588217 and bug 587873, where most of that work is/will be happening. Bug 587873 in particular will have an effect on what the right behavior here is.

Okay I'll hold off and see what results come from it all. I'm hoping that whatever changes are made aren't so drastic as to make the session format in Gecko 2.0 totally incompatible with the existing formats.

Now that bug 587873 is no longer being fixed for Firefox 4.0, it looks like this will need to be fixed separately. I still don't think closing the last browser window should result in lost session data if the "Show my Windows and Tabs from Last Time" setting is not set, unless the browser is actually exiting.

Here's the fix for this. I originally included it in the patch for bug 600545, but it really belongs in this bug. I'm not sure how one could be done since it requires closing the last browser window. According to bug 600545 comment 30, mozmill might work, but I'm not familiar with that. Even if I was, I don't have time to write up a test for this because of prior commitments.

Comment on attachment 482305 [details] [diff] [review] Fix by doing a deep copy It might actually be possible to test this... http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser/browser_354894.js does some finagling do this. I might be able to add a test in there or just write a new one. So r+, but let's not land this yet. I'll get dietrich to review a patch if I write one.

Comment on attachment 482305 [details] [diff] [review] Fix by doing a deep copy > let newClosedWindows = this._closedWindows.filter(function(aWinState) { > if (!state && !aWinState.isPopup) { >- state = aWinState; >+ // perform a deep copy so the original closed window object is not overwritten >+ state = JSON.parse(this._toJSONString(aWinState)); > return false; > } > return true; > }); This actually fails because this._toJSONString isn't defined inside the filter callback. So you actually just want to pass `this` to filter as an additional parameter. I missed this before, but caught it writing the test. Michael - I know you said you'd be out for a while but can't remember until when exactly. If you don't respond soon & I get the test reviewed, I'll just make the change myself.

I'm actually still traveling abroad and won't be back until about November 6th. If you can make the correction I would really appreciate it.

Michael - I hate to do this, but I may actually be scrapping what you have done, even though I r+'ed! While I was writing the test I realized that we missed a case and as a result could be losing a decent amount of data. Take this (assume OS X or Win/Linux where this._doResumeSession() is false): 1. window A with app tabs & normal tabs is closed (it's the last window) 2. we open a new window, which results in... a. take window A from _closedWindows b. strip out unpinned tabs (this data is thrown away!) c. restore just the pinned tabs At this point we still have _closedWindows data (for other windows), but we've completely lost the unpinned tabs from window A. So what I'm proposing (and started working on) is splitting the app tabs out. We restore that into the new window, and then we put the rest of the tabs back into _closedWindows so it could be restored independently if desired. Luckily I wrote something as a part of bug 588482 that essentially does that. So I'm working on that and making the tests I'm writing cover more cases so this behavior is actually covered.

Fix the bigger problem & test it a few different ways

Comment on attachment 486509 [details] [diff] [review] Patch v0.1 >+ // We want to split the window up into pinned tabs and unpinned tabs. >+ // Pinned tabs should be reloaded. If there are any remaining tabs, >+ // they should be added back to _closedWindows. >+ // We'll cheat a little bit and reuse _prepDataForDeferredRestore >+ // even though it wasn't built exactly for this. >+ let [newOpenState, newClosedState] = >+ this._prepDataForDeferredRestore(JSON.stringify({ windows: [closedWindowState] })); >+ >+ // These are our pinned tabs, which we should restore >+ if (newOpenState.windows.length) { >+ newWindowState = newOpenState.windows[0]; >+ delete newWindowState.__lastSessionWindowID; >+ } >+ >+ // In case there were no unpinned tabs, remove the window from _closedWindows >+ if (!newClosedState.windows.length) { >+ this._closedWindows.splice(closedWindowIndex, 1); >+ } >+ // Or update _closedWindows with the modified state >+ else { >+ delete newClosedState.windows[0].__lastSessionWindowID; >+ this._closedWindows[closedWindowIndex] = newClosedState.windows[0]; >+ } code looks fine. i think it'd be clearer if you did s/newOpenState/appTabsState/ and s/newClosedState/regularTabsState/ or something like that. r=me otherwise.

Updated patch for checkin when the tree reopens for non-beta7 blockers

Pushed http://hg.mozilla.org/mozilla-central/rev/e1b537e616f2