Closed Bug 842841 Opened 11 years ago Closed 11 years ago

Marionette's clearing of all timeouts breaking Gaia perf tests


(Remote Protocol :: Marionette, defect)

Not set


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

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


(Reporter: jgriffin, Assigned: jgriffin)



(1 file)

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

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 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 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?
Closed: 11 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,

Attachment #715791 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.