Stop using deprecated-sync-thenables in RDP server

RESOLVED FIXED in Firefox 60

Status

()

Firefox
Developer Tools: Framework
RESOLVED FIXED
2 years ago
21 days ago

People

(Reporter: jryans, Assigned: ochameau)

Tracking

(Blocks: 3 bugs)

unspecified
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 8700246 [details]
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/
Attachment #8700246 - Flags: review?(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.
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.
(Assignee)

Updated

9 months ago
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
(Assignee)

Comment 9

2 months ago
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
(Assignee)

Comment 10

2 months ago
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.
(Assignee)

Updated

2 months ago
Depends on: 1442312
Comment hidden (mozreview-request)

Comment 14

2 months ago
mozreview-review
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]
(Assignee)

Updated

2 months ago
Attachment #8700246 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 15

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

Comment 16

2 months ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 19

2 months ago
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

Comment 20

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/921a91c6538b
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Blocks: 1451861
You need to log in before you can comment on or make changes to this bug.