Closed Bug 633711 Opened 13 years ago Closed 13 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)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch patch (obsolete) — 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 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.
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)
(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...
(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 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...
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 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 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 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.
I had test fix in rejected patch, can You review it to land all together ?
Attachment #513883 - Flags: review?(neil)
Attachment #513883 - Flags: review?(neil) → review+
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)
Attachment #512722 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
No longer blocks: 726521
Depends on: 726521
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.

Attachment

General

Created:
Updated:
Size: