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.

try: https://tbpl.mozilla.org/?tree=Try&rev=99297e81c14f https://tbpl.mozilla.org/?tree=Try&rev=c694fcb5cbc3

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.

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.

What does block this approval ?

Comment on attachment 715791 [details] [diff] [review] Be less agressive with clearing timeouts, a=test-only