Closed Bug 943517 Opened 11 years ago Closed 9 years ago

Remove the deprecated-sync-thenables from the debugger server

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox41 fixed, firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox41 --- fixed
firefox43 --- fixed

People

(Reporter: bbenvie, Assigned: ejpbruel)

References

(Blocks 1 open bug)

Details

(Whiteboard: p=2)

Attachments

(2 files, 6 obsolete files)

      No description provided.
Depends on: 943512
Attached patch promise-debugger-server.patch (obsolete) — Splinter Review
This patch changes all uses of Promises in toolkit/debugger to use Promise.jsm.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
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.
The xpcom test failures are most likely due to the failures in bug 943512. But bc failures, though, remain ominous.
Attached patch Updated patch (obsolete) — Splinter Review
Just updated this patch, a tryserver build is running here:

https://tbpl.mozilla.org/?tree=Try&rev=1070cd81e7f9
Attachment #8338833 - Attachment is obsolete: true
Blocks: 982597
Whiteboard: p=2
Attached patch Deferred change to dbg-client.js (obsolete) — Splinter Review
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
Attachment #8389742 - Attachment is obsolete: true
(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 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.
(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.
Attached patch Promises in loader globals (obsolete) — Splinter Review
(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.
(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
I'm curious if you get this error locally: reference to undefined property x.SpecialPowers_wrappedObject
(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.
(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.
CCing Mihai for ideas.
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.
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.
Attached patch Rebased patch (obsolete) — Splinter Review
(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.
Attachment #8389826 - Attachment is obsolete: true
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?
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.
No longer blocks: 982597
Depends on: 982597
Attached patch Folded patch (obsolete) — Splinter Review
I folded the patches into one, including the conversions and test changes from bug 982597.
Attachment #8391095 - Attachment is obsolete: true
Attachment #8391369 - Attachment is obsolete: true
The folded patch shows a completely different set of failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=36262143&full=1&branch=try
Attached patch Work in progressSplinter Review
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
Attachment #8392116 - Attachment is obsolete: true
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.
Flags: needinfo?(mratcliffe)
Woops, was just trying to find out your preferred :name.
Flags: needinfo?(mratcliffe)
(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.
Depends on: 986481
Depends on: 988327
Depends on: 988330
Depends on: 988335
Blocks: 988997
Assignee: bbenvie → nobody
Status: ASSIGNED → NEW
Blocks: dbg-server
Assignee: nobody → ejpbruel
Depends on: 1096490
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
Attachment #8552364 - Flags: review?(jlong)
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!
(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 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!
Attachment #8552364 - Flags: review?(jlong) → review+
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
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
Summary: Convert to Promise.jsm in the debugger server → Remove the deprecated-sync-thenables from the debugger server
https://hg.mozilla.org/mozilla-central/rev/a83fbd4c96b8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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
Status: RESOLVED → REOPENED
Flags: needinfo?(ejpbruel)
Resolution: FIXED → ---
Depends on: 1164240
I will take another shot at this when I get back from vacation.
Flags: needinfo?(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
https://hg.mozilla.org/mozilla-central/rev/420dbb1751fc
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 41 → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.