1210790 - Consider removing the devtools.chrome.enabled pref.

Status: NEW

(DevTools :: General, defect)

Description

[Jan Keromnes [:janx]]

(From J. Ryan Stinnett [:jryans] (use ni?) in bug 1196785 comment 75)
> I think overall the pref's existence is a bit strange...  It's not a
> security protection really.  It hides browser debugging so as to not
> overwhelm people (I guess?).
> 
> Even then, I'm not sure it's worth it.  Let's file a follow up bug and ni?
> :canuckistani so we can re-debate this somewhere that I can find again.
> 
> My current opinion is that it would be nice not to need this pref, at least
> for the about:debugging page (but maybe also in other places too), since
> it's pretty clear from the categories what you are looking at, and you just
> want to use the thing.  As it is today, the check box is basically saying
> "should this page function correctly?" which is odd question to ask, I think.

There are checkboxes on both pages, because they both need a pref enabled to work (worker debugging or chrome debugging for addons). In my WIP devices category, there a remote debugging checkbox. But I'm all for removing useless flags.

Jeff, what do you think. Should we nuke devtools.chrome.enabled ?

Comment 1

[:Gijs (out for now; he/him)]

FWIW, I think we should keep this flag - and that's despite me having to set it in all my profiles, and in the profiles of people who I need to run random JS to figure out what's breaking for a particular bug, and so on and so forth. I think from a defense in depth perspective, it makes sense to keep this - we're a pretty small number of people that are "bitten" by this.

Comment 2

[Brian Grinstead [:bgrins]]

IIRC devtools.chrome.enabled has a confusing meaning for the browser toolbox - it stops the UI from showing up if it's false [0], but it wouldn't actually stop a remote connection from happening - that's done by  devtools.debugger.remote-enabled.

It does have a specific meaning for the Browser Console and Scratchpad though, and that's 'allow user to execute chrome-privileged code'.  And I think that's an important safeguard to have.

For this particular feature in about:debugging, when you say 'worker debugging' do you mean any worker or just ones running with chrome privilege?  Also I may not be remembering correctly but I recall that devtools.debugger.remote-enabled is what's used for addon debugging from about:addons, so maybe that should also be true for addon debugging in about:debugging

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/gDevTools.jsm#610

Comment 3

[Jan Keromnes [:janx]]

Brian it seems you're right, I didn't fully understand what the prefs do exactly. I just copied them from the Toolbox settings.

"devtools.debugger.remote-enabled" is needed for debugging Add-ons (because that uses a BrowserToolboxProcess).

"devtools.chrome.enabled" doesn't seem to be needed for debugging Add-ons (disabling it just removes the "Debug" button in about:addons, but the one in about:debugging still appears to work) or Workers (even chrome workers surprisingly).

"devtools.debugger.workers" I also just snatched from the Toobox options, but it only prevents the Workers[1] and the WorkersView[2] objects from working, neither of which are used by about:debugging (so worker debugging happens anyway).

[1] Workers https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/debugger-controller.js#431
[2] WorkersView https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/views/workers-view.js

I should probably add a checkbox for "devtools.debugger.remote-enabled" to the Add-ons pane, and either honor the two other prefs (i.e. by refusing to display/debug the associated targets in about:debugging) or remove their checkboxes.

Comment 4

[Jan Keromnes [:janx]]

Getting back on topic, it seems that "devtools.chrome.enabled" essentially just hides browser-chrome tools in order to "protect" users, except for the Browser Console & Scratchpad in which the pref can prevent the execution of chrome-privileged code (so in theory, if a Browser Console can be opened although "devtools.chrome.enabled" is false and the UI is hidden, it still won't have chrome privileges).

Ryan, given that you were the one suggesting we remove that pref, what do you think of this and Gijs' reply in comment 1?

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Jan Keromnes [:janx] from comment #4)
> (so in theory, if a Browser Console can be opened
> although "devtools.chrome.enabled" is false and the UI is hidden, it still
> won't have chrome privileges).

This isn't a theory - you can do this, and the input box is just not there, so users can copy/paste error messages but can't input anything. :-)

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Jan Keromnes [:janx] from comment #3)
> "devtools.chrome.enabled" doesn't seem to be needed for debugging Add-ons
> (disabling it just removes the "Debug" button in about:addons, but the one
> in about:debugging still appears to work) or Workers (even chrome workers
> surprisingly).

Can you please file a follow up for about:debugging to prevent listing / connecting to chrome workers unless if devtools.chrome.enabled is true?

> "devtools.debugger.workers" I also just snatched from the Toobox options,
> but it only prevents the Workers[1] and the WorkersView[2] objects from
> working, neither of which are used by about:debugging (so worker debugging
> happens anyway).
> 
> [1] Workers
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/
> debugger-controller.js#431
> [2] WorkersView
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/
> views/workers-view.js

Regarding the devtools.debugger.workers pref: I think this is what you are saying, but to be clear we shouldn't rely on the same pref that enables the experimental toolbox UI for worker debugging.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Here's my summary of the discussion so far:

* We don't need "devtools.debugger.workers" for about:debugging at all and this checkbox can be removed
* "devtools.chrome.enabled" the pref does have some purposes, but the only (future) effect it may have on about:debugging is filtering ChromeWorkers, like :bgrins suggests in comment 6.  Should about:debugging have a checkbox to control it?  Not clear, maybe not.
* "devtools.debugger.remote-enabled" is needed for add-ons currently.  Should we add a checkbox for this to about:debugging?

Comment 8

[Alexandre Poirot [:ochameau]]

What about setting this pref (and may be otherts) when running firefox from ./mach run?
(bug 1198759's patch may help figuring out where to hack that)

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jan Keromnes [:janx] from comment #0)
> (From J. Ryan Stinnett [:jryans] (use ni?) in bug 1196785 comment 75)
> > I think overall the pref's existence is a bit strange...  It's not a
> > security protection really.  It hides browser debugging so as to not
> > overwhelm people (I guess?).
> > 
> > Even then, I'm not sure it's worth it.  Let's file a follow up bug and ni?
> > :canuckistani so we can re-debate this somewhere that I can find again.
> > 
> > My current opinion is that it would be nice not to need this pref, at least
> > for the about:debugging page (but maybe also in other places too), since
> > it's pretty clear from the categories what you are looking at, and you just
> > want to use the thing.  As it is today, the check box is basically saying
> > "should this page function correctly?" which is odd question to ask, I think.
> 
> There are checkboxes on both pages, because they both need a pref enabled to
> work (worker debugging or chrome debugging for addons). In my WIP devices
> category, there a remote debugging checkbox. But I'm all for removing
> useless flags.
> 
> Jeff, what do you think. Should we nuke devtools.chrome.enabled ?

Sounds like you guys got this.