1313956 - Upgrade from Task.jsm to async/await.

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1313956: Async all the tests.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1313956: Async all the functions.

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

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8805884 [details]
Bug 1313956: Async all the tests.

You need to do more than just update the version number to update eslint as it also needs to work on taskcluster.

This is handled by the automation team these days.

@Ben: Can you or somebody on your team run the update script and add a patch?

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 8805884 [details]
Bug 1313956: Async all the tests.

I'm not sure how I got flagged here (I don't see my name in mozreview), but I'm not an appropriate reviewer for this.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Ben Hearsum (:bhearsum) from comment #4)
> I'm not sure how I got flagged here (I don't see my name in mozreview), but
> I'm not an appropriate reviewer for this.

Mike redirected the review to you, but I think a needinfo may have been more appropriate:

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> You need to do more than just update the version number to update eslint as
> it also needs to work on taskcluster.
> 
> This is handled by the automation team these days.
> 
> @Ben: Can you or somebody on your team run the update script and add a patch?

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ahal has been the most involved in linting work, I suppose they are best point of contact.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8805885 [details]
Bug 1313956: Async all the functions.

https://reviewboard.mozilla.org/r/89494/#review88956

Nice, a handful of little nits that you can take or leave.

::: browser/components/extensions/ext-bookmarks.js:95
(Diff revision 1)
>              }
>              bookmarks.push(convert(bookmark));
>            }
>            return bookmarks;
> -        }).catch(error => Promise.reject({message: error.message}));
> +        } catch (error) {
> +          return Promise.reject({message: error.message});

This looks overly broad, I think the throw a few lines above could just directly return Promise.reject() and we could avoid catching anything else that might get thrown here?
I know this is existing behavior that you haven't touched but while we're here...

::: toolkit/components/extensions/Extension.jsm:1061
(Diff revision 1)
>            locales,
>            builtinMessages: this.builtinMessages,
>          });
>  
>          return locales;
> -      }.bind(this));
> +      }).call(this);

How about just making this a method called `_doPromiseLocales()` or something instead of having it anonymous here?

Comment 8

[Andrew Halberstadt [:ahal]]

So I ran the update script, but then noticed that this patch uses ^3.6.0 which ends up installing eslint 3.9.0. Is this intended? I think we probably want to be pinning this so people don't end up using a different version from automation.

So should I upgrade to 3.6 or 3.9?

Comment 9

[Kris Maglione [:kmag]]

(In reply to Andrew Halberstadt [:ahal] from comment #8)
> So I ran the update script, but then noticed that this patch uses ^3.6.0
> which ends up installing eslint 3.9.0. Is this intended? I think we probably
> want to be pinning this so people don't end up using a different version
> from automation.
> 
> So should I upgrade to 3.6 or 3.9?

Upgrading to 3.9 would probably be better, but I was trying to be conservative.

The npm-shrinkwrap.json in practice overrides what's specified in package.json, so the "^" in the dependency version only applies if someone manually runs `npm install` and then re-generates the shrinkwrap.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8805885 [details]
Bug 1313956: Async all the functions.

https://reviewboard.mozilla.org/r/89494/#review88956

> This looks overly broad, I think the throw a few lines above could just directly return Promise.reject() and we could avoid catching anything else that might get thrown here?
> I know this is existing behavior that you haven't touched but while we're here...

The throw is mostly incidental, since this is mainly designed to deal with errors thrown by `bookmarks.fetch` for things like invalid bookmark IDs.

It could probably improved, but I think it's best left to a separate bug.

> How about just making this a method called `_doPromiseLocales()` or something instead of having it anonymous here?

Yeah, that would probably be better. I didn't really like doing it this way.

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8805885 [details]
Bug 1313956: Async all the functions.

https://reviewboard.mozilla.org/r/89494/#review88956

> The throw is mostly incidental, since this is mainly designed to deal with errors thrown by `bookmarks.fetch` for things like invalid bookmark IDs.
> 
> It could probably improved, but I think it's best left to a separate bug.

Well in that case we're exposing errors that come from within Places.jsm to callers which is generally bad.  You didn't break this though so leaving it as is seems okay.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8805885 [details]
Bug 1313956: Async all the functions.

https://reviewboard.mozilla.org/r/89494/#review88956

> Well in that case we're exposing errors that come from within Places.jsm to callers which is generally bad.  You didn't break this though so leaving it as is seems okay.

Well, I *did*, just not in this patch ;)

