Closed Bug 1544329 Opened 6 years ago Closed 5 years ago

Can't debug browser console

Categories

(DevTools :: Debugger, defect, P2)

67 Branch
defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 wontfix, firefox66 unaffected, firefox67 wontfix, firefox68 wontfix, firefox69 verified, firefox70 verified)

VERIFIED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox66 --- unaffected
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- verified
firefox70 --- verified

People

(Reporter: Oriol, Assigned: Oriol)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

  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

Has Regression Range: --- → yes
Has STR: --- → yes

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

Flags: needinfo?(poirot.alex)

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

(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

Flags: needinfo?(oriol-bugzilla)

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

Flags: needinfo?(oriol-bugzilla)

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?

Flags: needinfo?(poirot.alex) → needinfo?(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.

Flags: needinfo?(jimb)

(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?

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?

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.

Flags: needinfo?(jimb)

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

Flags: needinfo?(odvarko)

(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.

Flags: needinfo?(jimb)
Flags: needinfo?(odvarko)

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.

Attachment #9077779 - Attachment is obsolete: true
Keywords: checkin-needed
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14f196203dc3
Don't load browser console in an invisibleToDebugger compartment. r=ochameau

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

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

Flags: needinfo?(oriol-bugzilla)

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:
Flags: needinfo?(oriol-bugzilla)
Attachment #9077798 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]
Status: RESOLVED → VERIFIED
Flags: qe-verify+

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 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.

Attachment #9077798 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: