1748589 - Event breakpoints are not managed properly

Status: VERIFIED FIXED

(DevTools :: Debugger, defect)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce

1. Navigate to data:text/html,<meta charset=utf8><button>Click</button><script>document.querySelector("button").addEventListener("click", e => console.log(e))</script>
2. Open the debugger
3. In the event listener breakpoint panel, check click
4. Click on the button in the content page, the debugger should pause
5. In the event listener breakpoint panel, uncheck click
6. Click on the button in the content page again

Expected results

Nothing should happen

Actual results

The debugger pauses

---

I can reproduce in 94, but not in 91 ESR, I'll try to find a better regression range

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Running mozregression I got this range https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7f3e1f5b6253d91b5eeb60ac23a35cb12bafa5a&tochange=4eda9eb8926bdd50f4b80128ce3475eb7c6d9a4d , in which we can see Bug 1717802 , which seems to be a good candidate for regression

The event breakpoints tests we have don't cover removing breakpoints which is why it went unnoticed

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

In target-actor-mixin, addSessionDataEntry does have a if block for EVENT_BREAKPOINTS, but removeSessionDataEntry does not:
https://searchfox.org/mozilla-central/rev/fbf1e796ecead9484deced4d99f199f327ba25ab/devtools/server/actors/targets/target-actor-mixin.js#111,124,126,130,132

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Note that we're also not adding the event breakpoints properly
In https://searchfox.org/mozilla-central/rev/fbf1e796ecead9484deced4d99f199f327ba25ab/devtools/server/actors/targets/target-actor-mixin.js#119 , we're setting the active event breakpoints to be what is sent from the client, which is basically the last checked event in the debugger. So in the debugger event listener breakpoints panel if you:

• check the click checkbox
• and then check the auxclick checkbox

only the auxclick event listener bp will be enabled, not the click one

Comment 4

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

Set release status flags based on info from the regressing bug 1717802

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1748589 - [devtools] Fix event listener breakpoints toggling. r=bomsy.

In target-actor-mixin, we were calling setActiveEventBreakpoints only with the
new events we were receiving, which mean if the user was clicking a first event in
the UI, and then a second one, only the second one would have a functioning event breakpoint.
Also, we were not handling removing event breakpoints at all.
We're adding (add|remove)EventBreakpoints to the thread actor so it's easier
to update the list of event breakpoints.

The existing event breakpoints test is modified to ensure we don't regress such behaviour.

The call to setEventListenerBreakpoints is moved before dispatch the UPDATE_EVENT_LISTENERS
action so we can properly wait for the breakpoint to be set in tests.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26b0f3f4e2ab
[devtools] Fix event listener breakpoints toggling. r=bomsy.

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/26b0f3f4e2ab

Comment 8

[Ryan VanderMeulen (PTO, back 6-April)]

Is this something you wanted to nominate for Beta uplift?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

yes, it would be nice

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9257820 [details]
Bug 1748589 - [devtools] Fix event listener breakpoints toggling. r=bomsy.

Beta/Release Uplift Approval Request

• User impact if declined: In the DevTools debugger, event listener breakpoints won't work great:

• breakpoints can't be removed, meaning the user will get paused when they don't expect to
• only the last event breakpoint checked will be enabled, which will lead the debugger to not pause when the user expect to

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See STR from Comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): DevTools only change, with a mochitest
• String changes made/needed:

Comment 11

[Giorgia Nichita, Release Desktop QA]

Reproduced this issue using an affected Nightly build from 2022-01-04, following the STR from Comment 0, on macOS 10.14.

Verified fixed on 98.0a1 (20220111213255) across the following platforms: macOS 10.14, Windows 10 x64 and Ubuntu 20.04.

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9257820 [details]
Bug 1748589 - [devtools] Fix event listener breakpoints toggling. r=bomsy.

Approved for 97.0b3. Thanks.

Comment 13

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/333e8be95340

Comment 14

[Giorgia Nichita, Release Desktop QA]

Verified fixed on 97.0b3 using a treeherder build (Build ID: 20220112193650) across the following platforms: macOS 10.14, Windows 10 x64 and Ubuntu 20.04.