Closed Bug 997909 Opened 10 years ago Closed 10 years ago

Synchronize execute_async_script and waitFor timeouts

Categories

(Testing :: Marionette Client and Harness, defect)

defect
Not set
normal

Tracking

(firefox33 wontfix, firefox34 wontfix, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

(Keywords: pi-marionette-client, Whiteboard: [affects=b2g])

Attachments

(2 files)

According to Zac, sometimes execute_async_script commands that call waitFor timeout, but the waitFor continues executing.  He's seen this happen when calling this code:

https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_data_layer.js#L266

Ideally, the timeouts for these things would be synchronized, so that when an execute_async_script command timed out, any waitFor commands it was running would timeout as well.
The thing that caused this ended up being an infinite loop between Gecko and Gaia (which the devs will fix) but I don't see why waitFor should not have timed out so I think this is still valid!
err not "that caused it" , rather the thing that was going on when I found it.
See Also: → 984654
Does the JavaScript waitFor timeout? If so, how do we influence this timeout value?
Flags: needinfo?(jgriffin)
It's possible to pass a timeout value as the third parameter waitFor, otherwise it inherits the same timeout value as the execute_async_script call in which it was used.

This can create a problem if e.g., the execute_async_script call has a 30s timeout, and waitFor is called 10s into that...it won't time out until 10s after the execute_async_script call does.
Flags: needinfo?(jgriffin)
I think we've seen issues with waitFor seemingly not timing out. In Python we're currently using milliseconds for the timeouts, so the script timeout for devices is 20000. Is this what's used in the JavaScript waitFor, even if it's not explicitly passed in the execute_async_script call? Also, is this being treated as milliseconds? If it was incorrectly treated as seconds it could explain waitFor calls that continue to run long after a test has finished.
Theoretically, that shouldn't be happening.  The implementation is here, which assumes the timeout is in ms:  http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-simpletest.js#144
Whiteboard: [affects=b2g]
Blocks: 1061742
Blocks: 1064800
No longer blocks: 1064800
Blocks: 1064800
Assignee: nobody → jgriffin
Comment on attachment 8498275 [details] [diff] [review]
Make waitFor actually terminate if condition is never true,

Carrying over r+ from bug 1064800.
Attachment #8498275 - Flags: review+
Gip was fine, will land as soon as trees reopen.
https://hg.mozilla.org/mozilla-central/rev/fb353d80bc49
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
We need this to be uplifted to at least v2.1, could you please help with that? Thank you.
Flags: needinfo?(jgriffin)
Comment on attachment 8498275 [details] [diff] [review]
Make waitFor actually terminate if condition is never true,

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: This is a fix to a long-standing bug in Marionette, no impact on shipping code.
[Describe test coverage new/current, TBPL]: n/a
[Risks and why]:  n/a
[String/UUID change made/needed]: n/a
Attachment #8498275 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jgriffin)
Comment on attachment 8498275 [details] [diff] [review]
Make waitFor actually terminate if condition is never true,

low risk, test-only change, approving for uplift.
Attachment #8498275 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I still can reproduce on 10/05 pvt build

Gaia      778ebac47554e1c4b7e9a952d73e850f58123914
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/aa98eb8873a2
BuildID   20141005160206
Version   34.0a2
ro.build.date   Thu Sep 4 14:59:02 CST 2014
ro.bootloader   L1TC10011800
ro.build.version.incremental   27
Comment on attachment 8498275 [details] [diff] [review]
Make waitFor actually terminate if condition is never true,

Review of attachment 8498275 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/marionette-simpletest.js
@@ +175,4 @@
>        }
> +      var now = new Date();
> +      var deadline = (timeout instanceof Date) ? timeout :
> +                     new Date(now.valueOf + (typeof(timeout) == "undefined" ? this.timeout : timeout))

It should be now.valueOf().
Reopen per comment 19, Jonathan, could you please fix it? It leaks a bunch of memory and eats cpu. Thanks.
Status: RESOLVED → REOPENED
Flags: needinfo?(jgriffin)
Resolution: FIXED → ---
Oof, sorry about that.
Flags: needinfo?(jgriffin)
Attached patch Fix typo,Splinter Review
Attachment #8501177 - Flags: review?(mdas)
Attachment #8501177 - Flags: review?(mdas) → review+
We still need to uplift the updated fix to v2.1.
https://hg.mozilla.org/mozilla-central/rev/827b92f481dd
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Comment on attachment 8501177 [details] [diff] [review]
Fix typo,

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: This fixes a typo in test-only code, no impact to shipping code.
[Describe test coverage new/current, TBPL]: N/A
[Risks and why]: N/A
[String/UUID change made/needed]: N/A
Attachment #8501177 - Flags: approval-mozilla-aurora?
Comment on attachment 8501177 [details] [diff] [review]
Fix typo,

Aurora+
Attachment #8501177 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The patch haven't been uplifted to mozilla-b2g34_v2_1, how should I make that happen?
Flags: needinfo?(lmandel)
I'm marking Firefox 34 as won't fix as I think this fix is b2g specific. If not, please nom for beta uplift.
Flags: needinfo?(lmandel)
Comment on attachment 8501177 [details] [diff] [review]
Fix typo,

Given that this is a test only change and that it was approved for Aurora but missed the merge, I'm approving for b2g34.
Attachment #8501177 - Flags: approval-mozilla-aurora+ → approval-mozilla-b2g34+
Product: Testing → Remote Protocol

Moving bugs for Marionette client due to component changes.

Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in before you can comment on or make changes to this bug.