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] (review request backlog because of a work week)
:
:
Mentors:
Depends on:
Blocks: 734015
  Show dependency treegraph
 
Reported: 2012-06-13 13:22 PDT by Olli Pettay [:smaug] (review request backlog because of a work week)
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] (review request backlog because of a work week)
no flags Details | Diff | Splinter Review
with executeSoon (834 bytes, patch)
2012-06-14 05:24 PDT, Olli Pettay [:smaug] (review request backlog because of a work week)
dao+bmo: review+
Details | Diff | Splinter Review

Description User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2012-06-13 13:34:02 PDT
Doesn't really matter.
Comment 4 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2012-06-14 05:06:12 PDT
And based on the the tryserver the patch does actually fix the problem :)
Comment 7 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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.