For an unknown reason, switching initCssProperties function from Task.async+generator to an async function breaks something, leading to various tests to fail during toolbox closing. Here is a way to reproduce it: $ ./mach mochitest devtools/client/framework/test/browser_toolbox_tabsswitch_shortcuts.js devtools/client/framework/test/browser_toolbox_target.js ... Waiting for toolbox-ready Console message: [JavaScript Warning: "Unknown property ‘user-select’. Declaration dropped." {file: "resource://devtools/client/shared/components/reps/reps.css" line: 270}] Console message: [JavaScript Warning: "Unknown property ‘user-select’. Declaration dropped." {file: "resource://devtools/client/shared/components/reps/reps.css" line: 270}] Waiting for event: 'tabDetached' on [object Object]. Removing the iframes Waiting for toolbox-destroyed <== The test times out here https://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_toolbox_target.js#47 // And wait for toolbox-destroyed as toolbox unload is also full of // asynchronous operation that outlast unload event info("Waiting for toolbox-destroyed"); yield onToolboxDestroyed; info("Toolbox destroyed"); For some reason, "toolbox-destroyed" event is never fired? Note that something special in these tests is trigerring that. It doesn't fail with any other combination of tests running before/after these two. The first test is quite special as it opens all the tools...

Comment on attachment 8953073 [details] Bug 1440322 - Use async/await instead of Task.jsm on devtools/shared/fronts/css-properties.js. ? https://reviewboard.mozilla.org/r/222356/#review228258 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/shared/fronts/css-properties.js:251 (Diff revision 1) > } > > const cssProperties = new CssProperties(normalizeCssData(db)); > cachedCssProperties.set(client, {cssProperties, front}); > return {cssProperties, front}; > -}); > +} Error: Missing semicolon. [eslint: semi]

On this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c53c56fa69b4caa50986a230c79e24bcade7e6c&selectedJob=163699379&filter-searchStr=linux64%20opt You can see browser_toolbox_target.js failing in dt7, but that's not the only one test to fail, there is some more in dt2 and dt3, but I haven't tried to reproduce them locally. You always have to run more than one test to have a failure.

Here is what I have found so far: the problem is occurring here: https://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#1287-1291 let cssPropertiesDestroyer = this._cssPropertiesLoaded.then(({front}) => { if (front) { front.destroy(); } }); this._cssPropertiesLoaded seems to always be resolved at this point, however the `then` method is never called. We have three ways of dealing with async in this file, `Promise, `promise`, and `Task.async`. I am not too sure how to dig into the different promise implementations. I took a look already at Promise.jsm, but I'm not sure how imports work on M-C so I may be looking at the wrong thing. I am also unsure how these different implementations interact with each other. I also did some experimentation to see if we really need to use a promise, and so far I have been unable to break it with removing the promise. The only case that this would break is if `destroy` is called before `init` and I have been unable to trigger this behavior, even when delaying init arbitrarily. Here is what I tried: diff --git a/devtools/client/inspector/inspector.js b/devtools/client/inspector/inspector.js --- a/devtools/client/inspector/inspector.js +++ b/devtools/client/inspector/inspector.js @@ -150,8 +150,7 @@ Inspector.prototype = { // Localize all the nodes containing a data-localization attribute. localizeMarkup(this.panelDoc); - this._cssPropertiesLoaded = initCssProperties(this.toolbox); - yield this._cssPropertiesLoaded; + this._cssProperties = yield initCssProperties(this.toolbox); yield this.target.makeRemote(); yield this._getPageStyle(); @@ -1284,11 +1283,7 @@ Inspector.prototype = { this.animationinspector.destroy(); } - let cssPropertiesDestroyer = this._cssPropertiesLoaded.then(({front}) => { - if (front) { - front.destroy(); - } - }); + let cssPropertiesDestroyer = this._cssProperties.front.destroy(); this.sidebar.off("select", this.onSidebarSelect); let sidebarDestroyer = this.sidebar.destroy(); diff --git a/devtools/shared/fronts/css-properties.js b/devtools/shared/fronts/css-properties.js --- a/devtools/shared/fronts/css-properties.js +++ b/devtools/shared/fronts/css-properties.js @@ -247,8 +247,9 @@ const initCssProperties = async function const cssProperties = new CssProperties(normalizeCssData(db)); cachedCssProperties.set(client, {cssProperties, front}); + await new Promise(resolve => setTimeout(resolve, 10000)); // this is to demonstrate the delay return {cssProperties, front}; -} +}; /** * Synchronously get a cached and initialized CssProperties. The promise might be unnecessary and we might be able to go ahead with the above change if we don't want to touch the promise code just yet. (minus the arbitrary delay)

