Closed Bug 1543731 Opened 5 years ago Closed 5 years ago

When map scopes is disabled we should not compute mappings

Categories

(DevTools :: Debugger, defect, P1)

67 Branch
defect

Tracking

(firefox-esr60 unaffected, firefox66 unaffected, firefox67 verified, firefox68 verified)

VERIFIED FIXED
Firefox 68
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)

When we landed the map scopes checkbox, we did not ensure that the mappings would be disabled.

Summary: Map Scopes should be toggled by the checkbox → When map scopes is disabled we should not compute mappings
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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Assignee: nobody → jlaster

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:
Attachment #9059071 - Flags: approval-mozilla-beta?
Attachment #9057655 - Flags: approval-mozilla-beta?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Logan, would you mind reviewing?

Flags: needinfo?(lsmyth)
Flags: needinfo?(lsmyth)
Priority: -- → P1
Keywords: regression
Version: unspecified → 67 Branch

How was this verified?
Are you requesting uplift on both patches attached here?

Flags: needinfo?(jlaster)

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.

Flags: needinfo?(jlaster)
Attachment #9057655 - Flags: approval-mozilla-beta?
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
No longer blocks: 1532162
Flags: qe-verify+
Regressed by: 1532162
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.
Attachment #9059071 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Sounds good.

Here's a simple STR:

  1. go to https://firefox-dev.tools/debugger-examples/examples/increment/
  2. open the debugger and add a breakpoint in increment.js line 4
  3. click the increment button in the app.

ER:

  1. the debugger should pause
  2. the scopes pane has a "map scopes" checkbox in the header that is unchecked
  3. the scopes pane top section has the scope "Block" and variables "<this>", "arguments", and "val"
Whiteboard: [qa-triaged]

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 ?

Flags: needinfo?(jlaster)

Hi Rares, what are you seeing on beta?

Flags: needinfo?(jlaster)

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.

Thanks Rares. I think beta is fine. We do not need the "Paused on Breakpoint" message in the top right :)

Ok ok thanks Jason, I'll mark this accordingly.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: