This is mostly for session restore component, so I'm unsure for category. I know that our restoreTab way is much better, but it's really become pain in the ass in session restore world. Until today it's just makes me uncomfortable to port some tests and minor problems like bug 478707. But today i discovered that our restoreTab also leads to incompatibility and thus to different behavior in event handling, which will make extension developers life harder. Specifically our restoreTab doesnt provide SSTabRestoring/SSTabRestored event dispatching and handling. So i'm asking Council to think about replacing our restoreTab. I'll make patch, when You decide to accept it. That will make SeaMonkey Session Restore devs life much easier.

Well, there is no answer, so i came up with the patch.

Do you have a list of differences between the two undo close tab implementations? I don't see any obvious API differences in the patch.

Well, the old one has ability to disable undo, the new one restores tab in it's original position. Old saves closed tabs in savedBrowsers, new - in closedtabs. Thats it. New one fixes bug 478707, obsoletes Michael Kraft fix (don't remember bug #). New doesn't break sessionstore events flow. Just using it, i was able to pass most of session store test, that was fail with old one. And it makes porting much easier.

(In reply to comment #3) > the new one restores tab in its original position. I wonder how that works if we start with 4 tabs, close tab 3, then close tab 2, then undo the tab we closed first. > New doesn't break sessionstore events flow. We could certainly mimic more of the sessionstore events. > I was able to pass most of session store test, that was fail with old one Do you have a list?

(In reply to comment #4) > (In reply to comment #3) > > the new one restores tab in its original position. > I wonder how that works if we start with 4 tabs, close tab 3, then close tab 2, > then undo the tab we closed first. It uses browser.moveTabTo to the job. So basically - I don't know what happen, but nothing terrible IMHO. > We could certainly mimic more of the sessionstore events. > I went to big trouble to get tests that using SSTabRestored event to work because of asynchronous nature of our restoreTab, and this is only one of troubles. > > I was able to pass most of session store test, that was fail with old one > Do you have a list? Well it's a bit complicated right now to get exact list, but i have all-the-ss-tests patch i'm working on and from 5 to 7 are failing. Some of new 1.9.2 features i'm working now failing too. With this patch applied I noticed only 1, which maybe heavily modified/broken by myself. If someone much more experienced than me steps in and make things work with our restoreTab, I'll be happy. In today's situation picking up some of FF "ugliness", seems good price for our development simplicity.

Do you think it would be worthwhile going for a combined approach? 1. tabbrowser code should use browser.sessionstore.max_browsers_undo (tests using sessionstore-specific code should set this to zero) 2. sessionstore code should see if tabbrowser has any browsers saved if so it should hand off the restore to the tabbrowser otherwise it uses its internal restore code 3. undo close tab menuitem needs to query sessionstore for closed tabs

It looks promising, but also will break event flow, because of asynchronous restoreTab. What if: 1. use sessionstore's undoclosetab if ss is enabled/packaged/working. 2. fall back to SM old restoreTab otherwise. 3. add as a bonus to ss ability to disable undo as in our old implementation.

Neil, what You decided ?

Basically what this patch does is to create a separate preference browser.tabs.max_tabs_undo which controls the tabbed browser's saved cache of browser elements. Session store then peeks into the cache and we try to restore a cached browser if we can otherwise we fall back on session store. I added an undoCloseTab API to tabbrowser which is used by the internal context menu as well as the File menu and shortcut key. (The shortcut key doesn't know if there is anything to undo so I made a basic check.) I saved the browser and history in the tabData used by session store. I had to rename the property names so I could exclude them from the JSON. Misak, how many tests does this pass, both with and without setting browser.tabs.max_tabs_undo to zero? (Tests that work with it nonzero would be preferred as we then get to test restore tab too.)

Thanks for the patch Neil! I just have a quick question, which function in tests should use ? restoreTab or UndoCloseTab. Also look at screenshot, there is a little problem with closed tabs list.

(In reply to comment #10) > I just have a quick question, which function in tests should use undoCloseTab (the tabbrowser just calls the session store version). > Also look at screenshot, there is a little problem with closed tabs list. Strange, I'm sure it worked for me locally.

I applied patch on my comm-central trunk and ran tests: On clean tree without any patches: Browser Chrome Test Summary Passed: 727 Failed: 19 Todo: 12 *** End BrowserChrome Test Results *** INFO | automation.py | Application ran for: 0:02:51.282449 INFO | automation.py | Reading PID log: /tmp/tmpySfV3Epidlog == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->| Per-Inst Leaked Total Rem Mean StdDev Total Rem Mean StdDev 0 TOTAL 28 0 11765942 0 ( 3316.72 +/- 4987.11) 20066423 0 ( 7231.12 +/- 12569.90) mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/dom/tests/browser/browser_focus_steal_from_chrome.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug524365.js | Exception thrown at chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug524365.js:60 - TypeError: tabState.disallow is undefined TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_ApplicationPrefs.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after opening 1 additional tab - Got 4, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after opening a second additional tab - Got 5, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking index after moving tab - Got 4, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after closing 2 tabs - Got 3, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug399606.js | Exception thrown at chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug399606.js:74 - ReferenceError: XPCOMUtils is not defined TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug471962.js | TypeError: gBrowser.contentDocument.getElementById("postForm") is null TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should be all the lightweight themes and the default theme and the template - Got 6, expected 5 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got 2, expected {972ce4c6-7e08-4474-a285-3208198ce6fd} TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got 3, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got {972ce4c6-7e08-4474-a285-3208198ce6fd}, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have disabled lightweight themes - Got [object Object], expected null TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should not be able to switch to the current theme TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have changed theme - Got 3, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have updated the list - Got 5, expected 4 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have added the theme - Got 2, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have updated the list - Got 5, expected 4 With first patch: Browser Chrome Test Summary Passed: 696 Failed: 22 Todo: 12 *** End BrowserChrome Test Results *** INFO | automation.py | Application ran for: 0:04:16.302686 INFO | automation.py | Reading PID log: /tmp/tmpZTYnUVpidlog == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->| Per-Inst Leaked Total Rem Mean StdDev Total Rem Mean StdDev 0 TOTAL 27 0 11332774 0 ( 2579.21 +/- 4602.02) 19536520 0 ( 7058.86 +/- 12287.82) nsTraceRefcntImpl::DumpStatistics: 911 entries TEST-PASS | automationutils.processLeakLog() | no leaks detected! mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/dom/tests/browser/browser_focus_steal_from_chrome.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug524365.js | Exception thrown at chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug524365.js:60 - TypeError: tabState.disallow is undefined TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_ApplicationPrefs.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after opening 1 additional tab - Got 4, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after opening a second additional tab - Got 5, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking index after moving tab - Got 4, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after closing 2 tabs - Got 3, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug399606.js | Exception thrown at chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug399606.js:74 - ReferenceError: XPCOMUtils is not defined TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug471962.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug471962.js | TypeError: innerFrame is null TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should be all the lightweight themes and the default theme and the template - Got 6, expected 5 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got 2, expected {972ce4c6-7e08-4474-a285-3208198ce6fd} TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got 3, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got {972ce4c6-7e08-4474-a285-3208198ce6fd}, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have disabled lightweight themes - Got [object Object], expected null TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should not be able to switch to the current theme TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have changed theme - Got 3, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have updated the list - Got 5, expected 4 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have added the theme - Got 2, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have updated the list - Got 5, expected 4 With the second (Your) patch: There is lot of leaks of memory and objects and bunch of tests fail too: mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/dom/tests/browser/browser_focus_steal_from_chrome.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug524365.js | Exception thrown at chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug524365.js:60 - TypeError: tabState.disallow is undefined TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_ApplicationPrefs.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after opening 1 additional tab - Got 4, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after opening a second additional tab - Got 5, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking index after moving tab - Got 4, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/smile/test/browser_Browser.js | Checking length of 'Browser.tabs' after closing 2 tabs - Got 3, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug399606.js | Exception thrown at chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug399606.js:74 - ReferenceError: XPCOMUtils is not defined TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | Timed out TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | b should have scrolled vertically TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | b should have scrolled horizontally TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | c should have scrolled horizontally TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | d should have scrolled vertically TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug471962.js | TypeError: gBrowser.contentDocument.getElementById("postForm") is null TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keydown fired: A - Got 1, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keypress fired: A - Got 2, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keyup fired: A - Got 4, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: A - Got 7, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keydown fired: VK_DOWN - Got 1, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keypress fired: VK_DOWN - Got 2, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keyup fired: VK_DOWN - Got 4, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: VK_DOWN - Got 7, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keydown fired: VK_RETURN - Got 1, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keypress fired: VK_RETURN - Got 2, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keyup fired: VK_RETURN - Got 4, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: VK_RETURN - Got 7, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keydown fired: VK_ENTER - Got 1, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keypress fired: VK_ENTER - Got 2, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keyup fired: VK_ENTER - Got 4, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: VK_ENTER - Got 7, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keydown fired: VK_HOME - Got 1, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keypress fired: VK_HOME - Got 2, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keyup fired: VK_HOME - Got 4, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: VK_HOME - Got 7, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keydown fired: VK_END - Got 1, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keypress fired: VK_END - Got 2, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keyup fired: VK_END - Got 4, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: VK_END - Got 7, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keydown fired: VK_TAB - Got 1, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | keypress fired: VK_TAB - Got 2, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: VK_TAB - Got 3, expected 0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: VK_ESCAPE - Got 0, expected 4 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js | unexpected key events were dispatched or not dispatched: A - Got 0, expected 7 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should be all the lightweight themes and the default theme and the template - Got 6, expected 5 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got 2, expected {972ce4c6-7e08-4474-a285-3208198ce6fd} TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got 3, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Themes should be in the right order - Got {972ce4c6-7e08-4474-a285-3208198ce6fd}, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have disabled lightweight themes - Got [object Object], expected null TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should not be able to switch to the current theme TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have changed theme - Got 3, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have updated the list - Got 5, expected 4 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have added the theme - Got 2, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/extensions/test/browser_bug510909.js | Should have updated the list - Got 5, expected 4 I didnt tried sessionstore tests that not available on trunk yet. I need to patch them to use undoCloseTab. Work on progress.

Sorry, but your comment was too long to be useful, and you still forgot to include a count of the number of tests that failed with my patch. Or rather than a count, it might be helpful to indicate the changes in failing tests. And it's only session store tests that I'm really interested in. When you say you need to patch them to use undoCloseTab, what did they use originally?

Comment on attachment 419247 [details] [diff] [review] Combine sessionstore with restoreTab >+ aTab.tabData = { >+ pos: this.getTabIndex(aTab), >+ panel: oldBrowser.parentNode.id, >+ title: oldBrowser.contentDocument.title || >+ this.getTitleForURI(browser.currentURI) || I replaced browser with oldBrowser here, without it i get many errors. Results in short, both patches fail at 2 same tests. You patch produces memory leaks after tests. Mine not. That's all for now. So both patches are the way to go. Also i didn't played with preference value yet, hope will do it tomorrow.

(In reply to comment #14) > Also i didn't played with preference value yet, hope will do it tomorrow. Sorry I didn't notice that. I'll upload a new patch hopefully with fewer leaks.

Also fixing the oldBrowser bug. Also asking IanN for general review to see whether he can break it. (The only visible change should be that you can now undo close tab in a window restored using undo close window or session restore.)

Comment on attachment 421696 [details] [diff] [review] Hopefully with fewer leaks And I would appreciate if misak could check for leaks again (preferably both with the pref at 3 and at zero) and maybe even try out some session restore tests.

Same leaks present. My patch removes code introduced in Bug 479448, maybe that's why it didn't leak.

With pref set to zero tests behave just like my patch (there is timeout when one test fail), but leaks nsTraceRefcntImpl::DumpStatistics: 929 entries. With pref set to 3, no timeout in same test fail and nsTraceRefcntImpl::DumpStatistics: 927 entries.

Also, closed tabs list doesn't retain after application restart and closed tabs list shows up as on screen-shot above.

Comment on attachment 421239 [details] screenshot, closed tabs menu titles are wrong. Oops, the bug here is that getUndoList should use map, not filter.

(In reply to comment #20) > Also, closed tabs list doesn't retain after application restart Sigh, sessionstore heavily uses the _ prefix in its JSON already...

Does changing to os.addObserver(this, "browser:purge-session-history", true); have any effect on the leaks?

With os.addObserver(this, "browser:purge-session-history", true); nsTraceRefcntImpl::DumpStatistics: 932 entries Also one more test starts failing with this error on console: TEST-INFO | chrome://mochikit/content/browser/suite/common/tests/browser/browser_456342.js | Console message: [JavaScript Error: "uncaught exception: [Exception... "Cannot find interface information for parameter arg 0 [nsISHEntry.layoutHistoryState]" nsresult: "0x80570006 (NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)" location: "JS frame :: file:///home/misak/workspace/src/suite-opt/mozilla/dist/bin/components/nsSessionStore.js :: sss_toJSONString :: line 2610" data: no]"] The test itself only uses one ss API call - getClosedTabData. The bug on screenshot fixed - correct tab titles appearing on closed tabs menu, but both closed tabs and closed windows not retains after application restart. Examining sessionstore.json file in the profile shows that closed tabs and windows list is missing, so they somehow not going to be saved.

Okay, not sure if this is related but: 1) Open SM with a browser window open. 2) Open a second browser window. 3) Open a couple of sites in tabs in that second window. 4) Close second browser window. 5) Right click on tab in first window. Expected Result 1) "Undo Close Tab" should be disabled Actual Result 1) "Undo Close Tab" is enabled and when selected gives in error console: Error: uncaught exception: [Exception... "Not enough arguments [nsISessionStore.getClosedTabCount]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: chrome://navigator/content/tabbrowser.xml :: updatePopupMenu :: line 812" data: no]

