Closed Bug 1233891 Opened 4 years ago Closed 4 months ago

Stop using deprecated-sync-thenables in RDP client

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: jryans, Assigned: past)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Comment on attachment 8700247 [details]
MozReview Request: Bug 1233891 - Convert to Promise.jsm in RDP client. r=ejpbruel

https://reviewboard.mozilla.org/r/28671/#review25527

LGTM. Thanks for doing this Ryan.
Attachment #8700247 - Flags: review?(ejpbruel)
Comment on attachment 8700247 [details]
MozReview Request: Bug 1233891 - Convert to Promise.jsm in RDP client. r=ejpbruel

https://reviewboard.mozilla.org/r/28671/#review25529
Attachment #8700247 - 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.
These bugs are less about Promise.jsm as a destination and more about removing deprecated-sync-thenables.
Summary: Convert to Promise.jsm in RDP client → Stop using deprecated-sync-thenables in RDP client
The only usage from devtools/client is from old debugger frontend:
  https://searchfox.org/mozilla-central/source/devtools/client/debugger/debugger-controller.js
For this one, I think it will be easier to wait for it to be finally removed (bug 1314057)

Otherwise, this is still used in many Client classes from here:
  https://searchfox.org/mozilla-central/source/devtools/shared/client
So most of this bug would be to switch to DOM promise in this devtools/shared/client folder and see what breaks.
Depends on: 1314057
A try run with all devtools/shared/client refactored to DOM promise, all but DebuggerClient.request, which I suspect to be sensible to races:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=12153e91e75dc6479bb8393c3e9fe3a6774aa571
Another one, also including Debugger.request:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba84a8f82e1e1eb3029537769b9c705264857180
The result is that modifying Debugger.request breaks tons of tests with the following exception:
  A promise chain failed to handle a rejection: 'detach' active request packet to 'server1.conn17.child1/context25' can't be sent as the connection just closed. - stack: listenerJson@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:635:11
  Visible on this try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba84a8f82e1e1eb3029537769b9c705264857180&selectedJob=161953017

I think it would justify addressing this in its own bug/patch.

Otherwise, switching the rest of client APIs also introduce failures, but only against three tests:
  * devtools/client/shared/test/browser_toolbar_basic.js
    A promise chain failed to handle a rejection: this._remote is null - stack: null
    https://treeherder.mozilla.org/logviewer.html#?job_id=162034061&repo=try&lineNumber=17037
  * devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js
    A promise chain failed to handle a rejection: 'detach' request packet to 'server1.conn17.tab1' can't be sent as the connection is closed. - stack: request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:623
    https://treeherder.mozilla.org/logviewer.html#?job_id=162034089&repo=try&lineNumber=2687
  * devtools/server/tests/unit/test_client_request.js
    [test_client_request_after_close_callback : 251] A promise chain failed to handle a rejection: 'hello' request packet to 'server1.conn0.test1' can't be sent as the connection is closed. - stack: request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:623:14
    test_client_request_after_close_callback@/builds/worker/workspace/build/tests/xpcshell/tests/devtools/server/tests/unit/test_client_request.js:203:3
    https://treeherder.mozilla.org/logviewer.html#?job_id=162034077&repo=try&lineNumber=2674
Blocks: 1439048
Comment on attachment 8955203 [details]
Bug 1233891 - Use DOM promises instead of deprecated sync promises in devtools/shared.

https://reviewboard.mozilla.org/r/224370/#review230316


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/client/debugger-client.js:631
(Diff revision 1)
>      request.format = "json";
>      request.stack = getStack();
>  
>      // Implement a Promise like API on the returned object
>      // that resolves/rejects on request response
>      let deferred = promise.defer();

Error: 'promise' is not defined. [eslint: no-undef]
Attachment #8700247 - Attachment is obsolete: true
No longer blocks: 1439048
Product: Firefox → DevTools
Comment on attachment 8955203 [details]
Bug 1233891 - Use DOM promises instead of deprecated sync promises in devtools/shared.

Oops, I didn't meant to ask for review yet.
I'm not sure anything happened that would have fixed the issues we had in the past to switch to DOM Promises. So this is just rebased patches to see what fails.
Attachment #8955203 - Flags: review?(jryans)
Attachment #8955204 - Flags: review?(jryans)
No longer blocks: dt-fission
Assignee: nobody → past
Status: NEW → ASSIGNED

I've got a green enough try run. The changes are mostly a few catches here and there and some styling changes today after the prettier introduction to m-c.

Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d96361cf14f
Use DOM promises instead of deprecated sync promises in devtools/. r=ochameau
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Duplicate of this bug: 1233894
You need to log in before you can comment on or make changes to this bug.