794599 - Disable console by default in release builds

Status: RESOLVED FIXED

(Core :: General, defect, P2)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The implementation of the console is extremely expensive: lots of heap allocation, context switching, system calls, etc.  For sites that generate lots of CSS/JS warnings, this is going to be *significant* overhead.

All the crap we're probably doing for a site like CNN just makes me want to weep.

We should add a "Developer" option to turn the spew back on.

I'm going to nom for basecamp mostly to get this on the radar and hopefully someone working on it.  Don't have a strong opinion on blocking+/- yet.

Comment 1

[Andrew Overholt [:overholt]]

This doesn't feel like something we'd block the release on but seems like something we should do.  Who could do this?  Do we have a corresponding gaia issue for the "Developer" option?

Comment 2

[Benjamin Smedberg]

From email:

> So fundamentally, we'd like to have some type of on/off toggle which could enable/disable the log at runtime.
> Adding a new enabled attribute to the idl seems like the logical thing to do.

Presumably the primary purpose of this toggle is to avoid IPC traffic for console messages that we aren't going to use anyway, right? In that case yes, a bool flag makes sense.

Comment 3

[Dave Hylands [:dhylands]]

So, I'm going to propose that we create a setting called debug.console.enabled.

The settings app would control this like the other developer options. The question is does anybody have a preference on how to communicate this information from the settings app down to nsIConsoleService?

I could have the console service look for changes in the setting (I'm familiar with how to do this on the C++ side of things). Or I could have the js side do that watching and do something to call an function which just takes an enabled/disabled boolean (this part I'm not familiar with).

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

You know about http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#124 , right? :)  Maybe we're lucky and the console already has a turn-everything-off pref.

Comment 5

[Dave Hylands [:dhylands]]

I wasn't aware of the settings.js file. That looks useful. I'll need to read up on prefs. I couldn't see any evidence of a pref in the ConsoleService, but I'll see what's required to add one.

Comment 6

[Dave Hylands [:dhylands]]

Attachment: Adds a preference to control logging

Adds a preference to the console service to control logging.
Adds some JS code to reflect the gaia setting onto the preference.

Comment 7

[Dave Hylands [:dhylands]]

Attachment: Adds a developer debug item to the settings app (to control console logging)

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 677631 [details] [diff] [review]
Adds a developer debug item to the settings app (to control console logging)

Review of attachment 677631 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/settings.json
@@ +6,5 @@
>   "audio.volume.system": 7,
>   "audio.volume.voice_call": 5,
>   "audio.volume.fm": 15,
>   "bluetooth.enabled": false,
> + "debug.console.enabled": -1,

Why is it -1 instead of true/false? I feel like this value can be set at build time instead of relying on settings.js to do something with it when it first read it.

Comment 9

[Dave Hylands [:dhylands]]

