Closed Bug 997909 Opened 6 years ago Closed 5 years ago

Synchronize execute_async_script and waitFor timeouts


(Testing :: Marionette, defect)

Not set


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

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


(Reporter: jgriffin, Assigned: jgriffin)



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


(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:

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)
Duplicate of this bug: 984654
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 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:
Whiteboard: [affects=b2g]
No longer 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.
Closed: 5 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
BuildID   20141005160206
Version   34.0a2   Thu Sep 4 14:59:02 CST 2014
ro.bootloader   L1TC10011800   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.
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.
Closed: 5 years ago5 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,

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+
You need to log in before you can comment on or make changes to this bug.