1415300 - Update Debugger Frontend (11-7)

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Comment 1

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53c8026be0128760750df3c1e9cad589e30637e5

Comment 2

[Jason Laster [:jlast]]

Attachment: patch-11-7-1.patch

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8926073 [details] [diff] [review]
patch-11-7-1.patch

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

Not regressions but:
- there is still the bug where clicking on a link opens the site inside the debugger. fixing this before merge day should be high priority
- the outline view does not work: clicking on the functions does not do anything
  -> getting a lot of "error: resource://devtools/shared/base-loader.js -> resource://devtools/client/debugger/new/debugger.js, line 63564: ReferenceError: L10N is not defined"

Regression:
- the watch variables seem to be subject to race conditions. Variables that should be defined show up as unavailable

Setting R+, up to you to decide if you want to address any of this before landing.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: debugger_watch_2.gif

Attaching GIF to illustrate race condition issue. Will file an issue on GitHub tomorrow. ni? myself as reminder.

Comment 5

[Jason Laster [:jlast]]

seeing an issue with browser_dbg-wasm-sourcemaps.js, which doesn't surprise me. 

Unable to find source: doc-wasm-sourcemaps

My bigger concern is 50% size increase of the debugger bundle, +50K, -25K. I'd like to look into why that occurred tomorrow. Unfortunately I believe it's 90% explained by bumping a package version... but not sure what yet.

Comment 6

[Julian Descottes [:jdescottes]]

Clearing ni? as Jason logged https://github.com/devtools-html/debugger.html/issues/4617 

Would be nice to isolate the test case outside of piskelapp.com though.

Comment 7

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b751c046ac20d2b6217a954ef26ae632e978aaa

Comment 8

[Jason Laster [:jlast]]

Attachment: patch-11-7-8.patch

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8926584 [details] [diff] [review]
patch-11-7-8.patch

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

Same issues as with the previous bundle (links in scope, outline view, watch expressions). As usual up to you.

Comment 10

[Julian Descottes [:jdescottes]]

Another issue I missed, the "waiting for next execution" icon does not indicate if it's on or not.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2acadbdad8
Update Debugger frontend (11-7). r=jdescottes

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a78c51b2bd4
Backed out changeset 9e2acadbdad8 for failing devtools/client/debugger/new/test/mochitest/browser_dbg-quick-open.js r=backout on a CLOSED TREE

Comment 13

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47579d08113ce47cc39db6c307d0f9abb3397a36

Comment 14

[Jason Laster [:jlast]]

Attachment: patch-11-7-3.patch

Comment 15

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c525ba0d0bfa5e88496d5d2f12daa4c5afcce8c

Comment 16

[Jason Laster [:jlast]]

Attachment: patch-11-7-13.patch

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8926989 [details] [diff] [review]
patch-11-7-13.patch

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

LGTM, fixes the link in scopes issue reported in Bug 1400355

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/186ee641ae41
Update Debugger frontend (11-7). r=jdescottes

Comment 19

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea6a1a72343 for very frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=143481924&repo=mozilla-inbound in devtools/client/debugger/new/test/mochitest/browser_dbg-wasm-sourcemaps.js

Comment 20

[Phil Ringnalda (:philor)]

Also very frequent, though it would be easy to miss, shutdown hang on various flavors of Linux debug, like https://treeherder.mozilla.org/logviewer.html#?job_id=143464902&repo=mozilla-inbound

Comment 21

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90fa83bd41b2208df4e703560498164b814ed388

Comment 22

[Jason Laster [:jlast]]

Attachment: patch-11-7-14.patch

Comment 23

[Julian Descottes [:jdescottes]]

Did a bunch of retriggers on https://treeherder.mozilla.org/#/jobs?repo=try&revision=90fa83bd41b2208df4e703560498164b814ed388 and started a new try (could not add new jobs with treeherder UI for some reason?) with Windows and OSX at https://treeherder.mozilla.org/#/jobs?repo=try&revision=264966bdda3a975d2fb480f6064aff941ac05b6a

This has already been backed out twice, let's take some extra testing time before re-pushing.

Comment 24

[Julian Descottes [:jdescottes]]

Attachment: patch-11-7-14a.patch

Still getting regular failures on try for the two tests mentioned by Philor, so adding skip-ifs.

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=533ed9abacdd8b5da7b2c37ac0a21baebf03b13b

Comment 25

[Julian Descottes [:jdescottes]]

try looks green for opt builds, added retriggers for debugger, but should be fine to land now.

Comment 26

[Julian Descottes [:jdescottes]]

s/debugger/debug

Comment 27

[Julian Descottes [:jdescottes]]

Attachment: patch-11-7-14b.patch

Disabling two other tests that seem intermittent in debug:
- browser_dbg-preview.js
- browser_dbg-quick-open.js

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=691c3fc8395e93727ec948fbd63b45ba254c804f

Comment 28

[Julian Descottes [:jdescottes]]

Made a new try run with repeat syntax:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cb3874e01a554f6ad4868642e1a9bda09fdf50

Try is green except for known intermittents.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2136c9de02
Update Debugger frontend (11-7). r=jdescottes

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b2136c9de02

Comment 31

[Gerry Chang [:gchang]]

Hi :jdescottes,
May I know why we land this huge patches just right before merge day? This cause too much risk for 58.0beta 3. Can we back it out?

Comment 32

[Julian Descottes [:jdescottes]]

