Last Comment Bug 764540 - browser_tabfocus.js is racy
: browser_tabfocus.js is racy
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 734015
  Show dependency treegraph
 
Reported: 2012-06-13 13:22 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-06-14 11:40 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (836 bytes, patch)
2012-06-13 13:22 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
with executeSoon (834 bytes, patch)
2012-06-14 05:24 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
dao+bmo: review+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-13 13:22:31 PDT
Created attachment 632839 [details] [diff] [review]
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] (high review load, please consider other reviewers) 2012-06-13 13:24:34 PDT
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] 2012-06-13 13:32:56 PDT
(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] (high review load, please consider other reviewers) 2012-06-13 13:34:02 PDT
Doesn't really matter.
Comment 4 Dão Gottwald [:dao] 2012-06-13 23:33:20 PDT
(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] (high review load, please consider other reviewers) 2012-06-14 04:09:18 PDT
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] (high review load, please consider other reviewers) 2012-06-14 05:06:12 PDT
And based on the the tryserver the patch does actually fix the problem :)
Comment 7 Dão Gottwald [:dao] 2012-06-14 05:21:04 PDT
(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] (high review load, please consider other reviewers) 2012-06-14 05:24:24 PDT
Created attachment 633107 [details] [diff] [review]
with executeSoon

Ok, I'm not a browser/ peer. If there is such policy, then there is.
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-14 11:40:42 PDT
https://hg.mozilla.org/mozilla-central/rev/7253d15ea755

Note You need to log in before you can comment on or make changes to this bug.