842841 - Marionette's clearing of all timeouts breaking Gaia perf tests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

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.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: Be less agressive with clearing timeouts,

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.

Comment 2

[Jonathan Griffin (:jgriffin)]

try:

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

Comment 3

[Julien Wajsberg [:julienw]]

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.

Comment 4

[Jonathan Griffin (:jgriffin)]

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 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 715791 [details] [diff] [review]
Be less agressive with clearing timeouts,

See comment #4.

Comment 6

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42f61751c2f8

Comment 7

[Julien Wajsberg [:julienw]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42f61751c2f8

Comment 9

[Julien Wajsberg [:julienw]]

What does block this approval ?

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 715791 [details] [diff] [review]
Be less agressive with clearing timeouts,

a=test-only

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f96b3fe5d05b

Comment 12

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/eb413bd929fd