1484161 - Replace nsITimer usage in action module with Sleep from sync

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Andreas Tolfsen ❲:ato❳]

We have a Sleep utility for pausing the current thread and resolving
on a promise in testing/marionette/sync.js that could be used to
replace the nsITimer usage for pauses in testing/marionette/action.js.
Advantage would be that it has a simpler API.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Do you think this might help us with bug 1447449?

Comment 2

[Andreas Tolfsen ❲:ato❳]

No.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: Replace nsITimer with Sleep for dispatchPause.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 9001893 [details] [diff] [review]
Replace nsITimer with Sleep for dispatchPause.

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

::: testing/marionette/action.js
@@ -1425,5 @@
> -  const timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> -  let duration = typeof a.duration == "undefined" ? tickDuration : a.duration;
> -  return new Promise(resolve =>
> -      timer.initWithCallback(resolve, duration, Ci.nsITimer.TYPE_ONE_SHOT)
> -  );

If the caller doesn't handle the failure case of the promise it will cause an infinite hang. By moving to `Sleep` this is now internally handled.

So maybe we have cases when this happens and it is related to bug 1447449. At least those users would get a proper failure message now. We will see...

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #4)

> ::: testing/marionette/action.js
> @@ -1425,5 @@
> > -  const timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > -  let duration = typeof a.duration == "undefined" ? tickDuration : a.duration;
> > -  return new Promise(resolve =>
> > -      timer.initWithCallback(resolve, duration, Ci.nsITimer.TYPE_ONE_SHOT)
> > -  );
> 
> If the caller doesn't handle the failure case of the promise it will cause
> an infinite hang. By moving to `Sleep` this is now internally handled.

If I understand this correctly the promise will never fail, unless
timer.initWithCallback for some reason were to throw an error.  I
don’t think it does.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8f670aefa5
Replace nsITimer with Sleep for dispatchPause. r=whimboo

Comment 7

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/7f8f670aefa5

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #2)
> (In reply to Henrik Skupin (:whimboo) from comment #1)
>> Do you think this might help us with bug 1447449?
> No.

So this is actually not true. My checks clearly show that it indeed helps with bug 1447449. There is no hang in the `pause` action anymore for the provided testcase! I will follow-up with more details on that other bug.

Andreas, given that this is such a small change, I think we should uplift this to beta. Can you please handle that?

Comment 9

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #8)

> Andreas, given that this is such a small change, I think we should uplift
> this to beta. Can you please handle that?

We have the checkin-needed-beta tag for that.  I don’t see any harm
in uplifting it.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdc4d097c8ba

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing webdriver/tests/actions/control_click.py:

https://hg.mozilla.org/releases/mozilla-beta/rev/10e2940b1bdea803e05fc0e26285c4cceb7f5292

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=cdc4d097c8ba9c4a9748de7a0337e20bd528a421&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194887508&repo=mozilla-beta

[task 2018-08-20T16:44:28.427Z] 16:44:28     INFO - STDOUT: E           UnknownErrorException: unknown error (500): TypeError: Sleep is not a function
[task 2018-08-20T16:44:28.428Z] 16:44:28     INFO - STDOUT: E           
[task 2018-08-20T16:44:28.428Z] 16:44:28     INFO - STDOUT: E           Remote-end stacktrace:
[task 2018-08-20T16:44:28.428Z] 16:44:28     INFO - STDOUT: E           
[task 2018-08-20T16:44:28.429Z] 16:44:28     INFO - STDOUT: E           dispatchPause@chrome://marionette/content/action.js:1427:10
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: E           toEvents/<@chrome://marionette/content/action.js:1120:16
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: E           action.dispatchTickActions@chrome://marionette/content/action.js:1023:23
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: E           action.dispatch/chainEvents<@chrome://marionette/content/action.js:992:13
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: E           action.dispatch@chrome://marionette/content/action.js:990:22
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: E           performActions@chrome://marionette/content/listener.js:780:9
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: E           dispatch/</req<@chrome://marionette/content/listener.js:485:14
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: E           dispatch/<@chrome://marionette/content/listener.js:478:15
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: body       = {'actions': [{'actions': [{'duration': 0, 'type': 'pause'}, {'type': 'keyDown', 'value': ''}], 'id': 'keyboard_id', '...{'button': 0, 'type': 'pointerDown'}], 'id': 'pointer_id', 'parameters': {'pointerType': 'mouse'}, 'type': 'pointer'}]}
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: err        = <UnknownErrorException http_status=500>
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: method     = 'POST'
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: response   = <Response status=500 error=<UnknownErrorException http_status=500>>
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: self       = <Session e782e6aa-f220-45ea-908b-3e8e63b1eaa3>
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: url        = 'session/e782e6aa-f220-45ea-908b-3e8e63b1eaa3/actions'
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: tests/web-platform/tests/tools/webdriver/webdriver/client.py
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: :445: UnknownErrorException
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: =============================== warnings summary ===============================
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: None
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT:   Module already imported so cannot be rewritten: mozlog
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: -- Docs: http://doc.pytest.org/en/latest/warnings.html
[task 2018-08-20T16:44:28.436Z] 16:44:28     INFO - STDOUT: ===================== 3 failed, 1 warnings in 8.81 seconds =====================
[task 2018-08-20T16:44:28.437Z] 16:44:28     INFO - 
[task 2018-08-20T16:44:28.438Z] 16:44:28     INFO - TEST-UNEXPECTED-FAIL | /webdriver/tests/actions/control_click.py | test_control_click[\ue009-ctrlKey] - UnknownErrorException: unknown error (500): TypeError: Sleep is not a function

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #9)
> > Andreas, given that this is such a small change, I think we should uplift
> > this to beta. Can you please handle that?
> 
> We have the checkin-needed-beta tag for that.  I don’t see any harm
> in uplifting it.

It's not just like setting the flag, as we can see now. That's why I handed it over.

Checking with hg graft it turns out that there are a couple of dependencies causing merge conflicts. As such I would say that we do not uplift this patch, but take the simple approach from bug 1447449 to fix the hang.