If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Disable console by default in release builds

RESOLVED FIXED in Firefox 18

Status

()

Core
General
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: dhylands)

Tracking

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 677630 [details] [diff] [review]
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.
Attachment #677630 - Flags: review?(philipp)
(Assignee)

Comment 7

5 years ago
Created attachment 677631 [details] [diff] [review]
Adds a developer debug item to the settings app (to control console logging)
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 9

5 years ago
(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+
(Assignee)

Comment 12

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 808060
(Assignee)

Comment 13

5 years ago
Created attachment 677936 [details] [diff] [review]
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
Attachment #677631 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #677936 - Flags: review?(21)
(Assignee)

Comment 14

5 years ago
Created attachment 677940 [details] [diff] [review]
Updated gonk build to use new settings.py script

Updated the gonk build to use the newly created settings.py script.
Attachment #677940 - Flags: review?(21)
(Assignee)

Comment 15

5 years ago
Created attachment 677942 [details] [diff] [review]
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.
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+
Attachment #677936 - Flags: review?(21) → review+
Attachment #677940 - Flags: review?(21) → review+
(Assignee)

Comment 17

5 years ago
gecko portion pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/847cedab81ae
(Assignee)

Comment 18

5 years ago
Backed out due to debug orange
https://hg.mozilla.org/integration/mozilla-inbound/rev/443de24c2268
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 :-)
(Assignee)

Comment 20

5 years ago
Created attachment 679246 [details] [diff] [review]
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
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+
(Assignee)

Comment 22

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

Comment 23

5 years ago
https://hg.mozilla.org/mozilla-central/rev/ef57a12da1fe
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
https://hg.mozilla.org/releases/mozilla-aurora/rev/e132143c1df4
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.