1332295 - do_register_cleanup should support generators

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Marco Bonardo [:mak]]

It currently supports a promise, but not generators or async functions. that enforces to use a Task or build a new Promise object.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1332295 - do_register_cleanup should support generators and async functions.

Review commit: https://reviewboard.mozilla.org/r/105826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105826/

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 8828395 [details]
Bug 1332295 - do_register_cleanup should support generators and async functions.

https://reviewboard.mozilla.org/r/105826/#review110682

::: testing/xpcshell/head.js:608
(Diff revision 1)
>  
> -  let func;
> -  while ((func = _cleanupFunctions.pop())) {
> -    let result;
> +  let complete = _cleanupFunctions.length == 0;
> +  _Task.spawn(function*() {
> +    for (let func of _cleanupFunctions.reverse()) {
> -    try {
> +      try {
> -      result = func();
> +        yield func();

If an async function raises an error that will propogate up to the `catch(reportCleanupError)`, right?

::: testing/xpcshell/selftest.py:314
(Diff revision 1)
>    do_register_cleanup(function sync_cleanup_2() {
> +    checkpoints.push(6);
> +  });
> +
> +  do_register_cleanup(async function async_cleanup_4() {
> +    await undefined;

What does this do, just ensure that this function does async work without caring about the value it receives?

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8828395 [details]
Bug 1332295 - do_register_cleanup should support generators and async functions.

https://reviewboard.mozilla.org/r/105826/#review110682

> If an async function raises an error that will propogate up to the `catch(reportCleanupError)`, right?

I just tried this in Scratchpad, it works fine:
async function test() {
  await undefined;
  throw(new Error("test"));
}
Task.spawn(function*() {
  yield test();
}).catch(alert)

> What does this do, just ensure that this function does async work without caring about the value it receives?

It works the same as yield undefined; delaying the following code to the next tick (or micro tick at the end of the current tick). I just didn't want to complicate the code adding a further async function. Scratchpad example:
async function test() {
  await undefined;
  Cu.reportError(2);
}
async function test2() {
  Cu.reportError(1);
}
Task.spawn(function*() {
  Cu.reportError(0);
  let p1 = test();
  let p2 = test2();
  yield p1;
  yield p2;
  Cu.reportError(3);
}).catch(alert)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c13c36e04303
do_register_cleanup should support generators and async functions. r=ted

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c13c36e04303

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/20a8536b0bfa - everything else appears to be fine, but Linux32 debug was hitting bug 1324952 about 88% of the time, https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=369da2e22d4321ec877302c53f89da453f42c17d&group_state=expanded&filter-searchStr=44fbca1df5ee78b16de90a3aefc980ae81d2e5e0&tochange=c13c36e04303c88db6e95d20c4bde5ec821e64f9

Comment 7

[Marco Bonardo [:mak]]

that test is clearly broken, it keeps running after it's done:

[task 2017-02-06T01:39:39.502347Z] 01:39:39     INFO -  exiting test
[task 2017-02-06T01:39:39.504105Z] 01:39:39     INFO -  "onStopListening"
[task 2017-02-06T01:39:39.506444Z] 01:39:39     INFO -  "TLS handshake done"
[task 2017-02-06T01:39:39.508075Z] 01:39:39     INFO -  "TLS version used: 772"
[task 2017-02-06T01:39:39.510437Z] 01:39:39     INFO -  "input stream ready"
[task 2017-02-06T01:39:39.512233Z] 01:39:39  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_be_conservative.js |  - error should be NS_BASE_STREAM_CLOSED - 2152398864 == 2152136706

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e2baee09f14
do_register_cleanup should support generators and async functions. r=ted

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e2baee09f14