1908095 - Breakpoints are still triggered after being deleted

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P2)

Description

[1250079251]

Attachment: 2024-07-16_17-04-15.mkv

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:128.0) Gecko/20100101 Firefox/128.0

Steps to reproduce:

I debugged in a new environment created in about:profiles

I debugged tab-stash via about:debugging.

I set a breakpoint at /assets/oops-notification.js:2263 and removed it

---

tab-stash-3.0.1: https://addons.mozilla.org/en-US/firefox/addon/tab-stash/

Actual results:

But when I removed the breakpoint, it was still triggered by Firefox.

Expected results:

Deleted breakpoints should not be triggered

Comment 1

[1250079251]

Steps missing from Steps to reproduce:

• Create a new tab

This action will trigger the breakpoint

Comment 2

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

The Bugbug bot thinks this bug should belong to the 'DevTools::Debugger' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Julian Descottes [:jdescottes]]

Had a similar issue when testing webextensions previously, will try to reproduce.

Comment 4

[Julian Descottes [:jdescottes]]

My issue was different: Bug 1869495.

But I do reproduce with the STRs here. You don't even have to hit the breakpoint, just add and remove, then add a tab.

Comment 5

[Julian Descottes [:jdescottes]]

https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/devtools/client/debugger/src/client/firefox/commands.js#244-269

async function removeBreakpoint(location) {
  delete breakpoints[makeBreakpointServerLocationId(location)];

  const hasWatcherSupport = commands.targetCommand.hasTargetWatcherSupport();
  if (!hasWatcherSupport) {
    // Without watcher support, unconditionally forward removeBreakpoint to all threads.
    return forEachThread(async thread => thread.removeBreakpoint(location));
  }
  const breakpointsFront =
    await commands.targetCommand.watcherFront.getBreakpointListActor();
  await breakpointsFront.removeBreakpoint(location);

  // Call removeBreakpoint for threads linked to targets
  // not managed by the watcher.
  return forEachThread(async thread => {
    if (
      !commands.targetCommand.hasTargetWatcherSupport(
        thread.targetFront.targetType
      )
    ) {
      return thread.removeBreakpoint(location);
    }

    return Promise.resolve();
  });
}

For webextension targets, we only call

  const breakpointsFront =
    await commands.targetCommand.watcherFront.getBreakpointListActor();
  await breakpointsFront.removeBreakpoint(location);

Not sure I completely remember the situation for webextensions though.
I don't see any call actually processing the removed data entries, but maybe I'm looking at the wrong spot?

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #5)

For webextension targets, we only call

  const breakpointsFront =
    await commands.targetCommand.watcherFront.getBreakpointListActor();
  await breakpointsFront.removeBreakpoint(location);

Not sure I completely remember the situation for webextensions though.
I don't see any call actually processing the removed data entries, but maybe I'm looking at the wrong spot?

In theory, it should be going through this code:
https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/devtools/server/connectors/js-process-actor/DevToolsProcessChild.sys.mjs#393-417
Is targetActors empty for some reason?

Comment 7

[Julian Descottes [:jdescottes]]

The issue is when removing the breakpoint, and it seems we don't have any special code to handle webextensions there:
https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/devtools/server/connectors/js-process-actor/DevToolsProcessChild.sys.mjs#455-476

#removeSessionDataEntry(watcherActorID, type, entries) {
  const watcherDataObject =
    ContentProcessWatcherRegistry.getWatcherDataObject(watcherActorID, true);

  // When we unwatch resources after targets during the devtools shutdown,
  // the watcher will be removed on last target type unwatch.
  if (!watcherDataObject) {
    return;
  }

  // Maintain the copy of `sessionData` so that it is up-to-date when
  // a new worker target needs to be instantiated
  lazy.SessionDataHelpers.removeSessionDataEntry(
    watcherDataObject.sessionData,
    type,
    entries
  );

  for (const targetActor of watcherDataObject.actors) {
    targetActor.removeSessionDataEntry(type, entries);
  }
}

Comment 8

[Alexandre Poirot [:ochameau]]

Good catch! That must cause various issues, beyond breakpoints...

Comment 9

[Julian Descottes [:jdescottes]]

Will try to write some tests while fixing this.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1908095 - [devtools] removeSessionDataEntry for webextension toolboxes and watcher targets

Fix removing breakpoints in webextension toolboxes and watcher targets, add tests to cover both scenarios

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1908095 - [devtools] Regenerate debugger mochitest sourcemap resources

