988844 - Add functionality for async cleanup functions for xpcshell tests

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Currently there is no good way to do async cleanup operations (i.e. waiting on promises or tasks).

While it is possible to |add_task(cleanupOperations)|, this will be skipped if there is a failure in a previous test task.

Per [1] we could:
* change |do_register_cleanup()| to specially deal with functions returning promises etc., or
* add a |do_register_cleanup_task()|

Comment 1

[Georg Fritzsche [:gfritzsche]]

Related discussion:
https://groups.google.com/forum/#!msg/mozilla.dev.platform/14ER9uMe8Dw/6fukwFeXNAkJ

Comment 2

[Gregory Szorc [:gps]]

Related to AsyncTest.jsm (or whatever it's called these days - I can't even find the bug).

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(In reply to Gregory Szorc [:gps] from comment #2)
> Related to AsyncTest.jsm (or whatever it's called these days - I can't even
> find the bug).

What is AsyncTest.jsm?

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #3)
> What is AsyncTest.jsm?

I think Greg meant bug 867742.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Letting do_register_cleanup play nicely with functions that return a Promise

Here's one possible implementation.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8398454 [details] [diff] [review]
Letting do_register_cleanup play nicely with functions that return a Promise

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

This seems like a sane approach.

::: testing/xpcshell/head.js
@@ +434,5 @@
> +    if (result && typeof result == "object" && "then" in result) {
> +      // This is a promise, wait until it is satisfied before proceeding
> +      let complete = false;
> +      let promise = result.then(null, reportCleanupError);
> +      promise = promise.then(() => complete = true);

This could be chained with the previous line, right?

@@ +438,5 @@
> +      promise = promise.then(() => complete = true);
> +      let thr = Components.classes["@mozilla.org/thread-manager;1"]
> +                  .getService().currentThread;
> +      while (!complete) {
> +        thr.processNextEvent(true);

I wonder if it'd make more sense to execute all the cleanup functions, then spin the event loop at the end if any promises were executed, waiting for them all to finish? Not sure if that'd be more complex. I guess this way they all get executed serially, which is simple to reason about in any event.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> Comment on attachment 8398454 [details] [diff] [review]
> Letting do_register_cleanup play nicely with functions that return a Promise
> 
> Review of attachment 8398454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like a sane approach.
> 
> ::: testing/xpcshell/head.js
> @@ +434,5 @@
> > +    if (result && typeof result == "object" && "then" in result) {
> > +      // This is a promise, wait until it is satisfied before proceeding
> > +      let complete = false;
> > +      let promise = result.then(null, reportCleanupError);
> > +      promise = promise.then(() => complete = true);
> 
> This could be chained with the previous line, right?

It's a matter of taste, but yes, it most definitely could. Tell me which one you prefer.

> @@ +438,5 @@
> > +      promise = promise.then(() => complete = true);
> > +      let thr = Components.classes["@mozilla.org/thread-manager;1"]
> > +                  .getService().currentThread;
> > +      while (!complete) {
> > +        thr.processNextEvent(true);
> 
> I wonder if it'd make more sense to execute all the cleanup functions, then
> spin the event loop at the end if any promises were executed, waiting for
> them all to finish? Not sure if that'd be more complex. I guess this way
> they all get executed serially, which is simple to reason about in any event.

That was the idea, yes.
Note that I have changed the order of evaluation of cleanup functions, as they were executed from last to first. Is that ok with you?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=6d98ca73ae19

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #7)
> > This could be chained with the previous line, right?
> 
> It's a matter of taste, but yes, it most definitely could. Tell me which one
> you prefer.

I thought the chaining was more conventional, but I haven't written much promise-based JS, so you can leave this if you like it.

> That was the idea, yes.
> Note that I have changed the order of evaluation of cleanup functions, as
> they were executed from last to first. Is that ok with you?

If it doesn't break anything I don't think I have an opinion.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Letting do_register_cleanup play nicely with functions that return a Promise, v2

Obviously, changing the execution order didn't do well.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Letting do_register_cleanup play nicely with functions that return a Promise, v3

Same one without an uppercase on a filename.
Just spent 1/2 day debugging that Linux-only failure, sigh.

Try: https://tbpl.mozilla.org/?tree=Try&rev=4b2d629dd735

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa6cc6b2ef8f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Firefox Commit Bot]

The following changeset is now in Firefox Nightly:

> aa6cc6b2ef8f Bug 988844 - do_register_cleanup now accepts asynchronous cleanup functions. r=ted

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central