Closed Bug 1233890 Opened 4 years ago Closed 2 years ago

Stop using deprecated-sync-thenables in RDP server

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: jryans, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Blocks: 881050
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.
Attachment #8700246 - Flags: review?(ejpbruel)
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.
Attachment #8700246 - Flags: review+
Haven't had time to return to this.
Assignee: jryans → nobody
Status: ASSIGNED → NEW
We might be resolve as WONTFIX this bug or update the subject to use `new Promise`, due bug 1368456.
Blocks: 1384527
These bugs are less about Promise.jsm as a destination and more about removing deprecated-sync-thenables.
Summary: Convert to Promise.jsm in RDP server → Stop using deprecated-sync-thenables in RDP server
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
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.
(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?
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.
Blocks: 1439048
Depends on: 1442312
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]
Attachment #8700246 - Attachment is obsolete: true
Assignee: nobody → poirot.alex
A try run with --rebuild 5 to highlight potential intermittent:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d99e35029714143a13ac1a8827bdf13d468d59ef
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.
Attachment #8955205 - Flags: review?(jryans) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/921a91c6538b
Remove unnecessary usages of deprecated sync promises in devtools/server. r=jryans
https://hg.mozilla.org/mozilla-central/rev/921a91c6538b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
No longer blocks: 1439048
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.