Closed Bug 794599 Opened 8 years ago Closed 8 years ago

Disable console by default in release builds

Categories

(Core :: General, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: cjones, Assigned: dhylands)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
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?
blocking-basecamp: ? → +
Assignee: nobody → dhylands
Priority: -- → P2
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.
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).
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.
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.
Adds a preference to the console service to control logging.
Adds some JS code to reflect the gaia setting onto the preference.
Attachment #677630 - Flags: review?(philipp)
Attachment #677631 - Flags: review?(21)
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.
Attachment #677631 - Flags: review?(21) → review-
(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 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 {
Attachment #677630 - Flags: review?(philipp)
Attachment #677630 - Flags: review?(bzbarsky)
Attachment #677630 - Flags: review+
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.
Attachment #677630 - Flags: review?(bzbarsky) → review+
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.
Depends on: 808060
Adds a developer debug item to the settings app
Removed settings.json
Added settings.py to generate settings.json
Attachment #677631 - Attachment is obsolete: true
Attachment #677936 - Flags: review?(21)
Updated the gonk build to use the newly created settings.py script.
Attachment #677940 - Flags: review?(21)
Updated the code so that the preference consoleservice.enabled is still a simple reflection of the debug.console.enabled setting.
Attachment #677630 - Attachment is obsolete: true
Attachment #677942 - Flags: review?(bzbarsky)
Comment on attachment 677942 [details] [diff] [review]
v2 of the gecko portion of disabling the console

r=me
Attachment #677942 - Flags: review?(bzbarsky) → review+
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 :-)
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
Attachment #677942 - Attachment is obsolete: true
Attachment #679246 - Flags: review?(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...
Attachment #679246 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/ef57a12da1fe
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.