1544329 - Can't debug browser console

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Oriol Brufau [:Oriol]]

1. Open browser console
2. Open browser toolbox debugger
3. Press Ctrl+P to find files, type actors/object.js

The file doesn't appear.
Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9f6dc311b7fa154eaae3def3ab38413412ab8d29&tochange=fccf1be20f4b93514bf956a7c826c7990b4c89bd

Comment 1

[Pascal Chevrel:pascalc]

Alexandre it seems your patch in Bug 1532238 caused this regression could you have a look please? Thanks

Comment 2

[Oriol Brufau [:Oriol]]

Bug 1532238 loaded devtools server as invisibleToDebugger, using freshCompartment instead seems to fix the problem.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Oriol Brufau [:Oriol] from comment #2)

Bug 1532238 loaded devtools server as invisibleToDebugger, using freshCompartment instead seems to fix the problem.

Does this mean that we can close the report?

Honza

Comment 4

[Oriol Brufau [:Oriol]]

No, I was only mentioning a possible solution, the problem continues.

Comment 5

[Alexandre Poirot [:ochameau]]

Thanks a lot Oriol for the bug report, the regression window and the fix proposal!

I didn't realized that the browser console was debuggable via the browser console.
For some console I thought that chrome devtools codebase was already using invisibleToDebugger.

I think I'm confused with the purpose of invisibleToDebugger flag and we should review this more globally.
My knowledge of this flag mostly comes this, probably outdated comment over here:
https://searchfox.org/mozilla-central/source/devtools/shared/Loader.jsm#222-231

  /**
   * Sets whether the compartments loaded by this instance should be invisible
   * to the debugger.  Invisibility is needed for loaders that support debugging
   * of chrome code.  This is true of remote target environments, like Fennec or
   * B2G.  It is not the default case for desktop Firefox because we offer the
   * Browser Toolbox for chrome debugging there, which uses its own, separate
   * loader instance.
   * @see devtools/client/framework/ToolboxProcess.jsm
   */
  invisibleToDebugger: Services.appinfo.name !== "Firefox",

The most important thing is that the Debugger should be running in a distinct compartment than its debuggee.
Things changed a lot around compartments of Debugger and Debuggee when debugging chrome when Jan started sharing the exact same compartment for all JS contexts bound to the system principal.
Now, by default, all JSM, chrome documents as well as Sandboxes will all shared the same compartment.
So, all DevTools modules end up sharing the same compartment than all other system principale resources.
So that, in order to debug system principal debuggee, we have to spawn the debugger server in a distinct system compartment.
We could do that via invisibleToDebugger as well as freshCompartment.
The only difference is that invisibleToDebugger will hide all the modules from the debuggee list.

To be honest I'm quite lost to define when we do want to hide chrome resources from the debugger.
When I think about debugging a web page, I would say, yes, always hide devtools modules.
But when I think about debugging the chrome, I would rather say, no, always show the devtools modules.

May be the invisibleToDebuger flag became irrelevant over time?

Jim, do you have any opinion?

Comment 6

[Jim Blandy :jimb]

The invisibleToDebugger flag was created to make sure off-thread-compilation globals and (I think) the globals out of which self-hosted code gets cloned never accidentally become debuggees. It was not designed to manage cases of nested debugging. But I have learned that if you name a flag "invisibleToDebugger" instead of "selfHostedOrOffThread", everyone will hope that they have found the flag that will solve the problem they have in front of them, and it becomes a widely-used tool despite being not very good at its other jobs.

For obvious reasons, we should not allow chrome code to leak into the content debugging experience. But when chrome is debugging chrome, I don't think we need to keep it invisible. Simply putting things in separate compartments, and then choosing debuggee globals correctly, should suffice.

A proper solution would be for globals to be able to give the debugger some information about themselves: whether they're a sandbox or a JSM or an ES6 module or what-have-you, whether they're part of the debugger server and thus should not have breakpoints set in them, and so on. Then the code which decides which globals to add as debuggees could have some trustworthy data to rely on.

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Jim Blandy :jimb from comment #6)

The invisibleToDebugger flag was created to make sure off-thread-compilation globals and (I think) the globals out of which self-hosted code gets cloned never accidentally become debuggees.

Hum. That's interesting, as I think I'm having an issue with self-hosted code when working on my memory scripts.
Having said that, I'm not sure I understand things well enough to draw a conclusion here.
I do understand that invisibleToDebugger should not be used here, for the browser console.
But then... when should we use it? Is there still a case, or particular setup, where we have to ensure setting it to true?

Comment 8

[Oriol Brufau [:Oriol]]

So does it sound good to replace invisibleToDebugger with freshCompartment in https://searchfox.org/mozilla-central/rev/b418634cb3fe83ebb8d2c019cc1ba76974da1a0d/devtools/client/webconsole/hudservice.js#135 just to fix this regression, and leave thoughts about renaming or removing invisibleToDebugger for later?

Comment 9

[Patrick Brosset <:pbro>]

I see this question has not been answered. I think Jim is probably the best person to provide an answer, so let me needinfo him.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Jan, any chance you could help us get this bug un-stuck? We're getting low on time to get a fix into 68.

Comment 11

[Jim Blandy :jimb]

(In reply to Oriol Brufau [:Oriol] from comment #8)

So does it sound good to replace invisibleToDebugger with freshCompartment in https://searchfox.org/mozilla-central/rev/b418634cb3fe83ebb8d2c019cc1ba76974da1a0d/devtools/client/webconsole/hudservice.js#135 just to fix this regression, and leave thoughts about renaming or removing invisibleToDebugger for later?

If that works, then it sounds like a fine solution.

Comment 12

[Jason Laster [:jlast]]

Attachment: Bug 1544329 - Fix debugger schema validation. r=davidwalsh

Comment 13

[Oriol Brufau [:Oriol]]

Attachment: Bug 1544329 - Don't load browser console in an invisibleToDebugger compartment. r?ochameau

Comment 14

[Phabricator Automation]

Comment on attachment 9077779 [details]
Bug 1544329 - Fix debugger schema validation. r=davidwalsh

Revision D37923 was moved to bug 1565485. Setting attachment 9077779 [details] to obsolete.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14f196203dc3
Don't load browser console in an invisibleToDebugger compartment. r=ochameau

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/14f196203dc3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

I don't think this needs to be uplifted to ESR68, but did you want to nominate this for Beta approval?

Comment 18

[Oriol Brufau [:Oriol]]

Comment on attachment 9077798 [details]
Bug 1544329 - Don't load browser console in an invisibleToDebugger compartment. r?ochameau

Beta/Release Uplift Approval Request

• User impact if declined: Browser console can't be debugged by the browser toolbox
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See 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): Just stops loading the browser console in an invisibleToDebugger compartment. Instead it's done in a fresh compartment, to avoid regressing bug 1532238
• String changes made/needed:

Comment 19

[Daniel Cicas [:dcicas], Release QA]

Hello,

I tested this issue and I can confirm it is fixed in Fx 70.0a1 BuildId: 20190723034754 on Windows 10 x64, Ubuntu 18.04 x64 and mac OS 10.13.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9077798 [details]
Bug 1544329 - Don't load browser console in an invisibleToDebugger compartment. r?ochameau

Simple fix which fixes the ability to debug the browser console using the browser toolbox. Verified by QA on Nightly. Approved for 69.0b8.

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b0403a37fb00

Comment 22

[Daniel Cicas [:dcicas], Release QA]

Hello,

I can confirm this is fixed in the latest beta build from treeherder (https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=9fc885879ec5eeb2918c42a04d932a43870246fb).