Comment 13

[Andrew Halberstadt [:ahal]]

(In reply to Kris Maglione [:kmag] from comment #9)
> Upgrading to 3.9 would probably be better, but I was trying to be
> conservative.
> 
> The npm-shrinkwrap.json in practice overrides what's specified in
> package.json, so the "^" in the dependency version only applies if someone
> manually runs `npm install` and then re-generates the shrinkwrap.

I'm not a JS dev, so it's very possible I'm misunderstanding how this works. But if there are no node_modules under tools/lint/eslint, then we do call `npm install`[1] which seems to interpret the ^ in the version. Contrast with automation which is pinned to 3.9 by virtue of tooltool. So I think someone could potentially end up installing a different version locally.

At any rate, I'll just pin it to 3.9 as that seems like what will happen anyway.

[1] https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint.lint#82

Comment 14

[Kris Maglione [:kmag]]

(In reply to Andrew Halberstadt [:ahal] from comment #13)
> I'm not a JS dev, so it's very possible I'm misunderstanding how this works.
> But if there are no node_modules under tools/lint/eslint, then we do call
> `npm install`[1] which seems to interpret the ^ in the version. Contrast
> with automation which is pinned to 3.9 by virtue of tooltool. So I think
> someone could potentially end up installing a different version locally.
> 
> At any rate, I'll just pin it to 3.9 as that seems like what will happen
> anyway.

When I run it, at any rate, the shrinkwrap overrides the package.json. The normal practice in Node development is to use loose versioning so that you wind up with the latest version that shouldn't have breaking changes. The shrinkwrap is necessary for automation, but I still think that sticking to Node conventions in the package.json makes sense.

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Update eslint to 3.9

Keyword is *shouldn't* :). Here's the update to 3.9 on try and there are a bunch of new lint infractions:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b69b0377edc944ad2cccbdf33295cf8f9fd20f&selectedJob=30184026

Anyway, here is the patch for updating to 3.9, the eslint zip for this is already on tooltool. Please let me know if I should do a different version instead, otherwise I'll assume my role here is done.

Comment 16

[Kris Maglione [:kmag]]

(In reply to Andrew Halberstadt [:ahal] from comment #15)
> Created attachment 8806086 [details] [diff] [review]
> Update eslint to 3.9
> 
> Keyword is *shouldn't* :). Here's the update to 3.9 on try and there are a
> bunch of new lint infractions:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=27b69b0377edc944ad2cccbdf33295cf8f9fd20f&selectedJob=3
> 0184026
> 
> Anyway, here is the patch for updating to 3.9, the eslint zip for this is
> already on tooltool. Please let me know if I should do a different version
> instead, otherwise I'll assume my role here is done.

Hum. It looks like this was intentionally regressed:

https://github.com/eslint/eslint/commit/6e9ff08cf8ac9188331fcea7905ce162accefe81#diff-f7ae198ea24182d178b8f3376e61b6ed

Can you please downgrade to 3.8.1, and I'll file a bug to fix this in eslint?

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8805884 [details]
Bug 1313956: Async all the tests.

That looks better:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3d424cbdee74aaafa7d7a885833600cd6ad227d

Comment 18

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1313956 - Upgrade ESLint to 3.8.1 for improved async/await support,

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

Comment 19

[Andrew Halberstadt [:ahal]]

I don't think I'm able to push to the same review request.. feel free to land that change on my behalf once r+ is granted.

Comment 20

[Kris Maglione [:kmag]]

(In reply to Andrew Halberstadt [:ahal] from comment #19)
> I don't think I'm able to push to the same review request.. feel free to
> land that change on my behalf once r+ is granted.

Thanks!

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8805885 [details]
Bug 1313956: Async all the functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89494/diff/1-2/

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8805884 [details]
Bug 1313956: Async all the tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89492/diff/1-2/

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8806111 [details]
Bug 1313956 - Upgrade ESLint to 3.8.1 for improved async/await support,

https://reviewboard.mozilla.org/r/89622/#review89774

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a09ebf6ed7d2
Upgrade ESLint to 3.8.1 for improved async/await support, r=miker

Comment 25

[Kris Maglione [:kmag]]

I think I'm going to hold off landing the Task.jsm conversion until after the merge, so async functions have a bit more time to bake, but I'm landing the ESLint changes now so they don't bitrot in the mean time.

Comment 26

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

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

Comment 27

[Andrew Swan [:aswan]]

Comment on attachment 8805884 [details]
Bug 1313956: Async all the tests.

https://reviewboard.mozilla.org/r/89492/#review90882

Whew, this was epic, thanks for all the work.
I have a number of comments below but even as-is this patch makes things much better so we should go ahead and get it landed.
The two biggest opportunities I see for further improvement are
1. We have a number of instances of code that iterates through some sort of list, doing something asynchronous on each item.  Code that pre-dates async/Task does this with recursion and its a familiar pattern at this point, but async lets us write this code as regular for or while loops with await statements inside the loop body which I think is much simpler to read.
2. There are still a number of places where we have promises with error handlers (as the second argument to .then)  Maybe its just me, but I find a mixture of await and Promise handlers to be harder to read than code that uses one or the other exclusively.  I think most if not all of these can change to just use try/catch blocks and not have any explicit Promise handlers at all.

::: browser/components/extensions/test/browser/browser_ext_incognito_popup.js:60
(Diff revision 2)
> -        return testWindow(window);
> -      }).then(() => {
> -        return browser.windows.create({incognito: true, url: URL});
> -      }).then(window => {
> -        return windowReady.then(() => {
> -          return testWindow(window);
> -        }).then(() => {
> -          return browser.windows.remove(window.id);
> -        });
> -      }).then(() => {
> +        {
> +          let window = await browser.windows.getCurrent();
> +
> +          await testWindow(window);
> +        }
> +
> +        {
> +          let window = await browser.windows.create({incognito: true, url: URL});
> +          await windowReady;
> +
> +          await testWindow(window);
> +
> +          await browser.windows.remove(window.id);
> +        }

why the nested blocks here?

::: browser/components/extensions/test/browser/browser_ext_tabs_cookieStoreId.js:51
(Diff revision 2)
> +        try {
> -        // Tab Creation
> +          // Tab Creation
> -        browser.tabs.create({windowId: data.privateTab ? this.privateWindowId : this.defaultWindowId,
> -                             cookieStoreId: data.cookieStoreId})
> -
> +          let tab = await browser.tabs.create({
> +            windowId: data.privateTab ? this.privateWindowId : this.defaultWindowId,
> +            cookieStoreId: data.cookieStoreId,
> +          }).then((tab) => {

What's going on here?  It looks like the chained handlers here could be separate blocks separated by await statements...

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript.js:131
(Diff revision 2)
>            };
>            browser.test.assertEq(`No window matching ${JSON.stringify(details)}`,
>                                  error.message, "Got expected error");
>          }),
>  
> -        browser.tabs.create({url: "http://example.net/", active: false}).then(tab => {
> +        browser.tabs.create({url: "http://example.net/", active: false}).then(async tab => {

Could the enclosing function here switch to a series of "await ..." statements instead of the big Promise.all(), then you wouldn't need chained handlers here

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript_runAt.js:30
(Diff revision 2)
>        if (tries++ == MAX_TRIES) {
> -        return Promise.reject(new Error("Max tries exceeded"));
> +        throw new Error("Max tries exceeded");
>        }

Now that we're all async-ified, I think this would be easier to follow if this was a simple `while (tries++ < MAX_TRIES)` loop instead of this recursive thing.

::: browser/components/extensions/test/browser/browser_ext_tabs_insertCSS.js:14
(Diff revision 2)
>    let responseManagersSize = MessageChannel.responseManagers.size;
>  
>    let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/", true);
>  
> -  function background() {
> +  async function background() {
>      let promises = [

To state the obvious, the things in this array are not actually promises, which makes `promises` not a very good name.

::: browser/components/extensions/test/browser/browser_ext_tabs_insertCSS.js:41
(Diff revision 2)
>        let computedStyle = window.getComputedStyle(document.body);
>        return [computedStyle.backgroundColor, computedStyle.color];
>      }
>  
> -    function next() {
> +    async function next() {
>        if (!promises.length) {

Same comment as above about writing the looping structure here directly instead of using recursion

::: browser/components/extensions/test/browser/browser_ext_tabs_move.js:72
(Diff revision 2)
> -      browser.tabs.query(
> -        {lastFocusedWindow: true},
> +      let [, tab] = await browser.tabs.query({lastFocusedWindow: true});
> +
> -        tabs => {
> -          let tab = tabs[1];
> -          // Assuming that tab.id of 12345 does not exist.
> +      // Assuming that tab.id of 12345 does not exist.
> -          browser.tabs.move([tab.id, 12345], {index: 0})
> +      await browser.tabs.move([tab.id, 12345], {index: 0}).then(

Why not try/await here?

::: browser/components/extensions/test/browser/browser_ext_tabs_move_window.js:21
(Diff revision 2)
> -            browser.test.assertEq(tabs[0].windowId, destination.windowId);
> -            browser.test.notifyPass("tabs.move.window");
> -          });
>  
> -        // Assuming that this windowId does not exist.
> +      // Assuming that this windowId does not exist.
> -        browser.tabs.move(source.id, {windowId: 123144576, index: 0})
> +      await browser.tabs.move(source.id, {windowId: 123144576, index: 0}).then(

Same comment as above about using try

::: browser/components/extensions/test/browser/browser_ext_tabs_removeCSS.js:58
(Diff revision 2)
>        let computedStyle = window.getComputedStyle(document.body);
>        return [computedStyle.backgroundColor, computedStyle.color];
>      }
>  
> -    function next() {
> +    async function next() {
>        if (!promises.length) {

same comment about the loop stucture

::: browser/components/extensions/test/browser/browser_ext_tabs_zoom.js:164
(Diff revision 2)
>          checkZoom(tabIds[1], 1, 1.5),
>        ]);
> -    }).then(() => {
> +
> +
>        browser.test.log("Check that invalid zoom values throw an error");
> -      return browser.tabs.setZoom(tabIds[0], 42).then(
> +      await browser.tabs.setZoom(tabIds[0], 42).then(

Use a try {} ?

::: browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js:63
(Diff revision 2)
>            return;
>          }
>  
> -        browser.webNavigation.getAllFrames({tabId}).then((getAllFramesDetails) => {
> -          let getFramePromises = getAllFramesDetails.map((frameDetail) => {
> -            let {frameId} = frameDetail;
> +        let getAllFramesDetails = await browser.webNavigation.getAllFrames({tabId});
> +
> +        let getFramePromises = getAllFramesDetails.map(({frameId}) => {

nit, braces no longer needed

::: browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js:82
(Diff revision 2)
> -          while (getAllFramesDetails.filter((details) => details.frameId == nonExistentFrameId).length > 0) {
> +        while (getAllFramesDetails.filter((details) => details.frameId == nonExistentFrameId).length > 0) {
> -            nonExistentFrameId += 1;
> +          nonExistentFrameId += 1;
> -          }
> +        }
>  
> -          // Check that getFrame Promise is rejected with the expected error message on nonexistent frameId.
> +        // Check that getFrame Promise is rejected with the expected error message on nonexistent frameId.
> -          browser.webNavigation.getFrame({tabId, frameId: nonExistentFrameId, processId: 20}).then(() => {
> +        await browser.webNavigation.getFrame({tabId, frameId: nonExistentFrameId, processId: 20}).then(

use try {} ?

::: browser/components/extensions/test/browser/browser_ext_windows_create.js:55
(Diff revision 2)
> -          });
> -        });
>        }
>  
> -      browser.runtime.getPlatformInfo().then(info => { os = info.os; })
> -      .then(() => createWindow({state: "maximized"}, {state: "STATE_MAXIMIZED"}))
> +      try {
> +        let info = browser.runtime.getPlatformInfo();

`let {os}`

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:53
(Diff revision 2)
> -      browser.test.log("Close the new window");
> +        browser.test.log("Close the new window");
> -      return browser.windows.remove(window.id);
> -    }).then(() => {
> +        await browser.windows.remove(window.id);
> +      }
> -      browser.test.log("Create a new private window");
>  
> -      return browser.windows.create({incognito: true});
> +      {

is the nested block necessary?

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:90
(Diff revision 2)
>                                    "Create call failed as expected");
> -        }).then(() => {
> -          browser.test.log("Try to create a window with both a tab and an invalid incognito setting");
> +        });
> +
>  
> -          return browser.windows.create({tabId: tab.id, incognito: true});
> -        }).then(
> +      browser.test.log("Try to create a window with both a tab and an invalid incognito setting");
> +      await browser.windows.create({tabId: tab.id, incognito: true}).then(

use try {} ?

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:101
(Diff revision 2)
> -    }).then(() => {
> -      browser.test.log("Try to create a window with an invalid tabId");
>  
> -      return browser.windows.create({tabId: 0}).then(
> +
> +      browser.test.log("Try to create a window with an invalid tabId");
> +      await browser.windows.create({tabId: 0}).then(

try{}

::: browser/components/extensions/test/browser/browser_ext_windows_update.js:93
(Diff revision 2)
>        }
>  
> +      try {
> -      let windowId = browser.windows.WINDOW_ID_CURRENT;
> +        let windowId = browser.windows.WINDOW_ID_CURRENT;
>  
> -      browser.runtime.getPlatformInfo().then(info => { os = info.os; })
> +        let info = await browser.runtime.getPlatformInfo();

`let {os}`

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:92
(Diff revision 2)
> -      browser.test.assertEq(STORE_ID, stores[0].id, "expected store id returned");
> +    browser.test.assertEq(STORE_ID, stores[0].id, "expected store id returned");
> -      browser.test.assertEq(1, stores[0].tabIds.length, "one tab returned for store");
> +    browser.test.assertEq(1, stores[0].tabIds.length, "one tab returned for store");
> -      return browser.windows.create({incognito: true});
> -    }).then(privateWindow => {
> -      return browser.cookies.getAllCookieStores().then(stores => {
> +
> +    {
> +      let privateWindow = await browser.windows.create({incognito: true});
> +      let stores = await browser.cookies.getAllCookieStores();

You have this in the outer scope, no need to shadow it here.
In any case, why is this in a nested block?

::: toolkit/components/extensions/test/mochitest/test_ext_storage_content.html:111
(Diff revision 2)
> -  // Test cache invalidation.
> +    // Test cache invalidation.
> -  }).then(() => {
> -    return storage.set({"test-prop1": "value1", "test-prop2": "value2"});
> +    await storage.set({"test-prop1": "value1", "test-prop2": "value2"});
> +
> -  }).then(() => {
>      globalChanges = {};
>      // Schedule sendMessage after onMessage because the other end immediately

I don't understand this comment, but I guess you're not actually changing anything here...

::: toolkit/components/extensions/test/mochitest/test_ext_web_accessible_resources.html:80
(Diff revision 2)
>        [browser.extension.getURL("wild1.html"), true],
>        [browser.extension.getURL("wild2.htm"), false],
>      ];
>  
> -    function runTest() {
> +    async function runTest() {
>        if (!urls.length) {

comment above about loop structure

::: toolkit/components/extensions/test/xpcshell/test_ext_native_messaging.js:182
(Diff revision 2)
>  add_task(function* test_sendNativeMessage() {
> -  function background() {
> +  async function background() {
>      let MSG = {test: "hello world"};
>  
>      // Check error handling
> -    browser.runtime.sendNativeMessage("nonexistent", MSG).then(() => {
> +    await browser.runtime.sendNativeMessage("nonexistent", MSG).then(() => {

try {}

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_errors.js:39
(Diff revision 2)
>        args = args.map(arg => arg === null ? undefined : arg);
>        testCases.push([args, expectedError]);
>      }
>  
> -    function next() {
> +    async function next() {
>        if (!testCases.length) {

loop comment from above

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_errors.js:47
(Diff revision 2)
>        }
>        let [args, expectedError] = testCases.shift();
>        let description = `runtime.sendMessage(${args.map(String).join(", ")})`;
> -      return browser.runtime.sendMessage(...args)
> +
> +      await browser.runtime.sendMessage(...args)
>          .then(() => {

try {}

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8805884 [details]
Bug 1313956: Async all the tests.

https://reviewboard.mozilla.org/r/89492/#review90882

I skipped some of the cases where .then() clauses were part of separate promise chains, and didn't take too much away from the code's maintainability, mainly to simplify the diff. I might go back and change some of them as a follow-up.

And, yeah, I was thinking the same thing about the code that iterates over lists, mainly the pageAction/browserAction code. But, again, I think it's better as a follow-up.

> why the nested blocks here?

Because the `window` variable goes out of scope when we close it, and we don't wind up re-using the same variable for multiple windows in ways that aren't always obvious.

> What's going on here?  It looks like the chained handlers here could be separate blocks separated by await statements...

I'm probably going to take another pass at this in a follow-up. I couldn't do much more without the diff looking like a complete rewrite.

> Could the enclosing function here switch to a series of "await ..." statements instead of the big Promise.all(), then you wouldn't need chained handlers here

Yes, but the parallelism is intentional. Aside from making the test a bit faster, it's likely to turn up any weird timing errors that happen to arise. I agree that it would probably be a bit more readable if it were a series of `await` statements, though.

> Now that we're all async-ified, I think this would be easier to follow if this was a simple `while (tries++ < MAX_TRIES)` loop instead of this recursive thing.

Yeah, I actually had a nagging feeling that things were more complicated than they need to be now... but there were so many tests to fix...

> To state the obvious, the things in this array are not actually promises, which makes `promises` not a very good name.

Indeed, but you should tell that to the original reviewer :P I didn't change any of this.

> Why not try/await here?

Because of the error handler. I was thinking about adding `browser.test.rejects` and `browser.test.throws` to mach `Assert.rejects` and `Assert.throws`, and then come back and clean all of these up.

> nit, braces no longer needed

As a rule, I prefer to use braces and explicit `return` statements when the callbacks need to be wrapped over multiple lines. In my experience, the alternative is a lot harder to read and refactor when you come back to it later.

> is the nested block necessary?

Not strictly, but it makes it easier to track when the lexical variables go out of scope.

> You have this in the outer scope, no need to shadow it here.
> In any case, why is this in a nested block?

Same, to make it easier to track the lifetime of variables.

> I don't understand this comment, but I guess you're not actually changing anything here...

I understand it, but I agree that it's confusing.

Comment 29

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efc9b52a218f7ffd40ba346de74fd846a9059ceb
Bug 1313956: Async all the tests. r=aswan

Comment 30

[Carsten Book [:Tomcat]]

backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38818987&repo=mozilla-inbound

Comment 31

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c6db8195588
Backed out changeset efc9b52a218f for failing on own tests

Comment 32

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f56bd071c220800ca8e27054495fd42c7b3e624
Bug 1313956: Async all the tests. r=aswan

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f56bd071c22

Comment 34

[Mike Conley (:mconley) (:⚙️)]

markh just pointed me at this bug. The folks in #jsapi requested that we not use async/await just yet (or to let it percolate for a release) before doing whole-hog conversions like this. Is it too late to backout and wait a cycle?

Comment 35

[Kris Maglione [:kmag]]

I'm not landing any use in production code for now, only in test code that can't use Task.jsm. I have a plan to convert that code to use generators and Task.js if it becomes necessary, but the maintainability improvement at this point is too back it out, and I'd rather not waste time converting it to Task.js and back again if it's not necessary.

Comment 36

[Andy McKay]

Fixed by florians mass convert patch.

Comment 37

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.