do_register_cleanup should support generators

RESOLVED FIXED in Firefox 54

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

It currently supports a promise, but not generators or async functions. that enforces to use a Task or build a new Promise object.
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?
Attachment #8828395 - Flags: review?(ted) → review+
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)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/c13c36e04303
do_register_cleanup should support generators and async functions. r=ted
https://hg.mozilla.org/mozilla-central/rev/c13c36e04303
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Depends on: 1324952
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/6e2baee09f14
do_register_cleanup should support generators and async functions. r=ted
https://hg.mozilla.org/mozilla-central/rev/6e2baee09f14
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.