Two things happened before this started: http://hg.mozilla.org/mozilla-central/rev/4f62c9d94957 browser_477657.js, seems to open only one window? http://hg.mozilla.org/mozilla-central/rev/01adc20ea792 browser_526613.js, backed out already http://hg.mozilla.org/mozilla-central/rev/fa212b6a9d72 browser_394759.js

Before the backout of 01adc20ea792: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258216470.1258219721.9160.gz#err0 WINNT 5.2 mozilla-central opt test everythingelse on 2009/11/14 08:34:30 "s: moz2-win32-slave26" Running chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js... TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | Only one browser window should be open initially - Got 3, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | Two windows should exist at this point - Got 3, expected 2 TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | waiting for the current window to become active command timed out: 1200 seconds without output

After the backout: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258254597.1258257623.5121.gz#err1 WINNT 5.2 mozilla-central opt test everythingelse on 2009/11/14 19:09:57 "s: moz2-win32-slave40" TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_459906.js | rich textarea's content correctly duplicated - Got <br>, expected <b>Unique:</b> 1258255338517 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | Only one browser window should be open initially - Got 3, expected 1 buildbot.slave.commands.TimeoutError: command timed out: 1200 seconds without output

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258269469.1258272513.4477.gz WINNT 5.2 mozilla-central opt test everythingelse on 2009/11/14 23:17:49 "s: moz2-win32-slave15"

Added more output to browser_526613.js, hoping this will give us some useful hint: http://hg.mozilla.org/mozilla-central/rev/ead0f1b18d5f

so, the only interesting change is restoring blank state in browser_394759.js, i that's the case that means we are still bad counting windows for some reason, or ss is. I recall that my correct state-complete notification was solving this, so looks like the case.

the difference is that before we were not touching the main window, just closing additional ones, using blank state probably we close the window and open a new blank one. We could go back restoring the oldState, but this means that windows are taking a lot of time to close, and that we are still counting them or entering and exiting PB is restoring them (after 394759, we run the pb test). I highly recommend making the notification reliable and taking the browser-test changes, we are just fighting windmills when we have clear solutions already available.

What does making the notification reliable mean, other than that it would wait for the windows to be destroyed? Note that we're already checking window.closed when counting the windows in browser_526613.js, so this seems different from the original issue in bug 527074.

it will partially cover the case of moving to next test with open windows. But will also ensure that case inside the tests themselves (something that the browser-test change does not)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258285252.1258288657.3305.gz (In reply to comment #8) > it will partially cover the case of moving to next test with open windows. These windows used to be closed when I dealt with that issue in bug 527074, the windows that this test is seeing now don't seem to be closed.

http://hg.mozilla.org/mozilla-central/rev/3f1d79eed5aa This says that both windows are "uninitialized": http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258320487.1258323738.23060.gz http://hg.mozilla.org/mozilla-central/rev/97e018ae4052 This didn't report anything.

New suspect is browser_522545.js. http://hg.mozilla.org/mozilla-central/rev/8b369c0a2752

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258384767.1258388015.27405.gz TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_522545.js | Only one browser window should be open initially - Got 2, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_522545.js | Only one browser window should be open eventually - Got 3, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | Only one browser window should be open initially - Got 3, expected 1 buildbot.slave.commands.TimeoutError: command timed out: 1200 seconds without output

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258384767.1258388015.27405.gz WINNT 5.2 mozilla-central opt test everythingelse

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258410565.1258414870.13003.gz TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_514751.js | Only one browser window should be open initially - Got 2, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_514751.js | Only one browser window should be open eventually - Got 2, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_522545.js | Only one browser window should be open initially - Got 2, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_522545.js | Only one browser window should be open eventually - Got 3, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | Only one browser window should be open initially - Got 3, expected 1

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258430139.1258437448.5032.gz TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_490040.js | Only one browser window should be open initially - Got 2, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_490040.js | Only one browser window should be open eventually - Got 2, expected 1 ...

so, based on comment 16, browser_522545.js is opening a window and not closing it, but it takes originalState and calls setBrowserState, that should bring us back to 2. That means setBrowserState windows count is still broken.

can we print out originalState and check what's inside it (to understand if it's saving a bad state or we are bad counting) that would also at which address points the other spurious window.

