1233890 - Stop using deprecated-sync-thenables in RDP server

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Comment 1

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

Attachment: MozReview Request: Bug 1233890 - Convert to Promise.jsm in RDP server. r=ejpbruel

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

Comment 2

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54f36d12bf3a

Comment 3

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8700246 [details]
MozReview Request: Bug 1233890 - Convert to Promise.jsm in RDP server. r=ejpbruel

https://reviewboard.mozilla.org/r/28667/#review25525

Make sure you get try runs for these changes. I've found that these changes are generally easy to make, but can lead to failing tests due to subtle race conditions, etc.

Comment 4

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

Comment on attachment 8700246 [details]
MozReview Request: Bug 1233890 - Convert to Promise.jsm in RDP server. r=ejpbruel

:ejpbruel said on IRC that he meant to set r+ here.

Comment 5

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

Haven't had time to return to this.

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

We might be resolve as WONTFIX this bug or update the subject to use `new Promise`, due bug 1368456.

Comment 7

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

These bugs are less about Promise.jsm as a destination and more about removing deprecated-sync-thenables.

Comment 8

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e090386d6c2e0e9a9bb87886bd3d2e0059e01f28

Comment 9

[Alexandre Poirot [:ochameau]]

There is only one module still using the sync promises:
  https://searchfox.org/mozilla-central/source/devtools/server/main.js
Most of the usages don't look justified.
I only fear the one usage in queueResponse to be necessary.

Here is a try run with all but queueResponse usage refactored:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=18c17eb2a14cdd00819e332fc67ad130aeaf9076
And another one with queueResponse refactored:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4115d7915cc904d822061ae14fc4f36840a4714

Comment 10

[Alexandre Poirot [:ochameau]]

As expected, changing queueResponse behavior introduce test failure, whereas all the rest doesn't seem to.
(It may introduce intermittent? Is there a try syntax to force triggerring all tests X times?)

So queueResponse switch to async promise makes only one test to fail:
  https://treeherder.mozilla.org/logviewer.html#?job_id=161948677&repo=try&lineNumber=2390
  https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_addons_reload.js
It is surprising, but only one test seem to fail.
It fails because it doesn't correctly wait for the RDP requests to finish before closing about:debugging.
It may highlight an existing intermittent, I don't think it highlight any breakage due to the switch to async promise.

Comment 11

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

(In reply to Alexandre Poirot [:ochameau] from comment #10)
> As expected, changing queueResponse behavior introduce test failure, whereas
> all the rest doesn't seem to.
> (It may introduce intermittent? Is there a try syntax to force triggerring
> all tests X times?)

To force test jobs to run multiple times, there's `--rebuild N` for try syntax, is what you mean?

Comment 12

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

Also, locally you can run --verify on some test directory, which is quite handy for specific test files, but would take forever to run all of DevTools...  The TV job in automation runs this, but only for changed test files.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1233890 - Remove unnecessary usages of deprecated sync promises in devtools/server.

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

Comment 14

[Code Review Bot [:reviewbot]]

Comment on attachment 8955205 [details]
Bug 1233890 - Remove unnecessary usages of deprecated sync promises in devtools/server.

https://reviewboard.mozilla.org/r/224376/#review230314


Code analysis found 2 defects in this patch:
 - 2 defects 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/server/main.js:1084
(Diff revision 1)
> -    let onBrowserSwap = ({ detail: newFrame }) => {
> +      let onBrowserSwap = ({ detail: newFrame }) => {
> -      // Remove listeners from old frame and mm
> +        // Remove listeners from old frame and mm
> -      untrackMessageManager();
> +        untrackMessageManager();
> -      // Update frame and mm to point to the new browser frame
> +        // Update frame and mm to point to the new browser frame
> -      frame = newFrame;
> +        frame = newFrame;
> -      // Get messageManager from XUL browser (which might be a specialized tunnel for RDM)
> +        // Get messageManager from XUL browser (which might be a specialized tunnel for RDM)

Error: Line 1084 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/main.js:1172
(Diff revision 1)
> -    // Listen for connection close to cleanup things
> +      // Listen for connection close to cleanup things
> -    // when user unplug the device or we lose the connection somehow.
> +      // when user unplug the device or we lose the connection somehow.
> -    EventEmitter.on(connection, "closed", destroy);
> +      EventEmitter.on(connection, "closed", destroy);
>  
> -    mm.sendAsyncMessage("debug:connect", { prefix, addonId });
> +      mm.sendAsyncMessage("debug:connect", { prefix, addonId });
> -
> +   });

Error: Expected indentation of 4 spaces but found 3. [eslint: indent-legacy]

Comment 15

[Alexandre Poirot [:ochameau]]

A try run with --rebuild 5 to highlight potential intermittent:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d99e35029714143a13ac1a8827bdf13d468d59ef

Comment 16

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

Comment on attachment 8955205 [details]
Bug 1233890 - Remove unnecessary usages of deprecated sync promises in devtools/server.

https://reviewboard.mozilla.org/r/224376/#review230394

Thanks for working on this! :) These changes look good to me.

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8955205 [details]
Bug 1233890 - Remove unnecessary usages of deprecated sync promises in devtools/server.

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

Comment 18

[Alexandre Poirot [:ochameau]]

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b59e57f7bd4ab2c91a3c1ba9488bd4bde05fcbc

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/921a91c6538b
Remove unnecessary usages of deprecated sync promises in devtools/server. r=jryans

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/921a91c6538b