When map scopes is disabled we should not compute mappings
Categories
(DevTools :: Debugger, defect, P1)
Tracking
(firefox-esr60 unaffected, firefox66 unaffected, firefox67 verified, firefox68 verified)
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox66 | --- | unaffected |
firefox67 | --- | verified |
firefox68 | --- | verified |
People
(Reporter: jlast, Assigned: jlast)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [qa-triaged])
Attachments
(2 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
1.59 KB,
patch
|
loganfsmyth
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
When we landed the map scopes checkbox, we did not ensure that the mappings would be disabled.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Pushed by jlaster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c35f582b930c When map scopes is disabled we should not compute mappings. r=loganfsmyth
Comment 3•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: Bug 1543731
- User impact if declined: It will be impossible for users to disable "Map Scopes", which can have a significant performance cost.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This fixes a feature flag, which was supposed to be availab.e
- String changes made/needed:
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
•
|
||
Logan, would you mind reviewing?
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 6•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 7•5 years ago
|
||
How was this verified?
Are you requesting uplift on both patches attached here?
Assignee | ||
Comment 8•5 years ago
|
||
Hey Liz, We should land the patch from 5 days ago, which is a smaller version of the patch from 11 days ago, which landed on nightly.
We verified this patch on beta manually and added a mochitest for the initial patch, which is on nightly.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 9•5 years ago
|
||
Comment on attachment 9059071 [details] [diff] [review] uplift-ms-1.patch Fixes a perf regression in 67. Approved for 67.0b13. Let's get QA to verify this after uplift as well since we aren't uplifting the tests.
Assignee | ||
Comment 10•5 years ago
•
|
||
Sounds good.
Here's a simple STR:
- go to https://firefox-dev.tools/debugger-examples/examples/increment/
- open the debugger and add a breakpoint in increment.js line 4
- click the increment button in the app.
ER:
- the debugger should pause
- the scopes pane has a "map scopes" checkbox in the header that is unchecked
- the scopes pane top section has the scope "Block" and variables "<this>", "arguments", and "val"
Comment 11•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Hi, I Tested this issue in our latest Nightly build and the issue no longer occurs there. I will mark it as Verified.
But in Firefox Beta 67.0b13 with this changeset Mercurial > releases > mozilla-beta / changeset / ceb8833f34c3 the issue still occurs, After looking through the Pushlog we noticed that the fix should be in this beta build but we can still reproduce the issue there.
This is the build Id - 20190422163745 of the Build we downloaded from : https://tools.taskcluster.net/index/gecko.v2.mozilla-beta.nightly.latest.firefox
Is there a different Beta build we should test this on ?
Comment 14•5 years ago
|
||
Hi Jason, after Further Testing I noticed a few Differences in these builds and now I'm not really sure what the correct behavior is.
Release only shows :
Increment - val = 0
Beta shows :
Map Checkbox
Block > this, arguments and val
Nightly shows :
Map Checkbox
Block > this, arguments and val
But Nightly also shows the Message "Paused on Breakpoint" - Beta does not, I was confused when I didn't see that message and I thought that maybe the fix is not yet in Beta, But if the message shoudnt be displayed in Beta we can mark this issue as Verified as Fixed since everything else is there, sorry for the confusion.
Assignee | ||
Comment 15•5 years ago
|
||
Thanks Rares. I think beta is fine. We do not need the "Paused on Breakpoint" message in the top right :)
Comment 16•5 years ago
|
||
Ok ok thanks Jason, I'll mark this accordingly.
Updated•2 years ago
|
Description
•