943517 - Remove the deprecated-sync-thenables from the debugger server

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Comment 1

[Brandon Benvie [:benvie]]

Attachment: promise-debugger-server.patch

This patch changes all uses of Promises in toolkit/debugger to use Promise.jsm.

Comment 2

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=7fd5c0e55f6e

Comment 3

[Brandon Benvie [:benvie]]

I was afraid of this. Changing the server side implementation of Promise breaks client side tests. This may be relieved by changing the client side usage first.

Comment 4

[Brandon Benvie [:benvie]]

The xpcom test failures are most likely due to the failures in bug 943512. But bc failures, though, remain ominous.

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

Just updated this patch, a tryserver build is running here:

https://tbpl.mozilla.org/?tree=Try&rev=1070cd81e7f9

Comment 6

[:Paolo Amadini]

Attachment: Deferred change to dbg-client.js

The change in dbg-client.js was causing some browser-chrome failures because it caused the user interface to behave incorrectly. I think the change can be deferred to bug 982597. I just started a new tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=7e09b9a62647

Comment 7

[Brandon Benvie [:benvie]]

(In reply to :Paolo Amadini from comment #5)
> Created attachment 8389742 [details] [diff] [review]
> Updated patch
> 
> Just updated this patch, a tryserver build is running here:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=1070cd81e7f9

There seems to be a lot less failures than when I tested this initially, good sign.

Comment 8

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

Comment on attachment 8389826 [details] [diff] [review]
Deferred change to dbg-client.js

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

::: toolkit/devtools/server/main.js
@@ +56,5 @@
>  }
>  
> +loadSubScript.call(this, "resource://gre/modules/Promise-backend.js");
> +let promise = Promise;
> +let {defer, resolve, reject, all} = Promise;

We have multiple ongoing refactorings of the debugger server at the moment and we should tread carefully.

Unfortunately this is undoing Eddy's ongoing work to make the debugger server require()-friendly (bug 859372), which is a prerequisite for debugging workers.

I can see 2 ways to reconcile both patches:

1. Turn Promise-backend.js into a require-friendly module (like SDK promises).
2. Add the Promise object to the loader-provided globals.

What I like about 2 is that reverting to native promises in the server would be a 2-line patch.

Comment 9

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #8)
> We have multiple ongoing refactorings of the debugger server at the moment
> and we should tread carefully.
> 
> Unfortunately this is undoing Eddy's ongoing work to make the debugger
> server require()-friendly (bug 859372), which is a prerequisite for
> debugging workers.
> 
> I can see 2 ways to reconcile both patches:
> 
> 1. Turn Promise-backend.js into a require-friendly module (like SDK
> promises).
> 2. Add the Promise object to the loader-provided globals.
> 
> What I like about 2 is that reverting to native promises in the server would
> be a 2-line patch.

This sounds reasonable, though I don't have a clear picture of how the module system works, and I don't see how loadSubScript would break require().

Since the two options don't look like complex to implement for someone knowledgeable, do you think one of you can provide a tentative patch for it? I can then run it through tests as I'm doing with the current patch.

Comment 10

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

Attachment: Promises in loader globals

