After bug 726521 patch Dv1: { JavaScript error: chrome://global/content/config.js, line 426: view.selection is null TEST-INFO | chrome://mochitests/content/browser/suite/common/tests/browser/browser_394759_basic.js | Console message: [JavaScript Error: "view.selection is null" {file: "chrome://global/content/config.js" line: 426}] TEST-INFO | chrome://mochitests/content/browser/suite/common/tests/browser/browser_394759_basic.js | Console message: [JavaScript Error: "start is deprecated and will be removed in a future release. Check the nsILivemarkService interface." {file: "resource:///components/nsLivemarkService.js" line: 204}] TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_394759_basic.js | Test timed out INFO TEST-END | chrome://mochitests/content/browser/suite/common/tests/browser/browser_394759_basic.js | finished in 30117ms TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_394759_basic.js | Found a browser window after previous test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/tests/browser/browser_394759_behavior.js | There were 3 normal windows to repoen - Got 4, expected 3 } It fails somewhere after { 105 is(ss.getClosedWindowCount(), closedWindowCount, 106 "The reopened window was removed from Recently Closed Windows"); } *** [Mozilla/5.0 (Windows NT 5.0; rv:13.0a1) Gecko/20120201 Firefox/13.0a1 SeaMonkey/2.10a1] (nightly, 2012-02-01-00-30-09-comm-central-trunk) succeeds. Then I'm not sure yet what the issue/fix is: there are many fix/resync'/ports to do around these tests, maybe one of these will fix this too...

(In reply to Serge Gautherie (:sgautherie) from comment #0) > After bug 726521 patch Dv1: Same failure backported to SM 2.10 test :-< > [Mozilla/5.0 (Windows NT 5.0; rv:13.0a1) Gecko/20120201 Firefox/13.0a1 > SeaMonkey/2.10a1] (nightly, 2012-02-01-00-30-09-comm-central-trunk) > succeeds. Now, I can reproduce (almost always) :->

This executeSoon() wasn't there at all before. Dão, could we document why you added it (to Firefox)? *** (In reply to Serge Gautherie (:sgautherie) from comment #0) > JavaScript error: chrome://global/content/config.js, line 426: > view.selection is null I diagnosed the failure and suggest a (SeaMonkey) workaround. Neil, would you have an explanation/fix for this different behavior?

Comment on attachment 617795 [details] [diff] [review] (Av1) SeaMonkey browser_394759_basic.js: Add an 'aDelayForSeaMonkey' parameter to whenWindowLoaded(), as a workaround The console.js exception is a red herring, but you can fix it by setting the general.warnOnAboutConfig preference to false before running the test. The problem is that the test waits for the window to load before adding its tab restore listener, but because one of the tabs is blank, session store actually fires the restore event during the window load, before the test manages to add its listener. There are a number of ways you could resolve this: 1. You could just remove the executeSoon from whenWindowLoaded, because we don't want it in the undo close window case, and provideWindow already has its own executeSoon anyway. 2. You could add the tab restore listener to the window, instead of waiting for the window to load so that you can add it to the tab container. (At which point you could still remove the executeSoon from whenWindowLoaded, because only provideWindow is calling it.) 3. You could figure out why Firefox manages to add the listener before session store sends the tab restore event.

(In reply to neil@parkwaycc.co.uk from comment #3) > The console.js exception is a red herring, but you can fix it by setting the > general.warnOnAboutConfig preference to false before running the test. The exception doesn't happen when the test succeeds (and closes the page before that code runs). Yet, let's have a consistent display ;-) > The problem is that the test waits for the window to load before adding its > tab restore listener, but because one of the tabs is blank, session store > actually fires the restore event during the window load, before the test > manages to add its listener. Thanks for the details and hints! > There are a number of ways you could resolve this: > 1. You could just remove the executeSoon from whenWindowLoaded, because we > don't want it in the undo close window case, and provideWindow already has > its own executeSoon anyway. I'll do that in a second FF patch. > 2. You could add the tab restore listener to the window, instead of waiting > for the window to load so that you can add it to the tab container. (At > which point you could still remove the executeSoon from whenWindowLoaded, > because only provideWindow is calling it.) Let's do that, assuming the tab container part wasn't implicitly part of the test. > 3. You could figure out why Firefox manages to add the listener before > session store sends the tab restore event. Might be interesting to find out, but I would rather avoid investigating into (sessionrestore) code(s) just now :-|

Comment on attachment 618581 [details] [diff] [review] (Bv1-FF) browser_394759_basic.js: Use 'Services.prefs', Set 'general.warnOnAboutConfig' preference, Remove a 'whenWindowLoaded()' call, Add an 'info()' call, Nits [Checked in: See comment 9+10] Review of attachment 618581 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/test/browser_394759_basic.js @@ +82,3 @@ > let closedWindowCount = ss.getClosedWindowCount(); > > + provideWindow(function __t_pW_callback(newWin) { Nit: give this a less gross name @@ +107,5 @@ > > // SSTabRestored will fire more than once, so we need to make sure we count them > let restoredTabs = 0; > let expectedTabs = data.tabs.length; > + newWin2.addEventListener("SSTabRestored", function sstabrestoredListener(aEvent) { Please fix the whitespace here. This whole block needs to be unindented now.

Bv1-FF, with comment 5 suggestion(s).

Comment on attachment 618934 [details] [diff] [review] (Cv1-FF) Sessionstore tests: Improve and merge 'provideWindow()' and 'whenWindowLoaded()' into 'head.js', Add an 'info()' call, Nits [Checked in: See comment 12] Review of attachment 618934 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/test/browser_394759_behavior.js @@ +70,5 @@ > let settings = "chrome,dialog=no," + > (winData.isPopup ? "all=no" : "all"); > let url = "http://example.com/?window=" + windowsToOpen.length; > > + provideWindow(function __onTestURLLoaded(win) { Nit: don't put those leading underscores (here & the rest of the changes). I understand the reasoning, but it's mostly noise and inconsistent with what we've done in the tests. ::: browser/components/sessionstore/test/head.js @@ +220,5 @@ > executeSoon(aCallback); > }, true); > } > > +// 'aCallback' must accept 'aWindow' as its parameter. That's not really true because closures.

Comment on attachment 618581 [details] [diff] [review] (Bv1-FF) browser_394759_basic.js: Use 'Services.prefs', Set 'general.warnOnAboutConfig' preference, Remove a 'whenWindowLoaded()' call, Add an 'info()' call, Nits [Checked in: See comment 9+10] Bv1a-FF, with comment 8 suggestion(s).

Comment on attachment 618581 [details] [diff] [review] (Bv1-FF) browser_394759_basic.js: Use 'Services.prefs', Set 'general.warnOnAboutConfig' preference, Remove a 'whenWindowLoaded()' call, Add an 'info()' call, Nits [Checked in: See comment 9+10] https://hg.mozilla.org/mozilla-central/rev/b5c0d6abf69b

Comment on attachment 618934 [details] [diff] [review] (Cv1-FF) Sessionstore tests: Improve and merge 'provideWindow()' and 'whenWindowLoaded()' into 'head.js', Add an 'info()' call, Nits [Checked in: See comment 12] https://hg.mozilla.org/mozilla-central/rev/ba559aca9975 Cv1-FF, with comment 8 suggestion(s).

Comment on attachment 620252 [details] [diff] [review] (Av2-SM) browser_394759_basic.js: Resync' with fixed Firefox test [Checked in: Comment 13, 18 and 20] http://hg.mozilla.org/comm-central/rev/feebdae014b5 [Approval Request Comment] No risk, test-only.

https://tbpl.mozilla.org/php/getParsedLog.php?id=11386888&tree=Firefox&full=1 Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2012-05-02 05:30:00 PDT for push ba559aca9975 firefox15: verified.

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1335998965.1336004524.11846.gz&fulltext=1 WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/02 15:49:25 { INFO TEST-END | chrome://mochitests/content/browser/suite/common/tests/browser/browser_394759_basic.js | finished in 4847ms } V.Fixed

Ftr, this regression was caused by bug 658738.

(In reply to Serge Gautherie (:sgautherie) from comment #16) > Ftr, this regression was caused by bug 658738. Bug 726521 (which ported bug 658738).

Comment on attachment 620252 [details] [diff] [review] (Av2-SM) browser_394759_basic.js: Resync' with fixed Firefox test [Checked in: Comment 13, 18 and 20] http://hg.mozilla.org/releases/comm-beta/rev/9c608274d116

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1337595251.1337600055.1708.gz WINNT 5.2 comm-beta debug test mochitest-other on 2012/05/21 03:14:11 seamonkey2.10: verified.

Comment on attachment 620252 [details] [diff] [review] (Av2-SM) browser_394759_basic.js: Resync' with fixed Firefox test [Checked in: Comment 13, 18 and 20] http://hg.mozilla.org/releases/comm-aurora/rev/88dd18095ab0

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1338291115.1338294582.12338.gz OS X 10.6 comm-aurora debug test mochitest-other on 2012/05/29 04:31:55 seamonkey2.11: verified.