Closed Bug 842841 Opened 9 years ago Closed 8 years ago

Marionette's clearing of all timeouts breaking Gaia perf tests

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

Details

Attachments

(1 file)

In marionette-listeners, we clear all window timeouts on async return here:

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#328

This is breaking the Gaia perf tests, which hook into regular Gaia code and want setTimeouts in that code to continue working.
This patch fixes the problem with the Gaia perf tests.  I'll need to run it on try to make sure it's ok there as well.
Assignee: nobody → jgriffin
I wonder if we could not be more explicit and have less heuristics here.

Some propositions :
- have 2 functions in the script running on the device :
 * one "marionetteScriptFinished" -> clear all stuff
 * one "marionetteScriptReturns" -> merely return control to the controlling script

- have 2 functions or maybe a parameter in the controlling script's executingAsyncScript, that would tell marionette if this is the last execution or not.
The try runs were green, so I'm happy to land this as-is.  The previous implementation was buggy anyway...it was removing timeouts <= asyncTestTimeoutId, rather than >=; in other words, it was removing timeouts that were in place before the script was executed, rather than removing those that were added by the execution of the script.

In any event, there are many other ways that bad scripts can contaminate later scripts (like event listeners that never get removed), and we've recently implemented logic to handle this in a more global way, so specific code to arbitrarily remove timeouts it no longer needed.

If we find we still have reason to implement something like marionetteScriptReturns, however, I'm all for it.
Comment on attachment 715791 [details] [diff] [review]
Be less agressive with clearing timeouts,

See comment #4.
Attachment #715791 - Flags: review?(mdas)
Attachment #715791 - Flags: review?(mdas) → review+
Comment on attachment 715791 [details] [diff] [review]
Be less agressive with clearing timeouts,

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: no direct user impact but we can't run the perf tests without this
Testing completed: 
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

I'd like to have this on b2g18 as that's the gecko we're using to test.
Attachment #715791 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/42f61751c2f8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
What does block this approval ?
Comment on attachment 715791 [details] [diff] [review]
Be less agressive with clearing timeouts,

a=test-only
Attachment #715791 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
You need to log in before you can comment on or make changes to this bug.