do_register_cleanup should support generators

RESOLVED FIXED in Firefox 54

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
It currently supports a promise, but not generators or async functions. that enforces to use a Task or build a new Promise object.
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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+
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
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

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/c13c36e04303
do_register_cleanup should support generators and async functions. r=ted

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c13c36e04303
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Status: RESOLVED → REOPENED
status-firefox54: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
(Assignee)

Comment 7

2 years ago
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
(Assignee)

Updated

2 years ago
Depends on: 1324952

Comment 8

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/6e2baee09f14
do_register_cleanup should support generators and async functions. r=ted

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e2baee09f14
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.