Another issue appearing in error console: Error: uncaught exception: [Exception... "Not enough arguments [nsISessionStore.getClosedTabCount]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: chrome://navigator/content/tabbrowser.xml :: updatePopupMenu :: line 812" data: no] Steps to reproduce: 1) Open SM with a browser window open. 2) Open a couple of sites in tabs in that window. 3) Right click on tab in first window. Expected Result 1) "Undo Close Tab" should be disabled. Actual Result 1) "Undo Close Tab" is enabled and gives above error message (though doesn't give error in comment 26 when selected).

Sorry comment 26 should have had the error message: Error: uncaught exception: [Exception... "Operation is not supported" code: "9" nsresult: "0x80530009 (NS_ERROR_DOM_NOT_SUPPORTED_ERR)" location: "file:///mozdev/obj-suite-central-opt/mozilla/dist/bin/components/nsSessionStore.js Line: 2580"]

Steps to reproduce: 1) Open SM with a browser window open. 2) Open a second browser window. 3) Open three sites in three tabs. 4) Close one tab. 5) Close second window. 6) Undo close of window. 7) Undo close of tab. 8) Close a different tab. 9) Try to undo close of tab. Expected result 1) Tab gets restored. Actual result 1) Following message in error console: Error: uncaught exception: [Exception... "'[JavaScript Error: "too much recursion" {file: "file:///mozdev/obj-suite-central-opt/mozilla/dist/bin/components/nsSessionStore.js" line: 2580}]' when calling method: [nsISessionStore::getClosedTabData]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://navigator/content/tabbrowser.xml :: getUndoList :: line 1331" data: yes]

In comment 29, it is when trying to restore the tab from the File menu, it is disabled and mousing over, generates the error message.

Bah, it turns out that I forgot to rediff after fixing comment #22...

Well, only two test failing, but i should check them, i think they are messed a bit. Still leaks nsTraceRefcntImpl::DumpStatistics: 928 entries But everything other is fine now.

With latest patch get following message in error console: SessionStore: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMWindowUtils.getScrollXY]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: file:///mozdev/obj-suite-central-opt/mozilla/dist/bin/components/nsSessionStore.js :: sss_updateTextAndScrollDataForFrame :: line 1387" data: no] Steps to reproduce (sometimes) 1) Start SM with a browser window open 2) Open a few sites in tabs in that window 3) Open a second browser window 4) Open a few sites in tabs in the second window 5) Close one tab in second window 6) Close one tab in first window 7) Close second browser window 8) Look in error console

Tried several times, can't reproduce on Linux.

Comment on attachment 423509 [details] [diff] [review] Proposed patch >@@ -667,17 +667,13 @@ > > // store closed-tab data for undo You can't see it because my diff doesn't have enough context, but the call to _updateTextAndScrollDataForTab that calls _updateTextAndScrollDataForFrame that actually generates the error that you noticed happens the line before here, and is therefore not relevant to this bug. (Sorry.) The error really is unexpected because it means that a frame didn't have a document in it, which is probably never supposed to happen.

Pushed changeset b4e566c1043b to comm-central.

Hmm, how about leaks ?

Comment on attachment 414955 [details] [diff] [review] drop suite restoretab, use sessionstore Dropping review requests on patch that is now obsolete.