Ah I didn't word that quite right! this._cssPropertiesLoaded has the `then` method run in the first instance, but not in the second one, but I haven't been able to track down why yet!

(In reply to Yulia Startsev [:yulia] from comment #4) > this._cssPropertiesLoaded seems to always be resolved at this point, however > the `then` method is never called. We have three ways of dealing with async > in this file, > `Promise, `promise`, and `Task.async`. I am not too sure how > to dig into the different promise implementations. * "Promise" is DOM Promise, it is the native ones. You can use them via `Promise` object in the global scope of all modules. But depending on the module that use `Promise` symbol, it can be two different kind of Promise * "document" Promise, when used from a "browser module" "browser modules" are the modules loaded via BrowserLoader. This is all the module that have the same global scope than the panel documents. For example, all modules loaded in debugger are "browser modules". But it is different for each panel. The inspector only loads react modules as browser modules: https://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#2419 These promises are special as they follow the lifecycle of their document. So when we close the toolbox, we destroy all panel's iframes and all their promises. At that point, any reference to these promise still work, you can call: - promise.then() - resolve and reject handlers But the promise will never actually resolve/reject anymore. i.e. any callback passed to then/catch/... won't be called. * "system" Promise, when used from all other modules These promises are always alive. They will always work as expected. * "promise" are Promise.jsm ones. You can use them via promise module: let promise = require("promise"); These promises are also always alive. Their particularities is that they are implemented in JS here: https://searchfox.org/mozilla-central/source/toolkit/modules/Promise-backend.js As it is implemented in JS it may have a different behavior from native ones. * Task We often say Task.jsm for historical reasons. Task.jsm still exists in mozilla-central but is meant to be removed. devtools has a copy of it here: https://searchfox.org/mozilla-central/source/devtools/shared/task.js#14 It is also meant to be removed. Internally, it creates promises on its own. It uses defer module to do that: https://searchfox.org/mozilla-central/source/devtools/shared/defer.js And defer itself is using Promise.jsm ones.

Try run looks good! https://treeherder.mozilla.org/#/jobs?repo=try&revision=64b98b1bcf2fa293fa0ffee3c4dfe30509c5225d&selectedJob=164657339

Comment on attachment 8954420 [details] Bug 1440322 - Use async/await instead of Task.jsm on devtools/shared/fronts/css-properties.js. https://reviewboard.mozilla.org/r/223514/#review231954 I'm seeing this exception while opening and closing the inspector while it is still loading: Full message: TypeError: currentPanel is undefined Full stack: destroyInspector/this._destroyingInspector<@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js:2629:1 But it seems to happen also without your patch and doesn't break the toolbox, just log big stacks in the console. Thanks for having looked at that Yulia! ::: devtools/client/inspector/inspector.js:1286 (Diff revision 1) > > if (this.animationinspector) { > this.animationinspector.destroy(); > } > > - let cssPropertiesDestroyer = this._cssPropertiesLoaded.then(({front}) => { > + let cssPropertiesDestroyer = this._cssProperties.front.destroy(); I imagine I get the same question than you while looking at that. Can `destroy` be called before `init` finishes? It sounds unlikely. `_cssPropertiesLoaded` seems to have been introduced in bug 1265798 without real purpose.

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/214a9d4099a6 Use async/await instead of Task.jsm on devtools/shared/fronts/css-properties.js. r=ochameau