as subject, b-c tests should wait for the notification before going on.

diff -w for review

i forgot to change the capture to false in removeEventListener, this does.

this should not need a diff -w, also added comment on the low interval

a diff -w would not help readability here, sorry. this applies on top of current dao's changes.

notice if we don't take bug 528451 will make sense to change waitForBrowserState to actually enumerate windows, check closed status and wait for all of them to be correctly closed.

(In reply to comment #6) > notice if we don't take bug 528451 will make sense to change > waitForBrowserState to actually enumerate windows, check closed status and wait > for all of them to be correctly closed. You could do this for all tests that close windows, though. We should probably do this in browser-test.js instead.

sounds reasonable, so browser-test should enumerate all windows and wait till there are still windows with .closed == true.

It should in fact make sure that there's only one window, i.e. the one that it's running in.

i'll take a look and file a bug against the test harness.

filed bug 528469

Comment on attachment 412181 [details] [diff] [review] browser_394759.js diff -w I'm not quite sure I understand all of these changes: > newWin2.gBrowser.addEventListener("SSTabRestored", function(aEvent) { > newWin2.gBrowser.removeEventListener("SSTabRestored", arguments.callee, true); > >+ executeSoon(function() { > is(newWin2.gBrowser.tabContainer.childNodes.length, 2, > "The window correctly restored 2 tabs"); Why is executeSoon needed here? >- }, true); >+ }, false); Why's that - and shouldn't you also adjust the corresponding removeEventListener call? >- // restore pre-test state >- ss.setBrowserState(oldState); >- executeSoon(callback); >+ // Restore blank state. What was wrong with the pre-test state?

Comment on attachment 412185 [details] [diff] [review] browser_394759_privatebrowsing.js >+ // NOTE: We don't use 0 because on Windows the file could be locked for some >+ // time after we remove it, so we give it some breath. In that case, you'll have to insert an explicit timeout yourself, as setting the interval to 500 could still cause the session to be saved immediately if the last save operation happened more than half a second ago (a delay you might actually get when waiting for windows to open/close). Or does the test make sure that this can never happen?

Comment on attachment 412186 [details] [diff] [review] browser_526613.js This looks fine (maybe except for the oldState issue). At this point I'm just wondering, whether we could get waitForBrowserState to a place from where all SessionStore tests can use it without having to redeclare it for all setBrowserState related tests. Can we?

(In reply to comment #12) > (From update of attachment 412181 [details] [diff] [review]) > I'm not quite sure I understand all of these changes: > > > newWin2.gBrowser.addEventListener("SSTabRestored", function(aEvent) { > > newWin2.gBrowser.removeEventListener("SSTabRestored", arguments.callee, true); > > > >+ executeSoon(function() { > > is(newWin2.gBrowser.tabContainer.childNodes.length, 2, > > "The window correctly restored 2 tabs"); > > Why is executeSoon needed here? it gets rid of some console exception, i can avoid it, but it's usually fine to run after an event has been completely served. > >- }, true); > >+ }, false); > > Why's that - and shouldn't you also adjust the corresponding > removeEventListener call? just that we don't need capture here. yes i've commented before about the removeEventListener, it is fixed in the non-diffw patch > >- // restore pre-test state > >- ss.setBrowserState(oldState); > >- executeSoon(callback); > >+ // Restore blank state. > > What was wrong with the pre-test state? just that what this is trying to do is cleanup, so i don't understand why losing time saving what we expect to be a blank state. also avoid polluting next tests if something goes wrong.

(In reply to comment #13) > (From update of attachment 412185 [details] [diff] [review]) > >+ // NOTE: We don't use 0 because on Windows the file could be locked for some > >+ // time after we remove it, so we give it some breath. > > In that case, you'll have to insert an explicit timeout yourself, as setting > the interval to 500 could still cause the session to be saved immediately if > the last save operation happened more than half a second ago (a delay you might > actually get when waiting for windows to open/close). Or does the test make > sure that this can never happen? i could set the interval to a really large value at test start. yes it is not rock-solid, should cover most of the cases though.

(In reply to comment #14) > (From update of attachment 412186 [details] [diff] [review]) > This looks fine (maybe except for the oldState issue). At this point I'm just > wondering, whether we could get waitForBrowserState to a place from where all > SessionStore tests can use it without having to redeclare it for all > setBrowserState related tests. Can we? there is a gbug filed to have head_ files for b-c tests, but actually they don't exist... i think it's feasible but looks like out of scope here

FYI I'm using waitForBrowserState in tests for bug 522545, slightly modified (just put services outside since I was already using ss anyway). Might it be better to just leave it the same as these for consistency's sake? (In reply to comment #17) > there is a gbug filed to have head_ files for b-c tests, but actually they > don't exist... i think it's feasible but looks like out of scope here That'd be great. Out of scope here, but if we do this, we should file a followup to move this to a head_ file with dependency on that other bug.

Comment on attachment 412181 [details] [diff] [review] browser_394759.js diff -w Fine, r+=me with the empty line inside the blankState removed.

(In reply to comment #16) > i could set the interval to a really large value at test start. In that case, there'd be no difference between 0 and 500, as they'd both lead to an immediate save operation, wouldn't they?

(In reply to comment #20) > (In reply to comment #16) > > i could set the interval to a really large value at test start. > > In that case, there'd be no difference between 0 and 500, as they'd both lead > to an immediate save operation, wouldn't they? ha, you're right, well ideally the test should not depend on the status of the file. but looks like this in an interesting point to test, the file should not stay locked for eternity. from one side i'd like to create a polling waitForSessionStoreFile, from the other side i know people don't appreciate SetTimeout calls (looks like they create problems to valgrind and other memory analyzers and so we should reduce their usage). So i could wait for the file, or discard file testing. What do you think is better in this case?

(In reply to comment #19) > (From update of attachment 412181 [details] [diff] [review]) > Fine, r+=me with the empty line inside the blankState removed. which empty line? i don't see empty lines inside blankState...

i'll remove the blank line AFTER blankState, if that's not what you mean i'll push a followup with the last missing test changes.

(In reply to comment #21) > (In reply to comment #20) > So i could wait for the file, > or discard file testing. What do you think is better in this case? otherwise, we could detect failures in asyncCopy and fire a second delayed copy if the first fails. we had a couple failures where asyncCopy was failing and we never received the write-complete notification.

browser_394759.js http://hg.mozilla.org/mozilla-central/rev/01adc20ea792 browser_526613.js http://hg.mozilla.org/mozilla-central/rev/fa212b6a9d72

this only handles waitForBrowserState, i'll file a separate bug about locked files.

filed bug 528705

There's a new failure in browser_526613.js now that I haven't seen before. I suggest backing this out and let the test settle for a few days so that it's easier to separate pre-existing problems from new ones.

second occurrence: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258234503.1258237617.15734.gz

backed out the browser_526613.js patch: http://hg.mozilla.org/mozilla-central/rev/0d98ff9aef32

The failure is still there, which leaves the browser_394759.js change as a possible cause. I filed bug 528776 for that.

(In reply to comment #21) > So i could wait for the file, > or discard file testing. What do you think is better in this case? If this is indeed causing orange, what about repeating the test until it succeeds (but at most twice) - and adding a setTimeout between the two? Then the timeout would only be needed if the file indeed happens to be locked or the test is about to fail anyway. (In reply to comment #22) > which empty line? i don't see empty lines inside blankState... Right, I meant the one after the last waitForBrowserState call.

I don't exactly understand what's going on, but I think we should backout the browser_394759.js change for bug 528776, if only to figure out whether it really caused the new intermittent failure. Right now I'm blindly trying to log data from the stale windows in order to figure out where they come from, but I don't know if that will succeed.

(In reply to comment #31) > The failure is still there, That wasn't quite correct, the "Two windows should exist at this point - Got 3, expected 2" failure went away.

(In reply to comment #33) > I don't exactly understand what's going on, but I think we should backout the > browser_394759.js change for bug 528776, if only to figure out whether it > really caused the new intermittent failure. Right now I'm blindly trying to log > data from the stale windows in order to figure out where they come from, but I > don't know if that will succeed. The analysis so far clearly discharges browser_394759.js. I think browser_522545.js is likely the cause, as it also landed recently and runs directly before browser_526613.js.

i'd like to push the 3 changes (Well, 2 at this point), and we can move on after that with new discoveries, but i want to know if that could hurt your current analysis first.

I would like to know if the current browser_394759_privatebrowsing.js orange is fixed before we push changes to the test, in order to avoid situations where we can't tell whether changes helped or hurt. I still don't know if the browser_526613.js change was malicious. As I said, one failure disappeared with the backout, but it might just have been a different manifestation of the first failure (bug 528776), so it would be great if we could fix the first failure before relanding this.

note: all changes are currently backed out and waiting to define what's causing to open (or consider open) additional windows. If this ends up being a way to activate that behavior, we can follow code. this comment https://bugzilla.mozilla.org/show_bug.cgi?id=528699#c44 makes me think about blankState. is it possibile that using such blank state with ss is causing issues in certain situations, confusing SS or PB?

btw, that comment talks about empty tabs entries, that is not exactly this case.

since bug 528776 is reviewed and landing and is removing waiting for closed windows from sessionstore-browser-state-restored, as introduced by bug 528451, this bug is going to be wontfix, since there would be no added benefit in waiting a sync notification.

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

I filed bug 534489 on the failure noted in comment 41.

since bug 528776 has removed my fix to fire the notification only after all windows have been closed, this test fixes are now useless, they would just run synchronously. on the other side all pushed changes have already been backed out, so i'm just closing.