(In reply to comment #21) > can we print out originalState and check what's inside it Will do with the next push.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258456626.1258460115.32204.gz on linux.

(In reply to comment #20) > so, based on comment 16, browser_522545.js is opening a window and not closing > it, but it takes originalState and calls setBrowserState, that should bring us > back to 2. > That means setBrowserState windows count is still broken. browser_522545.js shouldn't be opening any additional windows. All the states I'm passing to setBrowserState have a single window so sessionstore should be reusing the open window. If it's not, well that's an additional WTF.

It looks like http://hg.mozilla.org/mozilla-central/rev/fa212b6a9d72 fixed bug 518970 and caused this.

From what you posted in https://bugzilla.mozilla.org/show_bug.cgi?id=518970#c166, that push did not cause us to leave a window open, but the window opens at a different point with or without that changeset. So, the problem of these spurious windows persists even after backing out the above changeset, both in browser_394759.js (1->2) and in browser_526613.js (2->3).

(In reply to comment #26) > and in browser_526613.js (2->3). i meant browser_522545.js

browser_522545.js should never have started with two windows in the first place, so the 2->3 problem isn't a priority to me right now. I want to know where that first window is coming from. I don't know if your push caused that window to open at a different point or if it's actually a different window. In any case, the result in bug 518970 comment 166 indicates that the problem is now somewhere in browser_394759.js, and I'm trying to figure out where exactly.

TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | There were 0 popup windows to repoen TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | There were 3 normal windows to repoen TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "notify"' when calling method: [nsITimerCallback::notify]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "<unknown>" data: no]"] TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | number of browser windows after test_behavior NEXT ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | oldState in test_purge has 3 windows instead of 1 TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | {"windows":[{"tabs":[{"entries":[{"url":"http://www.google.com/search?q=test&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:official&client=firefox-a","title":"test - Google Search","ID":232},{"url":"about:blank","ID":234,"scroll":"0,0"}],"index":2,"attributes":{},"_formDataSaved":true}],"selected":1,"_closedTabs":[{"state":{"entries":[{"url":"about:config","ID":216101513,"scroll":"0,0","formdata":{"#textbox":"Another value: 1258641519178"}}],"index":1,"attributes":{},"extData":{"key2":"Value 0.269606324779877"},"_formDataSaved":true},"title":"about:config","image":"","pos":1},{"state":{"entries":[{"url":"about:config","ID":279,"scroll":"0,0","formdata":{"#textbox":"Another value: 1258641519178"}}],"index":1,"attributes":{},"extData":{"key2":"Value 0.269606324779877"},"_formDataSaved":true},"title":"about:config","image":"","pos":1},{"state":{"entries":[{"url":"about:config","ID":280,"owner_b64":"SmIS26zLEdO3ZQBgsLbOywAAAAAAAAAAwAAAAAAAAEY=","scroll":"0,0","formdata":{"#textbox":""}}],"index":1,"attributes":{},"extData":{"Unique key: 1258641519173":"Unique value: 0.2967392590570229"},"_formDataSaved":true},"title":"about:config","image":"","pos":1},{"state":{"entries":[{"url":"about:","title":"About:","ID":216098287,"owner_b64":"NhAra3tiRRqhyKDUVsktxQAAAAAAAAAAwAAAAAAAAEYAAQAAAAAAAeDaHXAvexHTjNAAYLD8FKMHoizADOUR05MxABBLoP1AAAAAAAVhYm91dAAAAAAAAA==","scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true},"title":"About:","image":"","pos":1},{"state":{"entries":[{"url":"chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_346337_sample.html","title":"Test for bug 346337","ID":216098131,"owner_b64":"SmIS26zLEdO3ZQBgsLbOywAAAAAAAAAAwAAAAAAAAEY=","formdata":{"/xhtml:html/xhtml:body/xhtml:input[@name='input']":"","/xhtml:html/xhtml:body/xhtml:input[@name='spaced 1']":"","/xhtml:html/xhtml:body/xhtml:input[3]":"","/xhtml:html/xhtml:body/xhtml:input[@name='check']":false,"/xhtml:html/xhtml:body/xhtml:input[@name='uncheck']":true,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group']":false,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group'][2]":false,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group'][3]":true,"/xhtml:html/xhtml:body/xhtml:select[@name='any']":0,"/xhtml:html/xhtml:body/xhtml:select[2]":[],"/xhtml:html/xhtml:body/xhtml:textarea[@name='testarea']":"","/xhtml:html/xhtml:body/xhtml:textarea[@name='sized one']":"","/xhtml:html/xhtml:body/xhtml:textarea[3]":"","/xhtml:html/xhtml:body/xhtml:input[6]":{"type":"file","fileList":[]},"/xhtml:html/xhtml:body/xhtml:input[7]":{"type":"file","fileList":[]}},"scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true},"title":"Test for bug 346337","image":"","pos":1},{"state":{"entries":[{"url":"chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_346337_sample.html","title":"Test for bug 346337","ID":216098071,"owner_b64":"SmIS26zLEdO3ZQBgsLbOywAAAAAAAAAAwAAAAAAAAEY=","scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true},"title":"Test for bug 346337","image":"","pos":2},{"state":{"entries":[{"url":"http://localhost:8888/browser/browser/components/sessionstore/test/browser/browser_339445_sample.html","title":"Test for bug 339445","ID":242,"scroll":"0,0"}],"index":1,"attributes":{},"storage":{"http://localhost:8888":{"storageTestItem":"SUCCESS"}},"_formDataSaved":true},"title":"Test for bug 339445","image":"","pos":1},{"state":{"entries":[{"url":"http://localhost:8888/browser/browser/components/sessionstore/test/browser/browser_339445_sample.html","title":"Test for bug 339445","ID":216097981,"scroll":"0,0"}],"index":1,"attributes":{},"storage":{"http://localhost:8888":{"storageTestItem":"SUCCESS"}},"_formDataSaved":true},"title":"Test for bug 339445","image":"","pos":2},{"state":{"entries":[{"url":"http://localhost:8888/browser/browser/components/sessionstore/test/browser/browser_248970_b_sample.html","title":"Test for bug 248970","ID":240,"formdata":{"/xhtml:html/xhtml:body/xhtml:input[@name='input']":"1258641515307","/xhtml:html/xhtml:body/xhtml:input[@name='spaced 1']":"0.22839774955894943","/xhtml:html/xhtml:body/xhtml:input[3]":"three","/xhtml:html/xhtml:body/xhtml:input[@name='check']":true,"/xhtml:html/xhtml:body/xhtml:input[@name='uncheck']":false,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group']":false,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group'][2]":true,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group'][3]":false,"/xhtml:html/xhtml:body/xhtml:select[@name='any']":2,"/xhtml:html/xhtml:body/xhtml:select[2]":[1,3],"/xhtml:html/xhtml:body/xhtml:textarea[@name='testarea']":"","/xhtml:html/xhtml:body/xhtml:textarea[@name='sized one']":"Some text... 0.5079034254651983","/xhtml:html/xhtml:body/xhtml:textarea[3]":"Some more text\u000aThu Nov 19 2009 06:38:35 GMT-0800 (Pacific Standard Time)","/xhtml:html/xhtml:body/xhtml:input[6]":{"type":"file","fileList":["/dev/null"]}},"scroll":"0,0"}],"index":1,"attributes":{},"extData":{"key1":"Value 0.3732441020899212"},"_formDataSaved":true},"title":"Test for bug 248970","image":"","pos":1},{"state":{"entries":[{"url":"http://localhost:8888/browser/browser/components/sessionstore/test/browser/browser_248970_b_sample.html","title":"Test for bug 248970","ID":216097840,"formdata":{"/xhtml:html/xhtml:body/xhtml:input[@name='input']":"1258641515307","/xhtml:html/xhtml:body/xhtml:input[@name='spaced 1']":"0.22839774955894943","/xhtml:html/xhtml:body/xhtml:input[3]":"three","/xhtml:html/xhtml:body/xhtml:input[@name='check']":true,"/xhtml:html/xhtml:body/xhtml:input[@name='uncheck']":false,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group']":false,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group'][2]":true,"/xhtml:html/xhtml:body/xhtml:p/xhtml:input[@name='group'][3]":false,"/xhtml:html/xhtml:body/xhtml:select[@name='any']":2,"/xhtml:html/xhtml:body/xhtml:select[2]":[1,3],"/xhtml:html/xhtml:body/xhtml:textarea[@name='testarea']":"","/xhtml:html/xhtml:body/xhtml:textarea[@name='sized one']":"Some text... 0.5079034254651983","/xhtml:html/xhtml:body/xhtml:textarea[3]":"Some more text\u000aThu Nov 19 2009 06:38:35 GMT-0800 (Pacific Standard Time)","/xhtml:html/xhtml:body/xhtml:input[6]":{"type":"file","fileList":["/dev/null"]}},"scroll":"0,0"}],"index":1,"attributes":{},"extData":{"key1":"Value 0.3732441020899212"},"_formDataSaved":true},"title":"Test for bug 248970","image":"","pos":2}],"_hosts":{"www.google.com":true},"width":994,"height":986,"screenX":4,"screenY":4,"sizemode":"normal","cookies":[{"host":"www.google.com","value":"Q0=dGVzdA","path":"/search","name":"SS"}],"extData":{}},{"tabs":[],"selected":0,"_closedTabs":[]},{"tabs":[{"entries":[{"url":"about:config","ID":272,"owner_b64":"SmIS26zLEdO3ZQBgsLbOywAAAAAAAAAAwAAAAAAAAEY=","scroll":"0,0","formdata":{"#textbox":""}}],"index":1,"attributes":{},"_formDataSaved":true},{"entries":[{"url":"about:mozilla","ID":273,"owner_b64":"NhAra3tiRRqhyKDUVsktxQAAAAAAAAAAwAAAAAAAAEYAAQAAAAAAAS8nfAAOr03buTZBMmukiq4HoizADOUR05MxABBLoP1AAAAAAAVhYm91dAAAAAdtb3ppbGxh4NodcC97EdOM0ABgsPwUoweiLMAM5RHTkzEAEEug/UAAAAAADm1vei1zYWZlLWFib3V0AAAAB21vemlsbGEAAAA=","scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true},{"entries":[{"url":"about:buildconfig","ID":274,"owner_b64":"NhAra3tiRRqhyKDUVsktxQAAAAAAAAAAwAAAAAAAAEYAAQAAAAAAAS8nfAAOr03buTZBMmukiq4HoizADOUR05MxABBLoP1AAAAAAAVhYm91dAAAAAtidWlsZGNvbmZpZ+DaHXAvexHTjNAAYLD8FKMHoizADOUR05MxABBLoP1AAAAAAA5tb3otc2FmZS1hYm91dAAAAAtidWlsZGNvbmZpZwAAAA==","scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true}],"selected":1,"_closedTabs":[],"_hosts":{},"width":994,"height":986,"screenX":26,"screenY":0,"sizemode":"normal","title":"about:config"}],"selectedWindow":1,"_closedWindows":[{"tabs":[{"entries":[{"url":"http://window0.example.com/","ID":299,"scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true},{"entries":[{"url":"about:blank","ID":300,"scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true}],"selected":1,"_closedTabs":[],"_hosts":{"window0.example.com":true},"width":994,"height":986,"screenX":26,"screenY":0,"sizemode":"normal","title":"Problem loading page"},{"tabs":[{"entries":[{"url":"http://window1.example.com/","ID":297,"scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true},{"entries":[{"url":"about:blank","ID":298,"scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true}],"selected":1,"_closedTabs":[],"_hosts":{"window1.example.com":true},"width":994,"height":986,"screenX":26,"screenY":0,"sizemode":"normal","title":"Problem loading page"},{"tabs":[{"entries":[{"url":"http://window2.example.com/","ID":295,"scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true},{"entries":[{"url":"about:blank","ID":296,"scroll":"0,0"}],"index":1,"attributes":{},"_formDataSaved":true}],"selected":1,"_closedTabs":[],"_hosts":{"window2.example.com":true},"width":994,"height":986,"screenX":26,"screenY":0,"sizemode":"normal","title":"Problem loading page"}]} TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | 1 tab was removed TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | The correct tab was removed TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | The correct tab was remembered TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | Selected tab has changed TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | The window title was correctly updated TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | 2 tabs were removed TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | The correct tabs were removed TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | The correct tabs were remembered TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | Selected tab has changed TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | The window title was correctly updated TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | The correct number of tabs were removed, and the correct ones TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | All tabs to be forgotten were indeed removed TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | number of browser windows after test_purge - Got 2, expected 1 Running chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js... TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | Only one browser window should be open initially - Got 2, expected 1 TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | sessionstore.js was correctly removed: true TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | sessionstore.js is being written TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | Private Browsing is disabled TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | Correctly set window count TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | Opening new window

sorry i forgot to tell this comes from: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258640500.1258643940.3647.gz

Actually, sessionstore uses sometims this._windows, windows are removed from it when domwindowclosed is notified, that happens largely after win.close() is called.

for example this seems sensible. but i just took a quick look

This is what I think this should look like. It's untested, though.

morphing... bug 518970 tracks the orange

is unload sync regarding win.close()? since this is undoing the set-browser-state-complete notification change, and if it's not sync won't solve the issue

Comment on attachment 413341 [details] [diff] [review] check .closed >--- a/browser/components/sessionstore/src/nsSessionStore.js >+++ b/browser/components/sessionstore/src/nsSessionStore.js >@@ -1741,7 +1741,7 @@ SessionStoreService.prototype = { > var total = [], windows = []; > var nonPopupCount = 0; > var ix; >- for (ix in this._windows) { >+ for (ix in this._windows && !this._windows[ix].closed) { > total.push(this._windows[ix]); > windows.push(ix); > if (!this._windows[ix].isPopup) This wouldn't put these windows in the closed windows list, unless I'm missing something. sessionstore browser chrome tests passed with my patch, I've submitted it to the tryserver too.

(In reply to comment #35) > is unload sync regarding win.close()? I think it is.

my only concern is that we have to solve this situation: win.close() | some code enumerating windows | ss._onClose() so if unload is sync everything is fine. otherwise we are just reducing a bit the time during which in-the-middle code runs.

Well, it's not quite synchronous, but I wouldn't expect the same problems as with domwindowclosed.

unload seems to come from nsDocShell::Destroy, so I'm not sure anymore. There's also beforeunload, but I don't think that's appropriate for this.

(In reply to comment #40) > unload seems to come from nsDocShell::Destroy, so I'm not sure anymore. Yeah, I think domwindowclosed comes only shortly (if not directly) after this :(

I think this works, I've submitted it to the tryserver.

I wonder if this would fix bug 526194 without the patch there and without bug 526613. I think it should if it works reliably.

i don't understand why you want to get rid of bug 528451 at any cost, your patch does not look like needing that, nor it is related to this specific bug. Unless DOMWindowClose is completely sync, but nobody can be sure it will stay sync in future, if anything changes there this bug (And the blocker pb bug) will just come back.

DOMWindowClose must be sync, because it's cancellable.

But the "complete" notification will again fire when windows are marked closed but not actually closed, so this change regresses bug 528451. That's ok if module owner wants to lose that, but should not be done silently in a patch that does not need that change. I don't want to look pedantic, just trying to understand if we want good notifications or somehow-good notifications, if the somehow-good one is fine for the module owner, i'll just stop pointing out useless things.

See bug 529875, I don't think we want that notification at all.

if we remove the notification, it's fine to remove its fix, obviously, but should be done after, not before.

Even if the notification stayed, the fix from bug 528451 would just be adding unneeded complexity once windows marked as closed are handled correctly anyway.

Ok, this is simpler than using the nsIThreadManager (which was needed because DOMWindowClose is cancellable) and also handles entirely synchronous window.close() & ss.getBrowserState() calls. Added a test for that.

Comment on attachment 413802 [details] [diff] [review] discard stale windows before messing with the browser state r+=me with s/Widnow/Window/g.

Backed out to fix orange, the tree is also restricted to blocking bugs or approved patches.

hm, would have been useful to report which kind of orange this caused...

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1260676058.1260677468.11621.gz#err0 OS X 10.5.2 mozilla-central opt test everythingelse on 2009/12/12 19:47:38 s: bm-xserve18 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | number of open browser windows according to getBrowserState - Got 2, expected 1

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1260689860.1260691783.3433.gz WINNT 5.2 mozilla-central opt test everythingelse on 2009/12/12 23:37:40 s: win32-slave38 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_526613.js | number of open browser windows according to getBrowserState - Got 2, expected 1

I filed bug 534489 explicitly about the failures noted in comment 55 && 56, since we've had that failure at least three times during the past day, which makes me think it's a distinct regression from what this bug here was tracking (since there haven't been any test failure reports here for almost a month) Also, note that comment 55 & 56 have "Got 2" [open windows], whereas the last time that test was reported as failing here, it had "Got 3".

Are there steps to reproduce this bug from a QA perspective (i.e. How can I verify?)?