1874411 - Duplicated column breakpoint when adding a breakpoint on a line

Status: ASSIGNED

(DevTools :: Debugger, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

Attachment: duplicated column breakpoints

Steps to reproduce

1. Navigate to https://ffx-devtools-duplicated-column-breakpoint.glitch.me/
2. Open the debugger
3. Open the script.js source
4. Set a breakpoint on line 21 (resolve();)

Expected results

A breakpoint is set on the line

Actual results

Duplicated column breakpoints are displayed (see attached screenshot)

---

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

I can reproduce on 121, but not on 115 ESR

Comment 2

[Daniel Bodea [:danibodea]]

I can reproduce this issue in Release v121.0.1 and Beta v122.0b9, but not in Nightly v123.0a1 or ESR v115.6.0esr.

Firstly, I've attempted to find its fix, however, mozregression does not seem to offer the correct fix-regressor:

2024-01-15T10:51:58.863000: DEBUG : Found commit message:
Bug 1870100 - Move browser toolbar colors to browser-shared.css. r=dao,desktop-theme-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D196592

2024-01-15T10:51:58.863000: DEBUG : Did not find a branch, checking all integration branches
2024-01-15T10:51:58.865000: INFO : The bisection is done.
2024-01-15T10:51:58.866000: INFO : Stopped

But the change was definitely pushed between Nightly of 2023-12-16 (bad) and Nightly of 2023-12-17 (good).

Furthermore, I've attempted to find the regressor between branch 121 and 115:

2024-01-15T11:06:55.763000: DEBUG : Found commit message:
Bug 1856658 - [devtools] Avoid duplicating the large breakpoints positions array when using function filtering methods. r=devtools-reviewers,nchevobbe

The filterBySource was looping over the whole list once again but worse, was duplicating the array once more.

Same for filterByUniqLocation.

Finally, once we removed these two last filtering methods... we could prevent the creation of the whole "positions" intermediate object and craft directly what we aim to store in redux.

Differential Revision: https://phabricator.services.mozilla.com/D189951

2024-01-15T11:06:55.763000: DEBUG : Did not find a branch, checking all integration branches
2024-01-15T11:06:55.765000: INFO : The bisection is done.
2024-01-15T11:06:55.766000: INFO : Stopped

This result appears correct: bug 1856658.

Comment 3

[Hubert Boma Manilla (:bomsy)]

Did a quick bit of investigation, it seems like in this particular scenario getPossibleBreakpoints returns duplicate offsets for line 21 with column 12 https://searchfox.org/mozilla-central/rev/bc6a50e6f08db0bb371ef7197c472555499e82c0/devtools/server/actors/source.js#487

Maybe the regressing patch stopped handling the duplicates.

We can handle duplicates columns on the devtools side, which would be an easy fix, but it might make sense to check from the getPossibleBreakpoints.

I'm trying to add tests to help.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

:ochameau, since you are the author of the regressor, bug 1856658, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 5

[Hubert Boma Manilla (:bomsy)]

Attachment: WIP: Bug 1874411 - [devtools] Fix duplicate column breakpoints

Comment 6

[Bryan Thrall [:bthrall]]

I reproduced this in a local Firefox debug build on mozilla-central rev. 792c98927914 and analyzed it in Pernos.co. All of the calls to DebuggerScript::CallData::getPossibleBreakpoints() for the timer callback function return four unique breakpoint offsets. This indicates something is going wrong between there and where :bomsy was looking in comment 3.

I also noticed that once multiple column breakpoints on line 21 are visible, you can add more by toggling the breakpoint on line 18 on and off. Reloading the page resets the number of column breakpoints on line 21 to two.

I don't think the duplicate column breakpoints are coming from the SpiderMonkey Debugger API here, but if there's anything else you'd like me to look into, let me know!

Comment 7

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1874411 - [devtools] Fix duplicate column breakpoint positions r=#devtools-reviewers

Comment 8

[Hubert Boma Manilla (:bomsy)]

(In reply to Bryan Thrall [:bthrall] from comment #6)

I reproduced this in a local Firefox debug build on mozilla-central rev. 792c98927914 and analyzed it in Pernos.co. All of the calls to DebuggerScript::CallData::getPossibleBreakpoints() for the timer callback function return four unique breakpoint offsets. This indicates something is going wrong between there and where :bomsy was looking in comment 3.

I also noticed that once multiple column breakpoints on line 21 are visible, you can add more by toggling the breakpoint on line 18 on and off. Reloading the page resets the number of column breakpoints on line 21 to two.

I don't think the duplicate column breakpoints are coming from the SpiderMonkey Debugger API here, but if there's anything else you'd like me to look into, let me know!

Thanks Bryan, for looking into this.
I can indeed confirm the issue was not the spider monkey api but rather a bug here when getting top level debuggee scripts.
This issue was a descendant script was not getting removed because its parent script got removed first.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Hey Bryan, Alex has some questions for you on phabricator: https://phabricator.services.mozilla.com/D209513#7214132

(adding a needinfo so this doesn't go unnoticed :) )

Comment 10

[Bryan Thrall [:bthrall]]

From what I can see, this bug is currently waiting on code review for :bomsy's patch.

Is there any information or support you need from me or anyone else on the SpiderMonkey team?

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

I think bomsy had some questions/request for you, I'll let him write them here

Comment 12

[Hubert Boma Manilla (:bomsy)]

(In reply to Bryan Thrall [:bthrall] from comment #10)

From what I can see, this bug is currently waiting on code review for :bomsy's patch.

Is there any information or support you need from me or anyone else on the SpiderMonkey team?

Hi Bryan,
Sorry about the delays, from my end.
We had a quick chat in the tools meeting, and the conclusion was that it might be better to wait for the platform fixes and rather than go ahead with this patch (which will be more of a temporary fix).
Maybe we can help with providing tests for the platform bugs to progress ?

Comment 13

[Bryan Thrall [:bthrall]]

Sounds good!

I should be able to get started on the platform fixes next week.