Breakpoints are sometimes appearing and then disappearing from list on toolbox opening

RESOLVED FIXED in Firefox 67

Status

defect
--
critical
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: Harald, Assigned: bhackett)

Tracking

({regression})

67 Branch
Firefox 68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67+ fixed, firefox68 fixed)

Details

Attachments

(3 attachments)

Posted image ScreenFlow.gif

DevEdition 67b3.

Reproduced on send.firefox.com, in both docked and windowed toolbox mode.

STR:

  • Set breakpoint
  • Close and open toolbox again

AR: Breakpoint is shown correctly in the source, but the list entry appears for a blink of an eye and then disappears.

Video shows first re-open works, but second time fails to show BPs.

Definitely a bug. This looks like it was was introduced by https://bugzilla.mozilla.org/show_bug.cgi?id=1534786.

Brian, could you look into this and see if there are any easy fixes?

The contributing factors:

At https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/devtools/client/debugger/new/src/selectors/breakpointSources.js#36 we require a breakpoint to have text or a condition or be disabled to show it in the list.

At https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js#116 we create a breakpoint with no text.

At https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js#39 we bail out early if the breakpoint already exists at a given position, storing the text-less breakpoint into the store.

At https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/devtools/client/debugger/new/src/actions/sources/newSources.js#200 we call addBreakpoint with a pending, possibly-existing breakpoint.

All of these come together to mean that when you initially load the page, the breakpoint will show up in the sidebar list. Then once the source that it applies to finishes loading, the logic in newSources runs and potentially deletes the text from the breakpoint, causing it to vanish from the sidebar.

Flags: needinfo?(bhackett1024)

The most serious problem here is that the early return in addBreakpointPromise is guaranteed to leave the breakpoint in a deformed state: addBreakpoint() never supplies text/originalText for the breakpoint being added in the initial promise, so if the early return is taken the breakpoint will not render correctly. I don't think there's a reason for this early return anymore, so the patch I'm about to post removes it.

This doesn't fix everything with the page, however. There can still be a brief flicker where the breakpoint disappears in the window between the start of the addBreakpoint() and when its promise completes.

There is also a second problem which doesn't manifest here but could if the page were being modified. If the original location maps to a different generated location in the pending breakpoints vs. the actual locations on loading, we could get two breakpoints installed from the single pending breakpoint, at both its original and generated location. We expect the generated source to reach checkPendingBreakpoints first but there is no guarantee of that with all the asynchronous computation here.

I'd like to work on fixing both of these next week, but the breakpoint action/reducer code is way more complicated than necessary, and needs simplification as well.

Flags: needinfo?(bhackett1024)

That sounds good.

the breakpoint action/reducer code is way more complicated than necessary, and needs simplification as well.

We may want to consider breaking this down into strictly necessary and cleanup tasks. This issues seems pretty frustrating for users, so ideally whatever we land for it should be uplifted I'd think. That means we should do our best to keep the diff to a minimum. Perhaps we can do the branch-removal for now to make sure the breakpoint stays in the list and file secondary issues for the other tasks and only focus on uplifting this one?

Assignee: nobody → bhackett1024
Status: NEW → ASSIGNED

Yeah, this is a targeted patch which can be uplifted, but refactoring to make this code work more smoothly is ongoing maintenance which might break other things and shouldn't be uplifted.

Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f058ab7b989d
Don't leave deformed breakpoints in reducer state, r=loganfsmyth.
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
https://hg.mozilla.org/projects/ash/rev/f058ab7b989d1c413a49e6e9e087d30c4390a24d
Bug 1537589 - Don't leave deformed breakpoints in reducer state, r=loganfsmyth.

[Tracking Requested - why for this release]: Introduced by larger refactoring, this regression can cause breakpoints disappearing in frustrating ways.

(In reply to :Harald Kirschner :digitarald from comment #9)

[Tracking Requested - why for this release]: Introduced by larger refactoring, this regression can cause breakpoints disappearing in frustrating ways.

Brian, is that a patch we could safely uplift to beta or can it ride the 68 train?

Flags: needinfo?(bhackett1024)

Pascal, I'll look into that today.

Yeah, per comment 5 this fix can/should be uplifted.

Flags: needinfo?(bhackett1024)

Pascal, this patch should be ready to land. Would it help if I fill out the beta uplift form?

Flags: needinfo?(pascalc)

(In reply to Jason Laster [:jlast] from comment #14)

Pascal, this patch should be ready to land. Would it help if I fill out the beta uplift form?

You can fill it out now to save some time yes

Flags: needinfo?(pascalc)
Posted patch up-bp-1.patchSplinter Review

Beta/Release Uplift Approval Request

  • User impact if declined: Some breakpoints that are registered will not appear in the breakpoints panel
  • 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): it only affects the debugger.
  • String changes made/needed:
Attachment #9060256 - Flags: approval-mozilla-beta?
Comment on attachment 9060256 [details] [diff] [review]
up-bp-1.patch

Low risk, covered by tests and impacting only the debugger, uplift approved for 67 beta 14, thanks.
Attachment #9060256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.