851516 - browser-test.js should wait for delayed browser startup

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Joe Drew (not getting mail)]

Attachment: wait for browser-delayed-startup-finished before running tests

Right now our browser-chrome tests have a very subtle race with delayed browser startup. By accident, right now we (almost?) always avoid this problem, just because of the order onload events fire. But there's no reason, as far as I can tell, that this should always be the case, and indeed my patches on bug 716140 make it not reliably the case.

(What happens is that the test harness starts loading the test .html file, and then delayed browser startup loads the about:blank that we were passed on the command line in that tab, which *cancels* the test file's load.)

If, instead, we wait for delayed browser startup, we will reliably be able to load our tests.

Comment 1

[Joe Drew (not getting mail)]

This is running through Try: https://tbpl.mozilla.org/?tree=Try&rev=16c226ffb1be (accidentally flubbed the trychooser syntax earlier which is why the patch itself doesn't show up in the tbpl log)

Comment 2

[Dão Gottwald [:dao]]

"browser-delayed-startup-finished" isn't available in SeaMonkey. Can you listen to the first MozAfterPaint event following the load event instead? This is what triggers the "delayed startup" in Firefox.

Comment 3

[Joe Drew (not getting mail)]

Attachment: wait for MozAfterPaint before running tests

Fair enough. This also fixes a bug in the previous patch, which was that we'd wait for focus on a particular element, but our focus request was immediately thrown away because the browser's delayed startup calls focus() as well.

Try: https://tbpl.mozilla.org/?tree=Try&rev=19671290c4a0

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Wouldn't it be simpler to adjust the logic at http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-harness.xul to call TestStart after mozafterpaint, rather than onload?

Though I think I would prefer that browser-delayed-startup-finished is used for this, since it's an explicit signal from the app that things are ready. Relying on mozAfterPaint being that point for all apps seems kind of brittle. Other apps can add support for browser-delayed-startup-finished (or some other similar topic) if they want to support browser-chrome tests.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Nice find!

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 725499 [details] [diff] [review]
wait for MozAfterPaint before running tests

Review of attachment 725499 [details] [diff] [review]:
-----------------------------------------------------------------

overall this patch looks safe enough and will help us be more reliable.

::: testing/mochitest/browser-test.js
@@ +21,5 @@
> +function DelayedStartupObserver() {
> +  window.removeEventListener("MozAfterPaint", DelayedStartupObserver, false);
> +  // Queue us up in the event queue so browser startup has a chance to complete
> +  // (and we can reliably wait for focus).
> +  setTimeout(function() { gCanStart = true; }, 0);

Are we doing this setTimeout here to add more of a delay?  While things are working now and this patch/fix will aid a series of other cases, I see this as yet another hack.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 725499 [details] [diff] [review]
wait for MozAfterPaint before running tests

per comment 3. Joe: I'll take this if you want.

Comment 8

[Joe Drew (not getting mail)]

(In reply to Joel Maher (:jmaher) from comment #6)
> Comment on attachment 725499 [details] [diff] [review]
> wait for MozAfterPaint before running tests
> 
> Review of attachment 725499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> overall this patch looks safe enough and will help us be more reliable.
> 
> ::: testing/mochitest/browser-test.js
> > +  setTimeout(function() { gCanStart = true; }, 0);
> 
> Are we doing this setTimeout here to add more of a delay?  While things are
> working now and this patch/fix will aid a series of other cases, I see this
> as yet another hack.

This is because we have to wait for the browser's MozAfterPaint handler (i.e., browser-delayed-startup-finished) to finish, since both it and we want to call .focus().

Comment 9

[Joe Drew (not getting mail)]

Attachment: wait for browser-delayed-startup-finished before running tests

OK, back to plan #1. Joel, this is morally equivalent to the previous patch, but hopefully this one makes Gavin happy too, so I'm carrying over r+ - hope that's OK.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternate patch

This is what I was thinking of.

Joe: can you confirm this works and solves your problem?

Neil: SeaMonkey would need to either start firing browser-delayed-startup-finished, or we'd need to create a new notification (mochitest-browser-chrome-start?) that SeaMonkey/Firefox both fire instead.

I don't know whether there are other apps using this harness that matter.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> I don't know whether there are other apps using this harness that matter.

I guess metro and webapprt (looking at runTests). I'll propose another patch.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

I guess also b2g, since it was hacked in via http://hg.mozilla.org/mozilla-central/rev/441a93f42c6d :(

FML this code is gross.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: extend the hacks

This defines a magic symbol for apps to opt-in to controlling BrowserChrome behavior, makes Firefox do that.

Untested!

Comment 14

[Dão Gottwald [:dao]]

To me this looks a lot grosser than just waiting for MozAfterPaint by default. If some app wants the test suite to wait even longer, we could add hooks to allow that.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #14)
> If some app wants the test suite to wait even longer, we could add
> hooks to allow that.

(In a separate bug, if and when the need materializes. Waiting for load+MozAfterPaint shouldn't make it any worse for apps with different startup behavior than just waiting for load.)

Comment 16

[Dão Gottwald [:dao]]

Attachment: wait for MozAfterPaint before running tests

This is very straightforward. Attachment 725499 [details] [diff] came with some unnecessary complexity.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 726570 [details] [diff] [review]
wait for MozAfterPaint before running tests

This introduces a non-obvious dependency - this testing code doesn't actually care about painting at all, it cares that the app is properly initialized (delayedStartup has run), and so it should check that explicitly. That pretty much requires the app letting the harness know when it's ready. This approach will break if browser.js changes how it calls delayedStartup, which doesn't seem unlikely.

Comment 18

[Dão Gottwald [:dao]]

> This approach will break if browser.js changes how it calls
> delayedStartup, which doesn't seem unlikely.

It does seem unlikely to me, because we very deliberately wait for the first paint. See the discussion in bug 715402. In any case, if we change startup behavior, we can change browser-test.js as we see fit (worst case: introduce your hack). At present your hack is solving a problem that doesn't exist.

Comment 19

[[e-mail]]

When you say the test harness loads the test .html file, do you mean that there are tests that want to load an arbitrary .html file as part of their test, or is there a specific .html file that needs to be loaded as part of the harness?

If the former, then perhaps the test could open a new tab instead?

If the latter, then perhaps the .html file could be passed on the command line instead?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #18)
> It does seem unlikely to me, because we very deliberately wait for the first
> paint. See the discussion in bug 715402.

I'm well aware of that bug (I reviewed the patch), but just because we decided this was the best mechanism to use for the moment doesn't mean that will hold true in the future. We may additionally add other "initialization" steps separate from delayedStartup that we'll want to block running tests on. This kind of hack wouldn't deal with that either.

> In any case, if we change startup behavior, we can change browser-test.js as we
> see fit (worst case: introduce your hack). At present your hack is solving a
> problem that doesn't exist.

The problem it's solving is longer-term fragility of this test harness. We should have a solution that works intentionally, rather than working as a by-product of non-obvious assumptions.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to [e-mail] from comment #19)
> When you say the test harness loads the test .html file

It's not the harness that loads it, it's the first tests (and depending on how you run tests, arbitrary tests can be run first). So the harness needs to deal with that generically.

Comment 22

[Dão Gottwald [:dao]]

The arguments I made in bug 715402 are very fundamental. Once the loaded window is painted, the user is supposed to be able to use it. This will always need to be our point of reference, and since we want tests to run under realistic conditions, the same should be true for them. So on second thought I would even go as far as to say that the browser window should never have to notify the test suite. Tests generally having to wait longer than what I made them wait in attachment 726570 [details] [diff] [review] would be a bug.

Comment 23

[[e-mail]]

(In reply to Joe Drew from comment #0)
> (What happens is that the test harness starts loading the test .html file,
> and then delayed browser startup loads the about:blank that we were passed
> on the command line in that tab, which *cancels* the test file's load.)
(Note that SeaMonkey doesn't load the about:blank that we were passed on the command line in the first tab, even if you pass a charset or referrer.)

Comment 24

[Joe Drew (not getting mail)]

Comment on attachment 726536 [details] [diff] [review]
extend the hacks

This version, a version with the setter corrected, and a different version that Gavin gave me separately, all fail. https://tbpl.mozilla.org/?tree=Try&rev=5f688e0c945a

Comment 25

[Joe Drew (not getting mail)]

Attachment: wait for browser-delayed-startup-finished before running tests v2

Gavin, this patch (only slightly modified from my earlier one) works for me in exactly the places yours doesn't. I suspect it has to do with the fact that both browser-delayed-startup-finished calls .focus(), and so do we, and the two don't get along; if you do a setTimeout(), letting the focus finish, all's well.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Does this patch work, then? If not, just use attachment 726570 [details] [diff] [review] (assuming it works), and I can fix things up later.

Comment 27

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf440c32c3cf

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bf440c32c3cf