Depends on D218084

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a57fba6a542b
[devtools] removeSessionDataEntry for webextension toolboxes and watcher targets r=ochameau,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/a6ccaa04f182
[devtools] Regenerate debugger mochitest sourcemap resources r=devtools-reviewers,nchevobbe

Comment 13

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/a57fba6a542b
https://hg.mozilla.org/mozilla-central/rev/a6ccaa04f182

Comment 14

[Julian Descottes [:jdescottes]]

The fix has now landed on our Nightly channel. If possible, could you try the latest Nightly and verify this is fixed?

Comment 15

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

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for uplift? Fx130 goes to RC next week.

Comment 17

[Julian Descottes [:jdescottes]]

Oh right I forgot about this during mozweek.
It would be great to uplift it if possible, this can be quite annoying when debugging webextensions and workers.

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 9416815 [details]
Bug 1908095 - [devtools] removeSessionDataEntry for webextension toolboxes and watcher targets

Beta/Release Uplift Approval Request

• User impact if declined: Unable to remove breakpoints when debugging webextensions or workers
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Devtools only change, which is applying the same logic to handle devtools session data (including breakpoints) for webextension/worker targets in the same way as for regular web targets.
• String changes made/needed:
• Is Android affected?: No

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9416815 [details]
Bug 1908095 - [devtools] removeSessionDataEntry for webextension toolboxes and watcher targets

Approved for 130.0b8.

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/041c923216d9
https://hg.mozilla.org/releases/mozilla-beta/rev/b40b221056a4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9416815 [details]
Bug 1908095 - [devtools] removeSessionDataEntry for webextension toolboxes and watcher targets

Approved for 128.2esr.

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/85060db8ce99
https://hg.mozilla.org/releases/mozilla-esr128/rev/21bf8880f772

Comment 23

[Atila Butkovits]

Backed out from Esr128 for causing failures at browser_dbg-features-breakpoints.js.

Backout link: https://hg.mozilla.org/releases/mozilla-esr128/rev/b59270108b4e16f03a6272a47d3b819fdad66c4d

Push with failures: https://treeherder.mozilla.org/jobs?repo=mozilla-esr128&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=5d69735a10801f60a8bba889837eb052087f6099&selectedTaskRun=JGUWcs0QS3C11fdHfgNzVQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=471105823&repo=mozilla-esr128&lineNumber=8906

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Feel free to wontfix this for ESR128 if it's not a trivial fix-up.

Comment 25

[Julian Descottes [:jdescottes]]

I imagine the patch itself is fine, and it's rather the test update which might be missing some other intermediary patches?
Let's see if the test fails without the updates here. If not, I think we can probably remove the part which tests workers from the test patch, there is already a test for webextensions, and both use the same codepath, so I think it would give enough coverage.

Comment 26

[Julian Descottes [:jdescottes]]

I verified on NIGHTLY

Comment 27

[1250079251]

cool! The bug is gone!

Comment 28

[Julian Descottes [:jdescottes]]

Actually the implementation for workers was a bit different in 128, but it should be easy to adapt.

Comment 29

[Julian Descottes [:jdescottes]]

Attachment: Bug 1908095 - [devtools] removeSessionDataEntry for webextension toolboxes and watcher targets r=ochameau,devtools-reviewers a=RyanVM

Fix removing breakpoints in webextension toolboxes and watcher targets, add tests to cover both scenarios
Implementation update to fit 128 ESR

Comment 30

[Julian Descottes [:jdescottes]]

Attachment: Bug 1908095 - [devtools] Regenerate debugger mochitest sourcemap resources r=devtools-reviewers,nchevobbe a=RyanVM

Depends on D219752

Comment 31

[Julian Descottes [:jdescottes]]

Did a try push with my changes on top of ESR128 https://treeherder.mozilla.org/jobs?repo=try&revision=be37f112f849da8a8d24f99fd09e38b473c69dbe, hopefully this can tell us if the adapted version can work.

There was an error in my first patch, new try push at https://treeherder.mozilla.org/jobs?repo=try&revision=b2c00ca39a5c211eed24af05f6f91bc360167b10

Comment 32

[Julian Descottes [:jdescottes]]

Ryan: Could we use D219752 and D219753 for the ESR 128 uplift? The tests seem to be fine on my last try push

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Sure, thanks for doing the rebase!

Comment 34

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/7f2a28b89191
https://hg.mozilla.org/releases/mozilla-esr128/rev/abc9b44e67da