Closed Bug 1415300 Opened 2 years ago Closed 2 years ago

Update Debugger Frontend (11-7)

Categories

(DevTools :: Debugger, defect, P3)

57 Branch
defect

Tracking

(firefox58 fixed, firefox59 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(2 files, 7 obsolete files)

No description provided.
Attached patch patch-11-7-1.patch (obsolete) — Splinter Review
Attachment #8926073 - Flags: review?(jdescottes)
Assignee: nobody → jlaster
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P3
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.
Attachment #8926073 - Flags: review?(jdescottes) → review+
Attached image debugger_watch_2.gif (obsolete) —
Attaching GIF to illustrate race condition issue. Will file an issue on GitHub tomorrow. ni? myself as reminder.
Flags: needinfo?(jdescottes)
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.
Flags: needinfo?(jdescottes)
Attachment #8926073 - Attachment is obsolete: true
Attachment #8926094 - Attachment is obsolete: true
Attached patch patch-11-7-8.patch (obsolete) — Splinter Review
Attachment #8926584 - Flags: review?(jdescottes)
Keywords: checkin-needed
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.
Attachment #8926584 - Flags: review?(jdescottes) → review+
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
Keywords: checkin-needed
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
Attachment #8926584 - Attachment is obsolete: true
Attached patch patch-11-7-3.patch (obsolete) — Splinter Review
Attachment #8926982 - Flags: review?(jdescottes)
Attachment #8926982 - Attachment is obsolete: true
Attachment #8926982 - Flags: review?(jdescottes)
Attached patch patch-11-7-13.patch (obsolete) — Splinter Review
Attachment #8926989 - Flags: review?(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
Attachment #8926989 - Flags: review?(jdescottes) → review+
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
Attachment #8926989 - Attachment is obsolete: true
Attached patch patch-11-7-14.patch (obsolete) — Splinter Review
Attachment #8927168 - Flags: review?(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.
Attached patch patch-11-7-14a.patch (obsolete) — Splinter Review
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
Attachment #8927168 - Attachment is obsolete: true
Attachment #8927168 - Flags: review?(jdescottes)
Attachment #8927271 - Flags: review+
try looks green for opt builds, added retriggers for debugger, but should be fine to land now.
Keywords: checkin-needed
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
Attachment #8927329 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/0b2136c9de02
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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?
Flags: needinfo?(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?
Flags: needinfo?(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?
Flags: needinfo?(jlaster)
> 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.
Flags: needinfo?(hkirschner)
(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?
Flags: needinfo?(ryanvm)
Duplicate of this bug: 1400355
Attached patch 11-7-1.patchSplinter Review
Attachment #8927271 - Attachment is obsolete: true
Attachment #8927329 - Attachment is obsolete: true
Flags: needinfo?(jlaster)
Attachment #8929073 - Flags: review?(jdescottes)
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]:
Attachment #8927329 - Flags: approval-mozilla-beta?
Comment on attachment 8929073 [details] [diff] [review]
11-7-1.patch

no longer needed
Attachment #8929073 - Flags: review?(jdescottes) → review-
Attachment #8927329 - Attachment is obsolete: false
Flags: needinfo?(hkirschner)
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+.
Attachment #8927329 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.