881050 - [meta] Use Promise instead of "deprecated-sync-thenables" in Developer Tools

Status: RESOLVED FIXED

(DevTools :: General, defect, P5)

Description

[:Paolo Amadini]

Now that bug 810490 landed, we can look into switching the Developer Tools
to our new "Promises/A+" compliant implementation. This may be tried out on
top of "Promise.jsm", so that we can fix the cases where switching to a
delayed implementation of the "then" function would break some code.

Once the code of the Developer Tools is updated, we can switch the Add-on SDK
module "promise.js" to use "Promise.jsm" as its back-end, and continue to
import the SDK module to leverage its helper functions.

Fixing this requires some patience while going through all the failing tests.
For example, the JavaScript Debugger is tricky to convert because it uses
promises while enqueueing client-server packets, and delayed resolution of
"then" may make those packets go out of order. This would be easy to fix by
making each outgoing packet wait on the completion of the previous one,
except that an outgoing packet handler may trigger a nested event loop,
thus blocking the queue. So, another solution should be found here.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Generally, the steps are the following:
- in directory browser/devtools replace all occurrences of "resource://gre/modules/commonjs/sdk/core/promise.js" (the deprecated module) by "resource://gre/modules/Promise.jsm" (the new module);
- run the tests, see what breaks, fix the issues with the help of irc channels #introduction and #devtools.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

I'll work on this.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

How are things going, Panos?

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Due to vacations and work weeks I haven't started working on this yet, but it's high on my priority list.

Comment 5

[Brandon Benvie [:benvie]]

Panos, I'm not sure if you're working on this, but if not I'd be willing to do it.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

I've already started and I'm currently going through the test failures. There is still the problem on the server where we want to load the promise library using loadSubscript, which promise.js supported, but Promise.jsm doesn't. That's next on my list.

Comment 7

[Joe Walker [:jwalker]]

There are large chunks of GCLI that will break if we switch to fully async promises. The best solution would be to make GCLI use another promise implementation (maybe continue using the addon-sdk one). This should be simple to do.

Why does GCLI do this?

Originally argument resolution in GCLI was synchronous, but became async about a year ago. Command resolution is implemented using special kind of 'command' argument. Before async arguments, this made lots of sense. However there is nothing inherently async about command resolution.
Making command resolution fully async is very painful, so I made use of the addon-sdk's ability to have sync promises, and used a trick to rip the value out of a promise while staying in the same tick of the event loop.

I think that the best solution will probably be to stop command resolution from using the argument system, but I need time to investigate.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: promise.js -> promise.jsm

Here is a patch which contains the mechanical change from sdk's promise.js to Promise.jsm across devtools. A large number of tests fail with it.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Partial undoing of the conversion to Promise.jsm

Trying to fix the failed tests, I decided to concentrate on a smaller number of tools first, so in this patch I am undoing some of the conversions of the previous patch. This is just a WIP, which makes at least xpcshell and chrome mochitests work by reverting the debugger server changes, as well as gcli changes for the reasons Joe mentioned.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make Promise.jsm usable from mozIJSSubScriptLoader

This is my attempt to fix the debugger server issues properly, by splitting Promise.jsm in two files: one small Promise.jsm stub that just loads promise.js, which itself can be loaded using loadSubscript. This is the same approach we use for DevToolsUtils.jsm, which also needs to be loaded in the debugger server.

The changes to promise.js (after a straight copy from Promise.jsm) are to remove the EXPORTED_SYMBOLS declaration, convert the methods of the Promise object to properties of 'this' and use an arrow function for all() in order to close over 'this'.

With this patch xpcshell tests pass again, as do most of the debugger mochitests (about a dozen still fail). I haven't investigated yet why these failures occur, but I've temporarily disabled the chrome debugging tests, because they caused the test runner to fail spectacularly.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

As discussed with Brandon, he is going to pick up this work, so that I can focus on other things.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Brandon, are you working on this?

Comment 13

[Brandon Benvie [:benvie]]

Attachment: stubify-promise.patch

This patch is mainly just promise.js and Promise.jsm stub.

Comment 14

[Brandon Benvie [:benvie]]

Attachment: promise-conversion.patch

This patch is an in-progress conversion of the devtools to use Promise.jsm. browser/devtools folders confirmed to work:

* app-manager
* commandline
* fontinspector
* layoutview
* markupview
* profiler
* responseivedesign
* scratchpad
* styleeditor
* styleinspector
* sourceeditor
* tilt
* webconsole

still have issues:
* debugger
* framework
* inspector
* netmonitor
* shadereditor

The process I've been going through is converting one file at a time to Promise.jsm, running all tests for that tool, and fixing any failures that arise. So some files in the above folders have been converted successfully, but issues still remain with one or more. For example, almost all of framework has been converted except for toolbox-hosts.js.

