1728290 - Inconsistent behavior on `debugger;` statements when React Developer Tools webextension is installed

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Needs to be verified by others but for me, it seems that the debugger behaves strangely on some breakpoints when the React Developer Tools webextension is installed.

STRs:

• Install react developer tools https://addons.mozilla.org/fr/firefox/addon/react-devtools/
• Open https://bug1728287.bmoattachments.org/attachment.cgi?id=9238643 (test page from Bug 1728287)
• Open DevTools
• Debugger should quickly break on a debugger statement in a setTimeout loop
• Attempt to resume by clicking once (only once!) on one of the resume buttons

ER: Should resume
AR: Need to click a second time for the debugger to resume. Note that very rarely, it works with a single click. But 95% of the time I need to click twice, or it goes into a weird state and I need to click both on the resume button from the overlay and the one from the toolbox. Or I get no overlay, etc....

After disabling React DevTools from the DevTools settings (don't even have to uninstall them), breakpoints work a lot more consistently.

Some features such as disable all breakpoints also seem to behave oddly.

Comment 1

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Changing severity to S3 because hard to trigger (need specific extension)

Comment 2

[Hubert Boma Manilla (:bomsy)]

I can consistently reproduce the overlay issue, after first reload ...

Another STR (without setTimeout)

1. With react devtoper tools extension installed (as above)
2. https://phantom-breakpoints.glitch.me/
3. Open the fx devtools debugger
4. Reload the page
5. The debugger statement on line 11 should be hit
6. Click resume

AR
Debugger resumes but the overlay is not cleared (need to click resume in the overlay again to clear)

ER
Debugger resumes and the overlay disappears

Note:
After reloading the page again, everything seems to work properly for me

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

I quickly checked, and it's because once the user resumes (from the top level target), we hit https://searchfox.org/mozilla-central/rev/3fa5cc437a4937c621ea068ba5dc246f75831633/devtools/server/actors/thread.js#491 from the webextension target that is created for the React webextension

Comment 4

[mattheww]

I'd like to work on this, please.

The issue with breakpoints being hit inconsistently has been really frustrating for me. If you add a breakpoint, then reload the page, it will get paused on twice. Even if you remove it, it'll still pause once until you reload again. Occasionally, the pause overlay doesn't appear, so there's no way to continue at all.

It seems like when devtools.inspectedWindow.eval() is called for the first time, the devtools commands created in ExtensionParent attach a debugger and start listening for breakpoints and debugger statements, even though this isn't necessary. My plan is to extend the createdForBrowserConsole check in TargetMixin._attachAndInitThread() to also not attach for webconsole-owned descriptors.

• Is this a good approach? It works fine, but createdForBrowserConsole is described as a hack. I'm not sure if I should be increasing the reliance on it.
• Should the test go in devtools/client/debugger or toolkit/components/extensions?

Comment 5

[Hubert Boma Manilla (:bomsy)]

Hi Matthew,
Thanks for looking into this. Sure you can work on it.

(In reply to mattheww from comment #4)

I'd like to work on this, please.

Thanks for looking into this. Sure you can work on it. Assigning to you.

The issue with breakpoints being hit inconsistently has been really frustrating for me. If you add a breakpoint, then reload the page, it will get paused on twice. Even if you remove it, it'll still pause once until you reload again. Occasionally, the pause overlay doesn't appear, so there's no way to continue at all.

It seems like when devtools.inspectedWindow.eval() is called for the first time, the devtools commands created in ExtensionParent attach a debugger and start listening for breakpoints and debugger statements, even though this isn't necessary. My plan is to extend the createdForBrowserConsole check in TargetMixin._attachAndInitThread() to also not attach for webconsole-owned descriptors.

• Is this a good approach? It works fine, but createdForBrowserConsole is described as a hack. I'm not sure if I should be increasing the reliance on it.
• Should the test go in devtools/client/debugger or toolkit/components/extensions?

Seems like you are on the right path.
Pinging Alex has more experience in this area, so should be able to answer and guide.

Comment 6

[Alexandre Poirot [:ochameau]]

The main trouble we might get here is about disabling the thread actor.
WebExtension usages of the DevTools is only about the console actor and InspectedWindow commands, which relates to a dedicated actor.
While the console can interact with the thread actor, it should also work without it. As that's what happen with the browser console.

So it should be fine disabling it for the WebExtension targets. But we should have some extensive manual testing.

Now regarding how to disable the thread actor, you are on the right track.
We should either:

• have TargetMixin._attachAndInitThread bail out on descriptorFront.createdForBrowserConsole || descriptorFront.isForWebExtension (you would have to set this isForWebExtension boolean from TabDescriptorFront.setIsForWebExtension.
  or
• rename createdForBrowserConsole to disableThreadActor/doNotCreateThreadActor/doNotAttachThreadActor/... and have both CommandsFactory.createdForBrowserConsole and CommandsFactory.forTab set this flag on the descriptor.
  I have a slight preference for the second.
  The hack wasn't really about disable the thread actor, but rather about how to convey information between CommandsFactory down to TargetMixin. Putting a flag coming out of the blue like this feels hacky. But I don't have much better suggestion.

Regarding the tests, I would rather see it in the debugger folder as there isn't much specifics around webextensions.
The following test might be a useful template to start from:
https://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser_dbg-content-script-sources.js

Comment 7

[mattheww]

Attachment: Bug 1728290 - don't connect devtools webextensions to ThreadActors r=ochameau

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab2f0e774375
don't connect devtools webextensions to ThreadActors r=ochameau

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/ab2f0e774375

Comment 10

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

I managed to reproduce this bug on Firefox 98.0(20220304153049) on macOS 11. Confirmed as fixed on Firefox 100.0b8(20220419190903) and Nightly 101.0a1(20220419214607) on macOS 11, Win10 64-bits and Linux x86_64(Ubuntu 20.04).