1056048 - Make browser/devtools/markupview browser mochitests use Promise.jsm instead of deprecated-sync-thenables

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/head.js#9

Comment 1

[Sami Jaktholm]

Attachment: markupview-deprecated-sync-thenables.patch

Here's a patch that introduces Promise.jsm to markupview/markup-view.js and markupview/test/head.js.

There was only two places where Promise.jsm caused the promises to resolve after Markup View was destroyed. And that should only be possible during testing as I don't see a way for user to for example select a node and then destroy the toolbox on the next clock cycle.

Task.jsm with cancellable tasks would be useful in these kind of situations as these pending tasks could just be aborted when the view is destroyed instead of checking it "manually" every step in the promise chain...

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=15c6f960933d

Comment 2

[Patrick Brosset <:pbro>]

Thanks Sami. I've re-triggered a few test jobs on that try push, just to make sure because moving from a sync to an async promise implementation can lead to a lot of unexpected failures.
Cancellable async tasks would be awesome indeed, it's too bad that we have to check everything at each step of async functions.
Will review the code now.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8536365 [details] [diff] [review]
markupview-deprecated-sync-thenables.patch

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

::: browser/devtools/markupview/markup-view.js
@@ +366,5 @@
>  
>        this.showNode(selection.nodeFront, true).then(() => {
> +        if (this._destroyer) {
> +          return promise.reject("Could not mark node as selected, MarkupView " +
> +            "was destroyed while showing the node.");

yay for the developer-friendly message here. This helps.

@@ +372,5 @@
>          if (selection.reason !== "treepanel") {
>            this.markNodeAsSelected(selection.nodeFront);
>          }
>          done();
> +      }).then(null, (e) => {

nit: we usually don't enclose single arrow function args in parens.

@@ +377,1 @@
>          console.error(e);

Why not just a console.warn here? The logic here is that if this case ever happens, we are prepared to handle it, but just want to leave a trace in the logs that it did happen. I think a warning would be enough for this.

In fact, I think a common pattern would be to issue a warning when we know the action will fail because this._destroyer exists (that's the expected case), but we should keep throwing an error with stack trace whenever a promise is rejected but this._destroyer doesn't exist.
I like how this is done here for instance: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#763

Comment 4

[Patrick Brosset <:pbro>]

fwiw, here's a test that rejects the promise and shows the console.error:

add_task(function*() {
  let {inspector} = yield addTab("data:text/html,<p><span>test</span></p>").then(openInspector);
  ok(true);
  let nodeFront = yield getNodeFront("span", inspector);
  inspector.selection.setNodeFront(nodeFront, "test");
});

Comment 5

[Sami Jaktholm]

Attachment: markupview-deprecated-sync-thenables.patch

Thank you for the review. 

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> In fact, I think a common pattern would be to issue a warning when we know
> the action will fail because this._destroyer exists (that's the expected
> case), but we should keep throwing an error with stack trace whenever a
> promise is rejected but this._destroyer doesn't exist.

That makes sense.

Here's a revised version that prints an error only if markup view is not destroyed.

Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6e6a77624eb9

Comment 6

[Sami Jaktholm]

Try run(s) is mostly green, only hitting bug 1066474 in timeline actor tests which aren't related to markup view in any way.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/63c3a30697cb

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63c3a30697cb