browser_tabfocus.js is racy

RESOLVED FIXED

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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)
Attachment #632839 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #632839 - Flags: review? → review?(enndeakin)
(Assignee)

Comment 1

5 years ago
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 :)
(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).
(Assignee)

Comment 3

5 years ago
Doesn't really matter.
(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.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
And based on the the tryserver the patch does actually fix the problem :)
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 633107 [details] [diff] [review]
with executeSoon

Ok, I'm not a browser/ peer. If there is such policy, then there is.
Attachment #633107 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #632839 - Flags: review?(enndeakin)

Updated

5 years ago
Attachment #633107 - Flags: review?(dao) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7253d15ea755
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.