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.

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

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.

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 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.

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

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

Backout by apavel@mozilla.com: 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 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

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

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

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

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.

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

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

s/debugger/debug

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

Made a new try run with repeat syntax: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cb3874e01a554f6ad4868642e1a9bda09fdf50 Try is green except for known intermittents.

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

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?

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?

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

> 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.

(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?

(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.

(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.

Hi :jdescottes, Thanks for your help. Hi Ryan, Can you help backout this bug and bug 1416358?

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 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 on attachment 8929073 [details] [diff] [review] 11-7-1.patch no longer needed

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+.