Because of the interconnectedness, it's quite often that the offending code is found somewhere far from where the error arises or the test fails, or that it's some subtle timing dependency between two disparate pieces of code.

Comment 15

[Brandon Benvie [:benvie]]

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #12)
> Brandon, are you working on this?

Indeed, I have been working on it. I'll keep this bug more up to date on my progress.

Comment 16

[Joe Walker [:jwalker]]

The commits just added to https://github.com/joewalker/gcli/pull/16 (scroll to the bottom) fix bug 934965 and bug 830184 which prevented GCLI using the Promise.jsm form toolkit.

I hope this will land fairly soon.

Comment 17

[:Paolo Amadini]

Good to see progress on this conversion, I think it's important both to get the new Promise.jsm features and to reduce code complexity.

I see that the new "promise.js" file is loaded with "loadSubScript", and then it calls (apparently successfully) Components.utils.import for "XPCOMUtils.jsm" and "Services.jsm". So, what exactly is preventing us from using Components.utils.import in the first place?

If the issue is that you need two distinct "Promise.jsm" instances because the debugger would
otherwise stop thinking it's client code (bug 834756), can't we just create a one-line devtools-specific module (say "DebuggerServerPromise.jsm") that only calls loadSubScript on Toolkit's original "Promise.jsm" file, and import the former?

It's also not clear to me why the same problem doesn't occur with the imported "Services.jsm" and "XPCOMUtils.jsm" modules.

Comment 18

[Brandon Benvie [:benvie]]

Attachment: promise-conversion.patch WIP3

Fixes the framework failures.

Comment 19

[Brandon Benvie [:benvie]]

(In reply to :Paolo Amadini from comment #17)
> I see that the new "promise.js" file is loaded with "loadSubScript", and
> then it calls (apparently successfully) Components.utils.import for
> "XPCOMUtils.jsm" and "Services.jsm". So, what exactly is preventing us from
> using Components.utils.import in the first place?
> 
> If the issue is that you need two distinct "Promise.jsm" instances because
> the debugger would
> otherwise stop thinking it's client code (bug 834756), can't we just create
> a one-line devtools-specific module (say "DebuggerServerPromise.jsm") that
> only calls loadSubScript on Toolkit's original "Promise.jsm" file, and
> import the former?
> 
> It's also not clear to me why the same problem doesn't occur with the
> imported "Services.jsm" and "XPCOMUtils.jsm" modules.

Panos would be able to answer this better than me.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

The problem is precisely the one fixed in bug 834756 (which if you recall I had anticipated in bug 834756 comment 16), that the debugger needs to load the promises library in the same compartment as itself, so that it doesn't try to debug it. Unfortunately, the way jsm modules work is at odds with that, as every jsm is loaded in a separate compartment. So the only way that I know of to load a script in the same compartment is loadSubscript, which is why I made this arrangement of a Promise.jsm shell that wraps the promise.js core for all other consumers, while leaving the debugger with the option of just loading the core itself.

I think the reason we don't have issues with other jsm modules imported by the debugger is that these modules are only used in the startup phase of the server, before the actual debugging begins, whereas we use promises every time a remote protocol request is being handled.

Comment 21

[Brandon Benvie [:benvie]]

I'm going to break off the more problematic component conversions into their own bugs and get the bulk of the conversions landed in this bug.

Comment 22

[Brandon Benvie [:benvie]]

Attachment: promise-conversion.patch

Breaks off the stuff moved into different patches and also some of this into subpieces.

Comment 23

[Brandon Benvie [:benvie]]

Attachment: promise-style.patch

The changes to styleeditor and styleinspector.

Comment 24

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

The changes for framework. Changing any of gDevTools.jsm, target.js, or toolbox.js causes tests to fail for inspector o_O. This patch includes fixes for one of those tests but I'm still trying to figure out the others. Failures are in browser_inspector_breadcrumbs.js and browser_inspector_pseudoclass_lock.js. I don't know yet what the commonality is between these two. I believe once these two tests are fixed then these patches will be ready for review.

Comment 25

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Fixes test failures in inspector except for browser_inspector_pseudoclass-lock.js timing out.

Comment 26

[Brandon Benvie [:benvie]]

Since this touches so many different areas, I'm splitting everything off into their own bugs so as not to drive reviewers insane.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Brandon, are you still working on this bug?

Comment 28

[Brandon Benvie [:benvie]]

Yeah, although this is a meta bug now. Progress for each component is tracked in individual bugs.

Comment 29

[Rob Campbell [:rc] (:robcee)]

not a mentorable bug.

Comment 30

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

Close as WONTFIX since bug 1368456 (and possibly bug 1283869).

Comment 31

[:Paolo Amadini]

Actually, there are still instances of "deprecated-sync-thenables" in the tree that should be converted to Promise.

If some of the dependent bugs refer to code that has already been fixed, they can be closed individually.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

All dependencies are now fixed.