(In reply to :Paolo Amadini from comment #9)
> This sounds reasonable, though I don't have a clear picture of how the
> module system works, and I don't see how loadSubScript would break require().

The problem is loadSubscript is not available in workers, so we have to get rid of it in the debugger code.

> Since the two options don't look like complex to implement for someone
> knowledgeable, do you think one of you can provide a tentative patch for it?
> I can then run it through tests as I'm doing with the current patch.

Sure, this is what I had in mind. Passes all tests locally.

Comment 11

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #10)
> Sure, this is what I had in mind. Passes all tests locally.

Looks good, I've imported it in my local queue and I'll fold it with my patch.

In the meantime I'm working on fixing the browser-chrome failures, a particularly tricky one is devtools/shared/test/browser_toolbar_webconsole_errors_count.js. At some point, it is supposed to call a function that creates an error and a warning:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/test/browser_toolbar_webconsole_errors_count.html?force=1#27

In my local run, I see both the warning and the error:

ReferenceError: reference to undefined property testObj.fooBug788445
TypeError: window.foobarBug762996click is not a function

On the tryserver, the error is never generated, as if the code stopped or something went wrong with a console reporter or listener (example log: https://tbpl.mozilla.org/php/getParsedLog.php?id=36071273#error0).

I'm not sure where to look. For now, to investigate the other failures, I've started a new tryserver build with the test disabled:

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

Comment 12

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

I'm curious if you get this error locally: reference to undefined property x.SpecialPowers_wrappedObject

Comment 13

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #12)
> I'm curious if you get this error locally: reference to undefined property
> x.SpecialPowers_wrappedObject

Yes, I see it as a JavaScript strict warning.

Comment 14

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #11)
> started a new tryserver build with the test disabled:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=d5de0e4a4202

It seems that there are other failures with the same underlying cause, apparently that console errors aren't reported (see for example "failed to match rule: " in <https://tbpl.mozilla.org/php/getParsedLog.php?id=36144560#error0>). I'll try to find the console code and understand why it can be affected by the change in the debugger server.

Comment 15

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

CCing Mihai for ideas.

Comment 16

[:Paolo Amadini]

I took a look at the console error message handling starting from the listeners on the server side, and there doesn't seem to be anything obvious that uses Promises, except maybe the onPacket callback on the client side of the protocol, that however still uses the old Promise implementation.

I wonder if the problem is in the protocol or in the console reporting itself. I'll do a tryserver run with "devtools.debugger.log" enabled when the try tree reopens.

Comment 17

[Mihai Sucan [:msucan]]

The dom console API tests fail as well, which looks to be a serious issue. However, the DOM browser console API tests do not use the debugger server at all.

Do you have other patches in the queue? Or do you have local pull with lots of oranges, maybe from mozilla-inbound? fx-team should be 'cleaner'.

I recommend you enable debugger logs. We need to see which packets are sent/received.

I tried applying these patches locally, on top of the latest fx-team. They seem to need a rebase. All tests pass locally? If yes, I'm surprised to see so many failures on tbpl.

Comment 18

[:Paolo Amadini]

Attachment: Rebased patch

(In reply to Mihai Sucan [:msucan] from comment #17)
> Do you have other patches in the queue? Or do you have local pull with lots
> of oranges, maybe from mozilla-inbound? fx-team should be 'cleaner'.

I have only the patches on this bug, based on a recent mozilla-central revision, but the previous patch needed rebasing.

> I recommend you enable debugger logs. We need to see which packets are
> sent/received.

Yes, the attached patch has those enabled, we can push as soon as try reopens.

> I tried applying these patches locally, on top of the latest fx-team. They
> seem to need a rebase. All tests pass locally? If yes, I'm surprised to see
> so many failures on tbpl.

I didn't run all the browser-chrome test suite, but I ran all the tests in the folder where the console file resides. Even if I ran the entire suite, that I can only do when I'm not using the computer, and it failed, this will still not give me an idea about the cause of the problem.

Comment 19

[:Paolo Amadini]

The tree just reopened: https://tbpl.mozilla.org/?tree=Try&rev=25c84a64b4c6

Comment 20

[:Paolo Amadini]

So, I've added at the beginning of the test the line:

  Services.prefs.setBoolPref("devtools.debugger.log", true);

But I don't see packet information in the logs:

  https://tbpl.mozilla.org/php/getParsedLog.php?id=36161066&tree=Try&full=1#error0

Is there anything else that needs to be done to see the required information there?

Comment 21

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

You should set the pref in head.js, as the debugger server only reads the preference when loading. See how it's done in browser/devtools/debugger/test/head.js.

Comment 22

[:Paolo Amadini]

OK, new attempt here:

https://tbpl.mozilla.org/?tree=Try&rev=8c4e90e36a43

Comment 23

[:Paolo Amadini]

Attachment: Folded patch

I folded the patches into one, including the conversions and test changes from bug 982597.

Comment 24

[:Paolo Amadini]

The folded patch shows a completely different set of failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=36262143&full=1&branch=try

Comment 25

[:Paolo Amadini]

Attachment: Work in progress

This fixes other failures in the coalesced patch, it includes lots of executeSoon workarounds to event timing issues. Do you know who could do a review here?

Tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=189ffca84542

Comment 26

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

This might be easier if split into a different patch for each tool, or at least split out the patches which are simple import changes from the ones that involve adding |waitForTick()| or |executeSoon()|. Hard to tell what's going on in splinter and seems like a large number of changes are just import changes. Simple import changes are an easy r+ but adding |waitForTick()| or |executeSoon()| is something that definitely deserves to be looked over by the respective code owner(s).

For the debugger mochitests, :vporof would probably be the best reviewer.

For the debugger server and tracer stuff, :past or I can review.

For the inspector stuff, :bgrins, :pbrosset, or :mratcliffe would be good.

For the stylesheets stuff, :harth.

For the protocol.js stuff, :dcamp.

Comment 27

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

Woops, was just trying to find out your preferred :name.

Comment 28

[:Paolo Amadini]

(In reply to Nick Fitzgerald [:fitzgen] from comment #26)
> For the debugger mochitests, :vporof would probably be the best reviewer.
> For the debugger server and tracer stuff, :past or I can review.
> For the inspector stuff, :bgrins, :pbrosset, or :mratcliffe would be good.
> For the stylesheets stuff, :harth.
> For the protocol.js stuff, :dcamp.

This is great information, thanks!

I think I'll try to land as much single-line module conversions and front-end conversions as possible before looking into fixing the remaining failing tests for the debugger server.

Comment 29

[Eddy Bruel [:ejpbruel]]

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

Here's a patch that removes the deprecated-sync-thenables from script.js. Now that the deprecated-sync-thenables are removed from protocol.js it seems to just work.

Try run for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=025ba8cbdff3

Comment 30

[James Long (:jlongster)]

Eddy, so your work on removing it from protocol.js landed (haven't had time to track down those bugs)? This looks a little scary, but if you've landed a bunch of work prior to this and haven't seen any bugs with this change, then awesome!

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #30)
> Eddy, so your work on removing it from protocol.js landed (haven't had time
> to track down those bugs)? This looks a little scary, but if you've landed a
> bunch of work prior to this and haven't seen any bugs with this change, then
> awesome!

Yes, bug 1096490.

Comment 32

[James Long (:jlongster)]

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

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

Can't argue with this :) Awesome work!

Comment 33

[Eddy Bruel [:ejpbruel]]

Try push for this is somewhat stale, so here's another one on top of a recent pull of fx-team just in case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3f383b03d61

Comment 34

[Eddy Bruel [:ejpbruel]]

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-dt failures.
https://hg.mozilla.org/integration/fx-team/rev/aa618f42c5a6

https://treeherder.mozilla.org/logviewer.html#?job_id=1968625&repo=fx-team

Comment 36

[Eddy Bruel [:ejpbruel]]

Going to have another shot at this.

James recently removed ThreadSources from script.js, which contained most of the promise code in script.js. Here's a new try push to figure out if anything still breaks when we remove the deprecated-sync-thenables:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee7ef58bed3

Comment 37

[Pulsebot]

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

Comment 38

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

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

Comment 39

[Carsten Book [:Tomcat]]

sorry had to back this out again for frequent failures causing https://bugzilla.mozilla.org/show_bug.cgi?id=1164240 - thats the same failure as in comment #36 - seems this failure is OS X only (at least most on OSX) so maybe we would need next time a try run that covers this platform too

backed out on m-c and so later with the merges down also from fx-team and co

Comment 40

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/d1186138d219

Comment 41

[Eddy Bruel [:ejpbruel]]

I will take another shot at this when I get back from vacation.

Comment 42

[Eddy Bruel [:ejpbruel]]

And we're back! Here's a new try push that includes OSX. Let's see if there are still failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f32d90c2820

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/420dbb1751fc

Comment 44

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

https://hg.mozilla.org/mozilla-central/rev/420dbb1751fc