(In reply to Vivien Nicolas (:vingtetun) from comment #8)
> Comment on attachment 677631 [details] [diff] [review]
> Adds a developer debug item to the settings app (to control console logging)
> 
> Review of attachment 677631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/settings.json
> @@ +6,5 @@
> >   "audio.volume.system": 7,
> >   "audio.volume.voice_call": 5,
> >   "audio.volume.fm": 15,
> >   "bluetooth.enabled": false,
> > + "debug.console.enabled": -1,
> 
> Why is it -1 instead of true/false? I feel like this value can be set at
> build time instead of relying on settings.js to do something with it when it
> first read it.

The bug requests that the default value of the setting be true for debug builds and false for release builds. Since the setting originates from a JSON file, and not a .js file, this isn't really possible.

So I decided to set it to -1 and have the code change that into a boolean at runtime. That particular bit of code is in the other patch (b2g/chrome/content/settings.js)

I guess the other option is to change setting.json into settings.js or settings.py and have settings.json be generated. Then I could do the change at build time.

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 677630 [details] [diff] [review]
Adds a preference to control logging

Review of attachment 677630 [details] [diff] [review]:
-----------------------------------------------------------------

I can't really review the nsConsoleService changes. Fling that part over to bz who according to hg history has done reviews there before. bz, would you mind taking a quick look? Thanks!

::: b2g/chrome/content/settings.js
@@ +69,5 @@
> +SettingsListener.observe('debug.console.enabled', true, function(value) {
> +  // If value is numeric, then it hasn't yet been converted value.
> +  // So we figure out the correct initial value and store it.
> +  if (typeof value != "boolean") {
> +    if (typeof value == "number" && (value < 0)) {

I agree with Vivien. Why don't we make the initial value `null`?

  if (value === null) {
    value = Cc["@mozilla.org/xpcom/debug;1"].getService(Ci.nsIDebug2).isDebugBuild;
  } else {

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 677630 [details] [diff] [review]
Adds a preference to control logging

Maybe I'm missing something...  This is adding a new preference right?  What does the "hasn't yet been converted value" bit mean?

I would prefer the preference on the Gecko side be called "consoleservice.enabled", given what it does.

I assume the plan is to flip the pref for b2g only, and so extension compat is not an issue?

r=me with the above fixed or explained as relevant.

Comment 12

[Dave Hylands [:dhylands]]

The "hasn't (In reply to Boris Zbarsky (:bz) from comment #11)
> Comment on attachment 677630 [details] [diff] [review]
> Adds a preference to control logging
> 
> Maybe I'm missing something...  This is adding a new preference right?  What
> does the "hasn't yet been converted value" bit mean?

That's going to go away. We previously had our initial settings in a json file, and I'm rewriting it so that the json file is generated by a python script.

Then the initial setting will be on for debug builds, and off for release builds, and we'll just reflect the setting into the preference.

> I would prefer the preference on the Gecko side be called
> "consoleservice.enabled", given what it does.

No problem.

> I assume the plan is to flip the pref for b2g only, and so extension compat
> is not an issue?

The preference will default to true if it doesn't exist, so this will preserve the previous behaviour by default.

Comment 13

[Dave Hylands [:dhylands]]

Attachment: Adds a preference to control logging

Adds a developer debug item to the settings app
Removed settings.json
Added settings.py to generate settings.json

Comment 14

[Dave Hylands [:dhylands]]

Attachment: Updated gonk build to use new settings.py script

Updated the gonk build to use the newly created settings.py script.

Comment 15

[Dave Hylands [:dhylands]]

Attachment: v2 of the gecko portion of disabling the console

Updated the code so that the preference consoleservice.enabled is still a simple reflection of the debug.console.enabled setting.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 677942 [details] [diff] [review]
v2 of the gecko portion of disabling the console

r=me

Comment 17

[Dave Hylands [:dhylands]]

gecko portion pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/847cedab81ae

Comment 18

[Dave Hylands [:dhylands]]

Backed out due to debug orange
https://hg.mozilla.org/integration/mozilla-inbound/rev/443de24c2268

Comment 19

[Ed Morley [:emorley]]

When backing out please can you use a commit message like:
  "Backout/Back out/Backed out <changeset> (<and ideally also bug id>) reason"; since:
  "Bug 794599 - Backout 847cedab81ae due to debug orange"

...looks like another normal landing to the tool due to the Bug N prefix. (Adding support to the tool for the latter format would result in false positives).

Cheers :-)

Comment 20

[Dave Hylands [:dhylands]]

Attachment: v3 of the gecko portion of disabling the console

So the patch as written seems to cause some of the tests to fail on debug builds.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=847cedab81ae

So it appears that nsConsoleService::Init() may get called from other than the main thread.
I created a runnable which adds the pref watcher on the main thread and the debug orange goes away:
https://tbpl.mozilla.org/?tree=Try&rev=23ab85341a8c

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 679246 [details] [diff] [review]
v3 of the gecko portion of disabling the console

r=me.  Good catch on the threading thing...

Comment 22

[Dave Hylands [:dhylands]]

Pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef57a12da1fe

Comment 23

[(no longer active)]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e132143c1df4