1016301 - Refactor the debugger server to use native promises

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Eddy Bruel [:ejpbruel]]

One of the last dependencies on chrome in the debugger server is our use of Promise.jsm, which is blocking bug 1003097.

Rather than refactor Promise.jsm into an SDK module that can be loaded inside a worker, we should drop this dependency completely, and refactor the debugger server to use native promises, which are available in workers as well (see bug 918806).

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Convert testactors.js to use native promises

It seems that this is the only part of the server that needs to be refactored for worker debugging.

Comment 2

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

What about the deprecated-sync-thenables that are imported in script.js?

Comment 3

[Eddy Bruel [:ejpbruel]]

(In reply to Panos Astithas [:past] from comment #2)
> What about the deprecated-sync-thenables that are imported in script.js?

We want to get rid of those of course, but they do not depend on any chrome APIs, so strictly speaking getting rid of them is not a requirement for worker debugging.

Comment 4

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

Comment on attachment 8431604 [details] [diff] [review]
Convert testactors.js to use native promises

Review of attachment 8431604 [details] [diff] [review]:
-----------------------------------------------------------------

OK, but if you don't intend to complete the refactoring you should rename this bug.

Also, _sendToPrettyPrintWorker expects a |promise| object in its global scope (script.js is in a Franken-state that uses both Promise.jsm and deprecated-sync-thenables). Do you have any countermeasures in place to skip that code path, or will we get reference errors when clicking the pretty-print button?

::: toolkit/devtools/server/tests/unit/testactors.js
@@ +45,5 @@
>    constructor: TestTabList,
>    getList: function () {
> +    return new Promise((resolve, reject) => {
> +      resolve([tabActor for (tabActor of this._tabActors)]);
> +    });

Nit: native promises have Promise.resolve too, so you could keep using that.

Comment 5

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/fx-team/rev/b693a637468f

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b693a637468f

Comment 7

[Eddy Bruel [:ejpbruel]]

I've fixed the part of this bug on which bug 1003097 depended, so I'm removing that dependency.

Note that once worker debugging has reached a sufficient level of implementation, I want to spend some time on cleaning up the debugger server. Part of that includes switching the debugger server over to native DOM promises, so I'm keeping this bug assigned to me for the time being.

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Remove the deprecated-sync-thenables from protocol.js

Flagging Mike for review on this one since he was closely involved with debugging the memory leaks I was running into as a result of this change.

Thanks for your help Mike!

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8457942 [details] [diff] [review]
Remove the deprecated-sync-thenables from protocol.js

Review of attachment 8457942 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you remove dump("TEST").

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +192,5 @@
>     */
>    _onNewDocument: function() {
>      this._debuggee.getStyleSheets().then((styleSheets) => {
>        this._resetStyleSheetList(styleSheets);
> +    }).then(null, () => { dump("TEST"); });

Do you really want to dump "TEST" here?

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Remove the deprecated-sync-thenables from script.js

Assigning this one to Nick because most of it seems to be related to source mapping.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8458167 [details] [diff] [review]
Remove the deprecated-sync-thenables from script.js

Review of attachment 8458167 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/script.js
@@ +5453,5 @@
> +      // file, which will make a malformed URI error be thrown. Add the file
> +      // scheme prefix ourselves.
> +      url = "file://" + url;
> +      scheme = Services.io.extractScheme(url);
> +    }

Seems like this whole try/catch could be lifted out of the Promise constructor. I think that would be a little nicer because less of the code would be indented a lot and the Promise constructor would just wrap the stuff that is async so it is easier to determine what behavior the promise is encapsulating.

Comment 12

[Eddy Bruel [:ejpbruel]]

Attachment: Remove the deprecated-sync-thenables from main.js

One subtlety in this one. When resuming with an onStep resume limit, we want the response to arrive *before* the paused packet. Since the paused packet is send directly, this is a potential race condition, which manifested with the changes in this patch.

A workaround for this is to provide a way to put packets for events such as these on the response queue.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8458222 [details] [diff] [review]
Remove the deprecated-sync-thenables from main.js

Review of attachment 8458222 [details] [diff] [review]:
-----------------------------------------------------------------

Does this fix the pause/resume race exposed by the black boxing tests?

::: toolkit/devtools/server/main.js
@@ +553,5 @@
>     *         A promise object that is resolved once the connection is
>     *         established.
>     */
>    connectToChild: function(aConnection, aFrame, aOnDisconnect) {
> +    return new Promise(function (resolve, reject) {

This is pretty hard to read because this function is pretty big (I know that it already was), and this is compounded by how the Promise constructor is at the very top of the function, but resolve/reject aren't used until the very end of the function.

Can you move everything that isn't async out above the Promise constructor so its a little more clear what's going on? Thanks.

File a follow up bug to break this function up into smaller ones, too, please.

@@ +1172,5 @@
>      event.type = eventName;
>      this.send(event);
>    },
>  
> +  queueActorEvent: function (actorID, eventName, event) {

Doc comment please.

Extra bonus cool points if you add a doc comment for sendActorEvent and compare/contrast it with queueActorEvent, as well.

Comment 14

[Eddy Bruel [:ejpbruel]]

The initial try push for the first patch shows some oranges, but I've been unable to reproduce these after rebasing:
https://tbpl.mozilla.org/?tree=Try&rev=33ba1a61f04d

Pushing again to see if they persist.

Comment 15

[Eddy Bruel [:ejpbruel]]

Second try push for the first patch looks all green:
https://tbpl.mozilla.org/?tree=Try&rev=4e716740e939

Comment 16

[Eddy Bruel [:ejpbruel]]

After rebasing, it looks like the oranges are back again:
https://tbpl.mozilla.org/?tree=Try&rev=aad017ae7b53

Could this be timing related?

Comment 17

[Eddy Bruel [:ejpbruel]]

Did another try run, and this time it looks like the oranges persist :-(

https://tbpl.mozilla.org/?tree=Try&rev=15e4b4e314e4

Mike, could you take a look and see if you can reproduce the leaking nsIGlobalWindow issues locally this time?

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have looked at this for most of Friday, during the weekend and Monday morning.

Your green second try push should be disregarded as you forgot to apply the protocol.js patch.

On pretty much every occasion that we have test errors it seems that we are inside either an executeSoon or setTimeout. Because these are often used to work around race conditions and async promises would reveal them then this is almost expected.

The best workaround for that issue in my experience is to port the tests to use Task and use yield to wait for some event. At the very least we should be waiting for events instead of using timers.

As far as the leaked codemirror (and other) windows:
As far as I can see, they should have been destroyed... at least, the tool's destructor has called the destructor on the editor (which destroys the window). Maybe the front's destructor is called too soon?

What I don't understand is that when you sent me this patch the other day everything was green for me. Something must have changed during that time... I am just not sure what.

Comment 19

[Eddy Bruel [:ejpbruel]]

Since the last comment from Mike, we've removed the deprecated-sync-thenables from protocol.js in a separate bug , and opened bugs to remove them from the rest of the debugger code (bug 940537 for the frontend, and bug 943517 for the backend).

Removing the deprecated-sync-thenables will not be enough to start using native DOM promises though, because these do not play nicely with nested event loops in workers. We will have to stick with Promise.jsm promises until we find a solution for that problem.

I'm keeping this bug assigned to me for the time being, but I don't expect to be able to work on this any time soon.

Comment 20

[Eddy Bruel [:ejpbruel]]

This is not going to happen any time soon due to issues with worker debugging.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Lot of things changes in the devtools, we can close it now.