Sorry about that, I forgot about the soft freeze. Also this patch contains important bug fixes for the debugger (e.g. Bug 1400355) as well as performance improvements, so we really wanted to have it in 58.

Regarding a backout, note that you would also need to backout Bug 1416358.

For reference backing this out means we are not shipping the following changes (~100 commits):
https://github.com/devtools-html/debugger.html/compare/d9f18b2cd0792de70289d4dcde5ed3e38be87cf1...be179268c9b89390c13bdc9c4cca6000f6f583e5

I believe we will want to uplift some of them. And due to the way the debugger is developed (on GitHub, synced to m-c by updating a big JS bundle), uplifting specific patches will be costly. Could we think about a way to uplift a bundle update (which would be a patch similar to this one) sometime in the early 58 beta cycle?

Comment 33

[Julian Descottes [:jdescottes]]

Jason, have a look at the previous comments. Have a look at my /compare link, what are the most important patches in the list?

Comment 34

[Sylvestre Ledru [:Sylvestre]]

> uplifting specific patches will be costly
I share your pain but I think, at the end, landing a big patch, the cost is higher for Mozilla has a whole.
It makes any regression detection much harder, it makes impossible for release management to backout the offending patches, etc.

We should land the important patches in individual bugs.

Clearly, we should discuss on how to work more efficiently together.

Comment 35

[Gerry Chang [:gchang]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #32)
> Sorry about that, I forgot about the soft freeze. Also this patch contains
> important bug fixes for the debugger (e.g. Bug 1400355) as well as
> performance improvements, so we really wanted to have it in 58.
> 
> Regarding a backout, note that you would also need to backout Bug 1416358.
> 
> For reference backing this out means we are not shipping the following
> changes (~100 commits):
> https://github.com/devtools-html/debugger.html/compare/
> d9f18b2cd0792de70289d4dcde5ed3e38be87cf1...
> be179268c9b89390c13bdc9c4cca6000f6f583e5
> 

Hi :jdescottes,
If that's the case, we may need to back out these 2 bugs. To me, this is not baked enough in nightly and we don't have aurora phase to stabilize anymore. 

Do you have any strong marketing/business reason that we have to ship this in 58? If not, can we let this ride the train on 59?

Comment 36

[Patrick Brosset <:pbro>]

(In reply to Gerry Chang [:gchang] from comment #35)
> Do you have any strong marketing/business reason that we have to ship this
> in 58? If not, can we let this ride the train on 59?
Jason and Harald should answer this question. I'll NI? Harald too.

Comment 37

[Julian Descottes [:jdescottes]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #34)
> > uplifting specific patches will be costly
> I share your pain but I think, at the end, landing a big patch, the cost is
> higher for Mozilla has a whole.
> It makes any regression detection much harder, it makes impossible for
> release management to backout the offending patches, etc.
> 
> We should land the important patches in individual bugs.
> 
> Clearly, we should discuss on how to work more efficiently together.

We are trying to automate our sync process between GitHub and m-c so that we can synchronize more regularly. Then it won't be as critical if a given update doesn't make it for a given release. Obviously, we're not there yet.

And I totally agree that we should try and land patches in individual bugs, it's just that our current setup makes it difficult. 

(In reply to Gerry Chang [:gchang] from comment #35)
> Hi :jdescottes,
> If that's the case, we may need to back out these 2 bugs. To me, this is not
> baked enough in nightly and we don't have aurora phase to stabilize anymore. 
> 
> Do you have any strong marketing/business reason that we have to ship this
> in 58? If not, can we let this ride the train on 59?

I don't know so I'll let Jason answer here. I don't think this bundle update as a whole needs to ship, but some features and fixes in it are most likely needed. IMO we could backout the patches now and then think about the best way to uplift the critical parts.

Comment 38

[Gerry Chang [:gchang]]

Hi :jdescottes,
Thanks for your help.

Hi Ryan,
Can you help backout this bug and bug 1416358?

Comment 39

[Julien Cristau [:jcristau] (back April 22)]

Backed out from m-b:
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/839ec6c77d51e310832412b63324413b3b74a2af
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/cd8409437a4e9a4d34eb1385225c6e33a569642c

Comment 41

[Jason Laster [:jlast]]

Attachment: 11-7-1.patch

Comment 42

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ef52ccdacd568c7051a649fbb65f620e2b4b2a7

Comment 43

[Jason Laster [:jlast]]

Comment on attachment 8927329 [details] [diff] [review]
patch-11-7-14b.patch

[Feature/Bug causing the regression]:

1411727

[User impact if declined]:

  - WASM debugging is currently missing scopes and variables

 - React + Framework applications currently see 10 sec load times and several second pause times. 

[Is this code covered by automated tests?]:

- yes

[Has the fix been verified in Nightly?]:

- yes

[Needs manual test from QE? If yes, steps to reproduce]: 

- no

[List of other uplifts needed for the feature/fix]:

1416358 is needed after this lands.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

It only affects firefox when the debugger is open.

[String changes made/needed]:

Comment 44

[Jason Laster [:jlast]]

Comment on attachment 8929073 [details] [diff] [review]
11-7-1.patch

no longer needed

Comment 45

[Gerry Chang [:gchang]]

Comment on attachment 8927329 [details] [diff] [review]
patch-11-7-14b.patch

Afer talking with :jlast, we think this is very important for performance enhancement and React devs. So, we decide the let it back to 58 beta ASAP so that it can be baked long enough in beta cycle. Beta58+.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fb1d0acce911