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)
Testing
Marionette Client and Harness
Tracking
(firefox33 wontfix, firefox34 wontfix, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)
RESOLVED
FIXED
mozilla35
People
(Reporter: jgriffin, Assigned: jgriffin)
References
Details
(Keywords: pi-marionette-client, Whiteboard: [affects=b2g])
Attachments
(2 files)
1.49 KB,
patch
|
jgriffin
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
960 bytes,
patch
|
mdas
:
review+
lmandel
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Keywords: ateam-marionette-client
Comment 1•10 years ago
|
||
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!
Comment 2•10 years ago
|
||
err not "that caused it" , rather the thing that was going on when I found it.
Comment 3•10 years ago
|
||
Does the JavaScript waitFor timeout? If so, how do we influence this timeout value?
Flags: needinfo?(jgriffin)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: [affects=b2g]
Assignee | ||
Comment 8•10 years ago
|
||
Moving this patch over from bug 1064800
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jgriffin
Assignee | ||
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
try run on gaia-ui-test: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2844bc3a9227
Assignee | ||
Comment 11•10 years ago
|
||
Gip was fine, will land as soon as trees reopen.
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb353d80bc49
Target Milestone: --- → mozilla35
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fb353d80bc49
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 14•10 years ago
|
||
We need this to be uplifted to at least v2.1, could you please help with that? Thank you.
Flags: needinfo?(jgriffin)
Assignee | ||
Updated•10 years ago
|
status-b2g-v2.1:
--- → affected
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/202d5775fdff
status-b2g-v2.2:
--- → fixed
status-firefox33:
--- → wontfix
status-firefox34:
--- → fixed
status-firefox35:
--- → fixed
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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().
Comment 20•10 years ago
|
||
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 → ---
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8501177 -
Flags: review?(mdas)
Updated•10 years ago
|
Attachment #8501177 -
Flags: review?(mdas) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 23•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/827b92f481dd
Keywords: checkin-needed
Comment 24•10 years ago
|
||
We still need to uplift the updated fix to v2.1.
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/827b92f481dd
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•10 years ago
|
||
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 27•10 years ago
|
||
Comment on attachment 8501177 [details] [diff] [review] Fix typo, Aurora+
Attachment #8501177 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 28•10 years ago
|
||
The patch haven't been uplifted to mozilla-b2g34_v2_1, how should I make that happen?
Flags: needinfo?(lmandel)
Updated•10 years ago
|
Comment 29•10 years ago
|
||
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 30•10 years ago
|
||
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+
Updated•1 year ago
|
Product: Testing → Remote Protocol
Comment 32•1 year ago
|
||
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.
Description
•