764540 - browser_tabfocus.js is racy

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

(If I've understood this correctly)
The test works fine if browser1 is loaded before browser2, but if that is not the case
pageshow of the browser1 ends up triggering _browser_tabfocus_navigation_test_eventOccured
because the load event listener adds listener for pageshow.
Before Bug 734015 browser1 is always loaded before browser2. But once we start to
slow down bg tab loading, the order may change.

https://tbpl.mozilla.org/?tree=Try&rev=8795f1d322f8 (Bug 734015 + this one)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 632839 [details] [diff] [review]
patch

So the idea is the let load/pageshow events to be handled before actually starting the test. Hopefully tryserver looks good :)

Comment 2

[Dão Gottwald [:dao]]

(In reply to Olli Pettay [:smaug] from comment #1)
> So the idea is the let load/pageshow events to be handled before actually
> starting the test.

In that case you should use executeSoon(_run_focus_tests).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Doesn't really matter.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Olli Pettay [:smaug] from comment #3)
> Doesn't really matter.

When you have to use setTimeout (i.e. executeSoon doesn't work for what you're trying to do), there's probably something wrong with your test. Therefore it's our policy for browser chrome tests to use executeSoon instead of setTimeout whenever possible.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

executeSoon would work, but the test uses already setTimeout and it really doesn't matter which
one to use in this case, so I prefer consistency.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

And based on the the tryserver the patch does actually fix the problem :)

Comment 7

[Dão Gottwald [:dao]]

(In reply to Olli Pettay [:smaug] from comment #5)
> executeSoon would work, but the test uses already setTimeout

We should fix that. (Not necessarily here.)

> and it really doesn't matter which one to use in this case,

I know setTimeout works. This isn't the point. The point is that we have a policy that allows us to distinguish legitimate timeouts from suspicious ones.

> so I prefer consistency.

Temporary inconsistency is ok in this case.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with executeSoon

Ok, I'm not a browser/ peer. If there is such policy, then there is.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/7253d15ea755