867058 - java.lang.IllegalArgumentException: bytes.length 0 must be a positive number at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java) at org.mozilla.gecko.AllPagesTab.storeFaviconsInMemCache(AllPagesTab.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Scoobidiver (away)]

There's one crash in 23.0a1/20130429: bp-9c1d579a-f996-4e15-9ecc-4c5042130429.

java.lang.IllegalArgumentException: bytes.length 0 must be a positive number
	at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:25)
	at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:20)
	at org.mozilla.gecko.AllPagesTab.storeFaviconsInMemCache(AllPagesTab.java:900)
	at org.mozilla.gecko.AllPagesTab$10.doInBackground(AllPagesTab.java:922)
	at org.mozilla.gecko.AllPagesTab$10.doInBackground(AllPagesTab.java:918)
	at org.mozilla.gecko.util.UiAsyncTask$BackgroundTaskRunnable.run(UiAsyncTask.java:48)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.IllegalArgumentException%3A+bytes.length+0+must+be+a+positive+number+at+org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray%28BitmapUtils.java%29

Comment 1

[Scoobidiver (away)]

It's currently #1 top crasher in 22.0b1 with many duplicates.

Comment 2

[Alex Keybl [:akeybl]]

Lots of dupes with 20 crashes - let's see if this persists. If so, it would be great to check for device correlations.

mfinkle, who might be able to perform a code inspection?

Comment 3

[bhavana bajaj [:bajaj]]

I see one URL on crash-stats : http://www.dmi.dk/dmi/danmark/regionaludsigten/kbhnsj.htm

Comment 4

[Scoobidiver (away)]

(In reply to Alex Keybl [:akeybl] from comment #2)
> Lots of dupes with 20 crashes - let's see if this persists. If so, it would
> be great to check for device correlations.
It persists indeed. It occurs from Froyo to JB 4.2 on any kind of devices:
Asus Nexus 7 	38
Samsung Nexus 10 	13
Samsung SCH-I535 	11
Acer A500 	11
Samsung GT-I9100 	10
Samsung GT-P3110 	9
ASUS Transformer Pad TF300T 	8
Samsung SC-06D 	6
Samsung GT-P7510 	4
NEC LT-NA7 	4
Samsung GT-P1000 	4
...

For Aurora, it first showed up in 22.0a2/20130428 and happens one build out of four in the worst case. So the Aurora regression is:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=d7352cdf5e91&tochange=bf17ffb21f5e
It's likely a regression from bug 863288.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Some notes:
* Fx22 (on Beta) seems to be to greatest number of recent crashes
* The exception is thrown on purpose, if we find a Bytes array of zero length:
  http://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/gfx/BitmapUtils.java#33
* The crash reports I looked at seem to be caused by bogus favicons, saved in our database:
  http://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/awesomebar/AllPagesTab.java#834

A simple fix for this is to make the null check a bit more robust here:
http://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/awesomebar/AllPagesTab.java#835

if (b != null && b.length > 0)

A different approach might be to scan the DB and clean out bogus favicons.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I just wanted to check on how we save favicons to the DB. We appear to check for "empty" favicons:
http://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/Favicons.java#318

But we could be more robust by doing some checks deeper. We just save the Byte array without looking first:
http://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/db/LocalBrowserDB.java#768

Comment 7

[Brian Nicholson (:bnicholson)]

Note that we had width/height checks before, but they were removed in bug 863288:

diff --git a/mobile/android/base/awesomebar/AwesomeBarTab.java b/mobile/android/base/awesomebar/AwesomeBarTab.java
--- a/mobile/android/base/awesomebar/AwesomeBarTab.java
+++ b/mobile/android/base/awesomebar/AwesomeBarTab.java
-            Bitmap bitmap = BitmapFactory.decodeByteArray(b, 0, b.length);
-            if (bitmap != null && bitmap.getWidth() > 0 && bitmap.getHeight() > 0) {
+            Bitmap bitmap = BitmapUtils.decodeByteArray(b);
+            if (bitmap != null) {

diff --git a/mobile/android/base/awesomebar/HistoryTab.java b/mobile/android/base/awesomebar/HistoryTab.java
--- a/mobile/android/base/awesomebar/HistoryTab.java
+++ b/mobile/android/base/awesomebar/HistoryTab.java
-                Bitmap bitmap = BitmapFactory.decodeByteArray(b, 0, b.length);
-                if (bitmap != null && bitmap.getWidth() > 0 && bitmap.getHeight() > 0) {
+                Bitmap bitmap = BitmapUtils.decodeByteArray(b);
+                if (bitmap != null) {

etc.

Looks like these checks were also guarding against bytes.length == 0 (which would make sense, since a 0x0 image should have no bytes). We still check for 0 width or height in decodeByteArray(), but we do it after the bytes.length check which is too late.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Check bytes.length before decoding favicons

Comment 9

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Looks like these checks were also guarding against bytes.length == 0 (which
> would make sense, since a 0x0 image should have no bytes). We still check
> for 0 width or height in decodeByteArray(), but we do it after the
> bytes.length check which is too late.

Actually, I don't think we were guarding against bytes.length == 0 at all. Before, we directly called BitmapFactory.decodeByteArray() in Android, which doesn't throw an exception for a zero-length array. This gave us back a 0x0 bitmap, which is why we needed getWidth()/getHeight() checks on the result.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 752921 [details] [diff] [review]
Check bytes.length before decoding favicons

Note to self: These are the place where we could be getting a bogus favicon.

Comment 11

[Brian Nicholson (:bnicholson)]

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

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Should we nom this for beta/aurora?

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 752921 [details] [diff] [review]
Check bytes.length before decoding favicons

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 863288
User impact if declined: crashes with 0-size favicons
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low risk
String or IDL/UUID changes made by this patch: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d13ef195810b
https://hg.mozilla.org/releases/mozilla-beta/rev/ae657be3a4f1