1009311 - enforce 18-character limit to AndroidLog tag length

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: truncate tags longer than 18 characters

In bug 1004825, comment 11, rnewman writes:
> Note that the tag length should be no more than 23 characters (a limit which
> only isLoggable enforces via an exception, but hey, Android!), which after
> prepending "Gecko" means that AndroidLog.jsm should enforce an 18-char limit.
> 
> Worth a 3-line follow-up to either truncate or err, I think.

Here's a patch that does the former.  But is this really necessary?  I know isLoggable throws on longer tags, but this code doesn't access Android.util.log, it uses the native API, which says nothing about a tag length limit.

https://android.googlesource.com/platform/system/core/+/master/include/android/log.h

And WebappManagerWorker.js uses the 19-character tag "WebappManagerWorker" without any apparent problems:

D/GeckoWebappManagerWorker( 4777): onprogress: received 38889 bytes

So I wonder if this really matters, or perhaps the native limit is different.

Comment 1

[Richard Newman [:rnewman]]

I imagine things won't catch fire if this bug goes unfixed.

There's at least one good reason, though: I've seen in the past where someone has added an isLoggable check (our own Logger class that wraps Log does so, for example), and things have started failing because the log tag was too long. So you'd have to fix this bug this prior to adding loggability checks, at least.

Beyond that, it's a question of: why does Android impose a limit? And will behavior change in the future? Is it worth having really long log tags?

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8421416 [details] [diff] [review]
truncate tags longer than 18 characters

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

::: mobile/android/base/tests/testAndroidLog.js
@@ +32,5 @@
>    do_check_eq(47, AndroidLog.e.bind(null, "AndroidLogTest")("This is an error message."));
>  
> +  // Ensure the functions work when the tag length is greater than the maximum
> +  // tag length.
> +  let tag = ["X" for (i of Array(AndroidLog.MAX_TAG_LENGTH))].join("") + "X";

MAX_TAG_LENGTH + 1 rather than + "X"?

::: mobile/android/modules/AndroidLog.jsm
@@ +45,5 @@
>  const ANDROID_LOG_INFO = 4;
>  const ANDROID_LOG_WARN = 5;
>  const ANDROID_LOG_ERROR = 6;
>  
> +const MAX_TAG_LENGTH = 18;

Perhaps a brief explanation for the poor suckers that come after us?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: truncate tags longer than 18 characters

(In reply to Richard Newman [:rnewman] from comment #2)
> > +  let tag = ["X" for (i of Array(AndroidLog.MAX_TAG_LENGTH))].join("") + "X";
> 
> MAX_TAG_LENGTH + 1 rather than + "X"?

Indeed!

I originally implemented the repetitive concatenation via Array(n).join("X"), where "n" has to be one greater than the length of the output string, because that method uses the join separator rather than the array items to generate the string, and there is one fewer separator than there are items.

So I would have needed to pass MAX_TAG_LENGTH + 2 to make the tag too long.  And I figured passing MAX_TAG_LENGTH + 1 and then adding another "X" was marginally more readable.

But "n" can be the actual length when generating the string via an array comprehension, in which case specifying MAX_TAG_LENGTH + 1 makes more sense!

As it does with String.repeat, which I just noticed and to which I have switched!

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/repeat

Simple and sweet:

  let tag = "X".repeat(AndroidLog.MAX_TAG_LENGTH + 1);

https://github.com/mykmelez/gecko-dev/commit/73269e4ea51165ae634297cfc61cd46bb56f3fa9


> ::: mobile/android/modules/AndroidLog.jsm
> > +const MAX_TAG_LENGTH = 18;
> 
> Perhaps a brief explanation for the poor suckers that come after us?

You bet!  I wrote this:

// android.util.Log.isLoggable throws IllegalArgumentException if a tag length
// exceeds 23 characters, and we prepend five characters ("Gecko") to every tag,
// so we truncate tags exceeding 18 characters (although __android_log_write
// itself and other android.util.Log methods don't seem to mind longer tags).

https://github.com/mykmelez/gecko-dev/commit/1ca0c818d261bae1805e470c6c0756b885c488a1


Finally, I made the tests compute the expected lengths of the logged strings relative to MAX_TAG_LENGTH, so they don't start failing if we change its value.

https://github.com/mykmelez/gecko-dev/commit/ddf3c8b1aa08103c495f51b8bf7abfba29d3bbab


And I reran my primitive performance test, which averaged a respectable 127ms across ten runs (logging 1000 messages each run).

Here's the patch I'll land.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/feeb736171db

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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