Closed Bug 881050 Opened 7 years ago Closed 4 months ago

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

Categories

(DevTools :: General, defect, P5)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Paolo, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: meta, Whiteboard: [only do this if you understand promises])

Attachments

(10 obsolete files)

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.
Whiteboard: [mentor=Yoric][lang=js][only do this if you understand promises]
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.
Depends on: 834756
I'll work on this.
Assignee: nobody → past
Status: NEW → ASSIGNED
How are things going, Panos?
Flags: needinfo?(past)
Due to vacations and work weeks I haven't started working on this yet, but it's high on my priority list.
Flags: needinfo?(past)
Panos, I'm not sure if you're working on this, but if not I'd be willing to do it.
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.
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.
Attached patch promise.js -> promise.jsm (obsolete) — Splinter Review
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.
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.
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.
As discussed with Brandon, he is going to pick up this work, so that I can focus on other things.
Assignee: past → bbenvie
Brandon, are you working on this?
Flags: needinfo?(bbenvie)
Attached patch stubify-promise.patch (obsolete) — Splinter Review
This patch is mainly just promise.js and Promise.jsm stub.
Attachment #821050 - Attachment is obsolete: true
Attachment #821051 - Attachment is obsolete: true
Attachment #821056 - Attachment is obsolete: true
Flags: needinfo?(bbenvie)
Attached patch promise-conversion.patch (obsolete) — Splinter Review
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.
(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.
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.
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.
Attached patch promise-conversion.patch WIP3 (obsolete) — Splinter Review
Fixes the framework failures.
Attachment #828222 - Attachment is obsolete: true
(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.
Flags: needinfo?(past)
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.
Flags: needinfo?(past)
Depends on: 940537
Depends on: 940538
Depends on: 940541
Depends on: 940542
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.
Attached patch promise-conversion.patch (obsolete) — Splinter Review
Breaks off the stuff moved into different patches and also some of this into subpieces.
Attachment #830486 - Attachment is obsolete: true
Attached patch promise-style.patch (obsolete) — Splinter Review
The changes to styleeditor and styleinspector.
Attached patch promise-framework.patch (obsolete) — Splinter Review
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.
Attached patch promise-framework.patch (obsolete) — Splinter Review
Fixes test failures in inspector except for browser_inspector_pseudoclass-lock.js timing out.
Attachment #8335706 - Attachment is obsolete: true
Depends on: 943510
Summary: Use the new "Promise.jsm" implementation in Developer Tools → [meta] Use the new "Promise.jsm" implementation in Developer Tools
Since this touches so many different areas, I'm splitting everything off into their own bugs so as not to drive reviewers insane.
Depends on: 943512
Depends on: 943517
Depends on: 943621
Depends on: 943669
Depends on: 943672
Depends on: 943673
Depends on: 943681
Depends on: 943685
Brandon, are you still working on this bug?
Flags: needinfo?(bbenvie)
Yeah, although this is a meta bug now. Progress for each component is tracked in individual bugs.
Flags: needinfo?(bbenvie)
not a mentorable bug.
Whiteboard: [mentor=Yoric][lang=js][only do this if you understand promises] → [only do this if you understand promises]
Depends on: 982597
Assignee: bbenvie → nobody
Attachment #828207 - Attachment is obsolete: true
Attachment #8335702 - Attachment is obsolete: true
Attachment #8335703 - Attachment is obsolete: true
Attachment #8338178 - Attachment is obsolete: true
Status: ASSIGNED → NEW
Priority: -- → P5
Depends on: 995170
No longer blocks: 881047
Depends on: 1233887
Depends on: 1233888
Depends on: 1233891
Depends on: 1233890
Depends on: 1233894
Close as WONTFIX since bug 1368456 (and possibly bug 1283869).
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: [meta] Use the new "Promise.jsm" implementation in Developer Tools → [meta] Use Promise instead of "deprecated-sync-thenables" in Developer Tools
Depends on: 1406915
Depends on: 1442312
No longer blocks: 1439048
Product: Firefox → DevTools
No longer blocks: dt-fission

All dependencies are now fixed.

Status: REOPENED → RESOLVED
Closed: 2 years ago4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.