Closed
Bug 633711
Opened 14 years ago
Closed 14 years ago
Port relevant parts from Bug 589246 [Closed window state getting corrupted when closing and reopening last browser window without exiting browser]
Categories
(SeaMonkey :: Session Restore, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: misak.bugzilla, Assigned: misak.bugzilla)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
neil
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
From parent bug: 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.
Attachment #511937 -
Flags: review?(neil)
Comment 1•14 years ago
|
||
Comment on attachment 511937 [details] [diff] [review] patch >- Services.obs.notifyObservers(aWindow, NOTIFY_WINDOWS_RESTORED, ""); >+ Services.obs.notifyObservers(null, NOTIFY_WINDOWS_RESTORED, ""); Don't want this change of course. > else if (this._restoreLastWindow && aWindow.toolbar.visible && > this._closedWindows.length && this._doResumeSession()) { The main change is all within this block, right? So _doResumeSession is true? >+ if (!this._doResumeSession()) { So how can it ever be false? >+ let [appTabsState, normalTabsState] = >+ this._prepDataForDeferredRestore(JSON.stringify({ windows: [closedWindowState] })); >+ >+ // These are our pinned tabs, which we should restore >+ if (appTabsState.windows.length) { >+ newWindowState = appTabsState.windows[0]; >+ delete newWindowState.__lastSessionWindowID; >+ } >+ >+ // In case there were no unpinned tabs, remove the window from _closedWindows Nit: the blank lines contain spaces.
Assignee | ||
Comment 2•14 years ago
|
||
Fixed all nits, and disabled some tests that seems to be related. It disabled on Firefox too.
Attachment #511937 -
Attachment is obsolete: true
Attachment #512722 -
Flags: review?(neil)
Attachment #511937 -
Flags: review?(neil)
Comment 3•14 years ago
|
||
(In reply to comment #1) >(From update of attachment 511937 [details] [diff] [review]) >> else if (this._restoreLastWindow && aWindow.toolbar.visible && >> this._closedWindows.length && this._doResumeSession()) { >The main change is all within this block, right? So _doResumeSession is true? >>+ if (!this._doResumeSession()) { >So how can it ever be false? Still hoping for an answer to this question...
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #3) > (In reply to comment #1) > >(From update of attachment 511937 [details] [diff] [review]) > >> else if (this._restoreLastWindow && aWindow.toolbar.visible && > >> this._closedWindows.length && this._doResumeSession()) { > >The main change is all within this block, right? So _doResumeSession is true? > >>+ if (!this._doResumeSession()) { > >So how can it ever be false? > Still hoping for an answer to this question... I changed patch and removed that check: - this._closedWindows.length && this._doResumeSession()) { + this._closedWindows.length) { As Your question is quite right :)
Comment 5•14 years ago
|
||
Comment on attachment 512722 [details] [diff] [review] fix nits, modify and disable some tests OK, so most of this seems to be dealing with resuming pinned tabs even when the session isn't set to restore, which confused me. The one thing I did notice was that you added a parameter to the call to _isCmdLineEmpty but there are other calls and you obviously have to fix the function too...
Assignee | ||
Comment 6•14 years ago
|
||
Oops :( I'm not sure how i should get our clh though. I also fixed another test which is failing now with this patch. Now sessionstore tests fully passing on build.
Attachment #512722 -
Attachment is obsolete: true
Attachment #513389 -
Flags: review?(neil)
Attachment #512722 -
Flags: review?(neil)
Comment 7•14 years ago
|
||
Comment on attachment 513389 [details] [diff] [review] fix nits, modify even more and disable some tests Sorry, I don't think this change makes any sense at all. Probably best to forget about pinned tabs for the moment and go back to the previous patch.
Attachment #513389 -
Flags: review?(neil) → review-
Comment 8•14 years ago
|
||
Comment on attachment 512722 [details] [diff] [review] fix nits, modify and disable some tests Strictly speaking I don't think the ifdef is necessary because we don't treat closing the last window specially on the Mac in the first place.
Attachment #512722 -
Attachment is obsolete: false
Attachment #512722 -
Flags: review+
Comment 9•14 years ago
|
||
Comment on attachment 512722 [details] [diff] [review] fix nits, modify and disable some tests >+ this.restoreWindow(aWindow, state, this._isCmdLineEmpty(aWindow, state)); But I think we should remove the second parameter to _isCmdLineEmpty here in case it confuses somebody later on.
Assignee | ||
Comment 10•14 years ago
|
||
I had test fix in rejected patch, can You review it to land all together ?
Attachment #513883 -
Flags: review?(neil)
Updated•14 years ago
|
Attachment #513883 -
Flags: review?(neil) → review+
Assignee | ||
Comment 11•14 years ago
|
||
Comment on attachment 513883 [details] [diff] [review] same as r+ patch, but plus test (Checked in: see comment 11) Pushed: http://hg.mozilla.org/comm-central/rev/c28aff98e52d
Attachment #513883 -
Attachment description: same as r+ patch, but plus test → same as r+ patch, but plus test (Checked in: see comment 11)
Assignee | ||
Updated•14 years ago
|
Attachment #512722 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Comment 12•13 years ago
|
||
Arf, I committed a patch with a wrong bug number: http://hg.mozilla.org/comm-central/rev/5d0e20c97905 is bug 726521, not this one :-<
You need to log in
before you can comment on or make changes to this bug.
Description
•