1440322 - Switching devtools/shared/fronts/css-properties.js to async function breaks toolbox destruction

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

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 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1440322 - Use async/await instead of Task.jsm on devtools/shared/fronts/css-properties.js. ?

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

Comment 2

[Code Review Bot [:reviewbot]]

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]

Comment 3

[Alexandre Poirot [:ochameau]]

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.

Comment 4

[Yulia Startsev [:yulia]]

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)

Comment 5

[Yulia Startsev [:yulia]]

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!

Comment 6

[Alexandre Poirot [:ochameau]]

(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.

Comment 7

[Yulia Startsev [:yulia]]

Attachment: Bug 1440322 - Use async/await instead of Task.jsm on devtools/shared/fronts/css-properties.js.

[Fixed Mochi tests] r=?

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

Comment 8

[Yulia Startsev [:yulia]]

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

Comment 9

[Alexandre Poirot [:ochameau]]

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.

Comment 10

[Pulsebot]

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

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/214a9d4099a6