1061936 - Excess logging in BrowserApp/GeckoApp

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

BrowserApp and GeckoApp currently spew a bunch of info on (for instance) every tab event they get. We don't need it.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch 1

This rips out the things I didn't think we needed and logs exceptions in a few places they were ignored. I played a bit with a really really dumb log wrapper we could use. I'll toss it up, but curious what you think rnewman? I looked at Logger, and it seems... way more complex that what I wanted here (a simple way to have log statements that weren't recorded by default).

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

This is the simple logger. It basically just blocks/shows log messages based on a single Gecko tag. Classes that opted in would be stuck with that :) We could store something per-tag (or per a few narrowly defined tags? Gecko, DB, Toolbar, Settings, etc?)

Not sure this is worth doing....

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8483023 [details] [diff] [review]
Patch 1

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

::: mobile/android/base/GeckoApp.java
@@ -1200,5 @@
>          // the UI.
>          // This is using a sledgehammer to crack a nut, but it'll do for
>          // now.
>          if (BrowserLocaleManager.getInstance().systemLocaleDidChange()) {
> -            Log.i(LOGTAG, "System locale changed. Restarting.");

This is very rare, but something that's really vital to forensics. Leave it in?

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8483025 [details] [diff] [review]
Patch

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

Generally in favor, but bear in mind: the reason we want to cache those log level values is that they're expensive, performing file IO on every call... but this implementation fires off *five* such checks during static initialization, which will be incurred by its first use from GeckoApp. Hidden cost.

This might even be enough to affect startup time, and the implicit log level check might trigger a StrictMode warning. Please verify this before landing!

If you want to see the approach we take for background services -- which is much more sophisticated (per-thread meta tags, able to log to files, able to be reconfigured on the fly), but also much more complicated -- take a look at Logger and AndroidLevelCachingLogWriter.

::: mobile/android/base/util/Log.java
@@ +1,1 @@
> +package org.mozilla.gecko.util;

License.

@@ +1,3 @@
> +package org.mozilla.gecko.util;
> +
> +import org.mozilla.gecko.AppConstants.Versions;

Unused import.

@@ +2,5 @@
> +
> +import org.mozilla.gecko.AppConstants.Versions;
> +
> +public class Log {
> +    private static boolean logInfo    = android.util.Log.isLoggable("Gecko", android.util.Log.INFO);

Extract constant.

N.B., isLoggable can throw if the log tag is longer than 23 characters. Not relevant here, but file it away in your brain.

Comment 5

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/16d4b2727467

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16d4b2727467