Closed Bug 1484161 Opened 2 years ago Closed 2 years ago

Replace nsITimer usage in action module with Sleep from sync

Categories

(Testing :: Marionette, defect, P1)

defect

Tracking

(firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(1 file)

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.
Do you think this might help us with bug 1447449?
Flags: needinfo?(ato)
No.
Flags: needinfo?(ato)
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
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...
Attachment #9001893 - Flags: review?(hskupin) → review+
(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.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8f670aefa5
Replace nsITimer with Sleep for dispatchPause. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/7f8f670aefa5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(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?
Blocks: 1447449
Flags: needinfo?(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.
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-beta]
Whiteboard: [checkin-needed-beta]
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
Flags: needinfo?(ato)
(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.
You need to log in before you can comment on or make changes to this bug.