When map scopes is disabled we should not compute mappings

VERIFIED FIXED in Firefox 67

Status

defect
P1
normal
VERIFIED FIXED
a month ago
a month ago

People

(Reporter: jlast, Assigned: jlast)

Tracking

(Regression, {regression})

67 Branch
Firefox 68
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [qa-triaged])

Attachments

(2 attachments)

Assignee

Description

a month ago

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

Assignee

Updated

a month ago
Summary: Map Scopes should be toggled by the checkbox → When map scopes is disabled we should not compute mappings

Comment 2

a month 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

a month ago
bugherder
Status: NEW → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Assignee: nobody → jlaster
Assignee

Comment 4

a month 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:
Attachment #9059071 - Flags: approval-mozilla-beta?
Assignee

Updated

a month ago
Attachment #9057655 - Flags: approval-mozilla-beta?
Assignee

Updated

a month ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 5

a month ago

Logan, would you mind reviewing?

Assignee

Updated

a month ago
Flags: needinfo?(lsmyth)
Flags: needinfo?(lsmyth)
Assignee

Updated

a month ago
Priority: -- → P1
Assignee

Updated

a month ago
Keywords: regression
Version: unspecified → 67 Branch

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

Flags: needinfo?(jlaster)
Assignee

Comment 8

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

Flags: needinfo?(jlaster)
Attachment #9057655 - Flags: approval-mozilla-beta?
Status: REOPENED → RESOLVED
Last Resolved: a month agoa month 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+
Assignee

Comment 10

a month ago

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]

Comment 12

a month 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 ?

Flags: needinfo?(jlaster)
Assignee

Comment 13

a month ago

Hi Rares, what are you seeing on beta?

Flags: needinfo?(jlaster)

Comment 14

a month 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

a month ago

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

Comment 16

a month ago

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

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