Closed
Bug 528776
Opened 15 years ago
Closed 15 years ago
getBrowserState considers closed windows as open
Categories
(Firefox :: Session Restore, defect)
Tracking
()
RESOLVED
FIXED
Firefox 3.7a1
People
(Reporter: dao, Assigned: dao)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
8.96 KB,
patch
|
zeniko
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•15 years ago
|
Whiteboard: [orange]
Assignee | ||
Comment 1•15 years ago
|
||
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
Assignee | ||
Comment 2•15 years ago
|
||
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
Comment 3•15 years ago
|
||
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"
Summary: Some browser chrome test leaves two open windows around → Some browser chrome test leaves two open windows around, causing random failures in sessionstore/test/browser/browser_526613.js, then timeout
Assignee | ||
Comment 4•15 years ago
|
||
Added more output to browser_526613.js, hoping this will give us some useful hint:
http://hg.mozilla.org/mozilla-central/rev/ead0f1b18d5f
Comment 5•15 years ago
|
||
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.
Comment 6•15 years ago
|
||
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.
Assignee | ||
Comment 7•15 years ago
|
||
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.
Blocks: 528452
Comment 8•15 years ago
|
||
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)
Assignee | ||
Comment 9•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
Assignee | ||
Comment 11•15 years ago
|
||
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.
Assignee | ||
Comment 12•15 years ago
|
||
New suspect is browser_522545.js.
http://hg.mozilla.org/mozilla-central/rev/8b369c0a2752
Assignee | ||
Comment 13•15 years ago
|
||
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
Comment 14•15 years ago
|
||
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258384767.1258388015.27405.gz
WINNT 5.2 mozilla-central opt test everythingelse
Assignee | ||
Comment 15•15 years ago
|
||
Assignee | ||
Comment 16•15 years ago
|
||
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
Assignee | ||
Comment 17•15 years ago
|
||
Assignee | ||
Comment 18•15 years ago
|
||
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
...
Assignee | ||
Comment 19•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → dao
Comment 20•15 years ago
|
||
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.
Comment 21•15 years ago
|
||
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.
Assignee | ||
Comment 22•15 years ago
|
||
(In reply to comment #21)
> can we print out originalState and check what's inside it
Will do with the next push.
Comment 23•15 years ago
|
||
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258456626.1258460115.32204.gz
on linux.
OS: Windows XP → All
Comment 24•15 years ago
|
||
(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.
Assignee | ||
Comment 25•15 years ago
|
||
It looks like http://hg.mozilla.org/mozilla-central/rev/fa212b6a9d72 fixed bug 518970 and caused this.
Comment 26•15 years ago
|
||
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).
Comment 27•15 years ago
|
||
(In reply to comment #26)
> and in browser_526613.js (2->3).
i meant browser_522545.js
Assignee | ||
Comment 28•15 years ago
|
||
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.
Comment 29•15 years ago
|
||
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
Comment 30•15 years ago
|
||
sorry i forgot to tell this comes from:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258640500.1258643940.3647.gz
Comment 31•15 years ago
|
||
Actually, sessionstore uses sometims this._windows, windows are removed from it when domwindowclosed is notified, that happens largely after win.close() is called.
Comment 32•15 years ago
|
||
for example this seems sensible. but i just took a quick look
Assignee | ||
Comment 33•15 years ago
|
||
This is what I think this should look like. It's untested, though.
Assignee | ||
Comment 34•15 years ago
|
||
morphing... bug 518970 tracks the orange
Comment 35•15 years ago
|
||
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
Assignee | ||
Comment 36•15 years ago
|
||
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.
Assignee | ||
Comment 37•15 years ago
|
||
(In reply to comment #35)
> is unload sync regarding win.close()?
I think it is.
Comment 38•15 years ago
|
||
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.
Assignee | ||
Comment 39•15 years ago
|
||
Well, it's not quite synchronous, but I wouldn't expect the same problems as with domwindowclosed.
Assignee | ||
Comment 40•15 years ago
|
||
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.
Assignee | ||
Comment 41•15 years ago
|
||
(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 :(
Assignee | ||
Comment 42•15 years ago
|
||
I think this works, I've submitted it to the tryserver.
Attachment #413343 -
Attachment is obsolete: true
Assignee | ||
Comment 43•15 years ago
|
||
I wonder if this would fix bug 526194 without the patch there and without bug 526613. I think it should if it works reliably.
Comment 44•15 years ago
|
||
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.
Assignee | ||
Comment 45•15 years ago
|
||
DOMWindowClose must be sync, because it's cancellable.
Comment 46•15 years ago
|
||
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.
Assignee | ||
Comment 47•15 years ago
|
||
See bug 529875, I don't think we want that notification at all.
Comment 48•15 years ago
|
||
if we remove the notification, it's fine to remove its fix, obviously, but should be done after, not before.
Assignee | ||
Comment 49•15 years ago
|
||
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.
Assignee | ||
Comment 50•15 years ago
|
||
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.
Attachment #413341 -
Attachment is obsolete: true
Attachment #413380 -
Attachment is obsolete: true
Attachment #413802 -
Flags: review?(zeniko)
Updated•15 years ago
|
Attachment #413802 -
Flags: review?(zeniko) → review+
Comment 51•15 years ago
|
||
Comment on attachment 413802 [details] [diff] [review]
discard stale windows before messing with the browser state
r+=me with s/Widnow/Window/g.
Assignee | ||
Comment 52•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Comment 53•15 years ago
|
||
Backed out to fix orange, the tree is also restricted to blocking bugs or approved patches.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 54•15 years ago
|
||
hm, would have been useful to report which kind of orange this caused...
Comment 55•15 years ago
|
||
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
Comment 56•15 years ago
|
||
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
Comment 57•15 years ago
|
||
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".
Assignee | ||
Comment 58•15 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 59•15 years ago
|
||
Are there steps to reproduce this bug from a QA perspective (i.e. How can I verify?)?
You need to log in
before you can comment on or make changes to this bug.
Description
•