940537 - Remove the deprecated-sync-thenables from the debugger frontend

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P2)

Comment 1

[Brandon Benvie [:benvie]]

Attachment: promise-debugger.patch

My initial work on this.

Comment 2

[Eddy Bruel [:ejpbruel]]

I will take another look at this once I get back from vacation.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Remove the deprecated-sync-thenables from the debugger client

Removing the deprecated-sync-thenables from the debugger client causes one of the conditional breakpoint tests to fail.

On closer inspection, this test simulates a button click on a breakpoint, and then checks whether the UI has been properly updated. However, it does not wait for the button click to be fully processed before checking the UI, so this test is inherently racy.

This was not a problem when the frontend still used synchronous promises, since the button click would be entirely processed within a single tick of the event loop. With asynchronous promises, however, the button click is processed over several ticks of the event loop. As a result, the UI is not fully updated yet by the time the tests tries to check it.

The solution I came up with is to add an event that is emitted from the devtools window whenever a breakpoint click has been fully processed. Note that this does not completely solve the problem, as conditional breakpoints can asynchronously open a popup. This operation does not return a promise, so is not guaranteed to be completed when the BREAKPOINT_CLICKED event fires.

Though not perfect, this solution is at least an improvement on the previous situation, and makes the test pass locally.

Comment 4

[Eddy Bruel [:ejpbruel]]

Try push for the above patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b15c55e76d

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Remove the deprecated-sync-thenables from the UI

Removing the deprecated-sync-thenables from the UI (specifically, the debugger-controller) causes most of the pretty print tests to fail.

The problem here is that the code that handles pretty printing in the debugger-controller tries to define a property on a promise. That was possible with the deprecated-sync-thenables, but Promise.jsm promises are not extensible.

The solution here is to store the property in question separate from the promise itself.

Comment 6

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8661199 [details] [diff] [review]
Remove the deprecated-sync-thenables from the debugger client

Victor said he'd review these.

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 8661199 [details] [diff] [review]
Remove the deprecated-sync-thenables from the debugger client

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

::: browser/devtools/debugger/views/sources-view.js
@@ +1052,5 @@
>      }
>  
> +    promise.then(() => {
> +      window.emit(EVENTS.BREAKPOINT_CLICKED);
> +    });

This function begs to be rewritten in task.js

Comment 8

[Victor Porof [:vporof][:vp]]

Comment on attachment 8661202 [details] [diff] [review]
Remove the deprecated-sync-thenables from the UI

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1402,5 @@
> +    if (cached && cached.pretty === wantPretty) {
> +      return cached.promise;
> +    }
> +
> +    let textPromise = new Promise((resolve, reject) => {

Please oh please don't mix Promise with promise.

Also please task I can't read this.

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Remove the deprecated-sync-thenables from the UI

New patch with comments addressed.

I understand your preference for Task.js. However, I'm not going to rewrite the patch at this point, since that would just be busywork. The point of this bug is to remove the deprecated-sync-thenables, so I'm aiming for the minimal change that will accomplish that.

That said, I have more patches coming up. I'll aim to use Task.js for those if it helps you review these.

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8661236 [details] [diff] [review]
Remove the deprecated-sync-thenables from the UI

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1402,5 @@
> +    if (cached && cached.pretty === wantPretty) {
> +      return cached.promise;
> +    }
> +
> +    let textPromise = new Promise((resolve, reject) => {

DOM Promises or imported promises? My previous comment stands...

Comment 11

[James Long (:jlongster)]

Eddy, this is going to directly conflict with my refactoring (which is done but we need to polish and get tests passing). I thought you know I was refactoring this part and was going to wait. 

My refactoring fixes all of this too, so I don't think you need to spend time on this. I think we can land it in a couple weeks, and that's only because I'm going to a conf next week. I'll talk to you tomorrow about this and see how we can collaborate to land my stuff.

Comment 12

[Eddy Bruel [:ejpbruel]]

I talked to James on irc, and we agreed that I will hold off from changing the promise implementation on the frontend until James can land his big refactor.

That said, switching promise implementations uncovers a lot of race conditions in our tests (I'm currently getting xpcshell test failures on try for the first patch). We need to fix those first anyway, and should be able to do so without actually changing the promise implementations. 

I'm going to hold off on this bug for now, and will open a separate bug for fixing said race conditions in individual tests. Note that we might not have to fix all tests, since some of them will be obsoleted by James' refactor anyway.

Comment 13

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

Discussed with :ejpbruel, I am going to make an attempt here now that :jlongster's refactor has landed.

Comment 14

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

Attachment: MozReview Request: Bug 940537 - Convert to Promise.jsm in debugger UI. r=jlongster

Review commit: https://reviewboard.mozilla.org/r/28663/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28663/

Comment 15

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54f36d12bf3a

Comment 16

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

Comment on attachment 8700245 [details]
MozReview Request: Bug 940537 - Convert to Promise.jsm in debugger UI. r=jlongster

More test failures to resolve first.

Comment 17

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

Well, I got pretty close:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=799fe691c719

The main failures are in browser_dbg_worker-window.js.  So far, I have not found an obvious fix.  :ejpbruel, any advice?

Comment 18

[James Long (:jlongster)]

I vaguely remember discussing with you my fix for worker-window.js in bug 1132501. Did you try applying that? It's basically a one-liner. If not I can show you the fix again tomorrow.

That patch should land this week, just waiting on a review. If it seems to stall I plan on spinning out a few smaller bugs to land because I found a few things like this to fix.

Comment 19

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

(In reply to James Long (:jlongster) from comment #18)
> I vaguely remember discussing with you my fix for worker-window.js in bug
> 1132501. Did you try applying that? It's basically a one-liner. If not I can
> show you the fix again tomorrow.

Yes, I did include it, here's the variation I tried:

https://hg.mozilla.org/try/rev/0633ab493243

It did not appear to fix the errors I am seeing.

Comment 20

[Eddy Bruel [:ejpbruel]]

I don't have any advice how to fix the error at this point, since I'm also struggling with test failures for the same test. I talked to jryans and we agreed to spend some time tomorrow looking into this bug together.

Comment 21

[Tom Tromey :tromey]

If this is just for the front end, then I would suggest closing it, because the new front end doesn't use this module.
On the other hand, I notice that devtools/server still uses deprecated-sync-thenables :( so maybe this bug should
be repurposed.

Comment 22

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

(In reply to Tom Tromey :tromey from comment #21)
> If this is just for the front end, then I would suggest closing it, because
> the new front end doesn't use this module.
> On the other hand, I notice that devtools/server still uses
> deprecated-sync-thenables :( so maybe this bug should
> be repurposed.

There are separate bugs for this issue in the RDP server (bug 1233890) and client (bug 1233891).

I think I'll hang onto this for now, since the old debugger code is still in tree and still used for Browser Toolbox.  If we happen to delete the old debugger without this fixed, then of course it would make sense to close this.  We'll see who wins... :)

Comment 23

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

(I have looked at this again recently, but as usual, I get distracted by other things.)

Comment 24

[Jason Laster [:jlast]]

Closing this bug because it is specific to the old debugger ui.