528452 - sessionstore tests that use setBrowserState should wait for sessionstore-browser-state-restored

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[Marco Bonardo [:mak]]

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

Comment 1

[Marco Bonardo [:mak]]

Attachment: browser_394759.js diff -w

diff -w for review

Comment 2

[Marco Bonardo [:mak]]

Attachment: browser_394759.js

Comment 3

[Marco Bonardo [:mak]]

Attachment: browser_394759.js

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

Comment 4

[Marco Bonardo [:mak]]

Attachment: browser_394759_privatebrowsing.js

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

Comment 5

[Marco Bonardo [:mak]]

Attachment: browser_526613.js

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

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Dão Gottwald [:dao]]

(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.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Dão Gottwald [:dao]]

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

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Marco Bonardo [:mak]]

filed bug 528469

Comment 12

[Simon Bünzli]

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 13

[Simon Bünzli]

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 14

[Simon Bünzli]

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?

Comment 15

[Marco Bonardo [:mak]]

(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.

Comment 16

[Marco Bonardo [:mak]]

(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.

Comment 17

[Marco Bonardo [:mak]]

(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

Comment 18

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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 19

[Simon Bünzli]

Comment on attachment 412181 [details] [diff] [review]
browser_394759.js diff -w

Fine, r+=me with the empty line inside the blankState removed.

Comment 20

[Simon Bünzli]

(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?

Comment 21

[Marco Bonardo [:mak]]

(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?

Comment 22

[Marco Bonardo [:mak]]

(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...

Comment 23

[Marco Bonardo [:mak]]

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.

Comment 24

[Marco Bonardo [:mak]]

(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.

Comment 25

[Marco Bonardo [:mak]]

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

Comment 26

[Marco Bonardo [:mak]]

Attachment: browser_394759_privatebrowsing.js

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

Comment 27

[Marco Bonardo [:mak]]

filed bug 528705

Comment 28

[Dão Gottwald [:dao]]

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.

Comment 29

[Dão Gottwald [:dao]]

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

Comment 30

[Dão Gottwald [:dao]]

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

Comment 31

[Dão Gottwald [:dao]]

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

Comment 32

[Simon Bünzli]

(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.

Comment 33

[Dão Gottwald [:dao]]

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.

Comment 34

[Dão Gottwald [:dao]]

(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.

Comment 35

[Dão Gottwald [:dao]]

(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.

Comment 36

[Marco Bonardo [:mak]]

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.

Comment 37

[Dão Gottwald [:dao]]

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.

Comment 38

[Marco Bonardo [:mak]]

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?

Comment 39

[Marco Bonardo [:mak]]

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

Comment 40

[Marco Bonardo [:mak]]

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.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 42

[Daniel Holbert [:dholbert]]

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

Comment 43

[Marco Bonardo [:mak]]

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.