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)

RESOLVED FIXED in Firefox 22

Status

()

--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: scoobidiver, Assigned: bnicholson)

Tracking

({crash, regression, topcrash})

22 Branch
Firefox 24
ARM
Android
crash, regression, topcrash
Points:
---

Firefox Tracking Flags

(firefox22+ fixed, firefox23+ fixed, firefox24 fixed, fennec22+)

Details

(Whiteboard: [native-crash][startupcrash], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
status-firefox23: --- → affected
(Reporter)

Updated

6 years ago
status-firefox22: --- → affected
Keywords: regression
Version: Trunk → Firefox 22
(Reporter)

Comment 1

6 years ago
It's currently #1 top crasher in 22.0b1 with many duplicates.
tracking-fennec: --- → ?
tracking-firefox22: --- → ?
Keywords: topcrash
Whiteboard: [native-crash] → [native-crash][startupcrash]

Comment 2

6 years ago
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?
I see one URL on crash-stats : http://www.dmi.dk/dmi/danmark/regionaludsigten/kbhnsj.htm
(Reporter)

Comment 4

6 years ago
(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.
Blocks: 863288
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.
Assignee: nobody → bnicholson
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
(Assignee)

Comment 7

6 years ago
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.

Updated

6 years ago
tracking-firefox22: ? → +
tracking-firefox23: --- → +
(Assignee)

Comment 8

6 years ago
Created attachment 752921 [details] [diff] [review]
Check bytes.length before decoding favicons
Attachment #752921 - Flags: review?(mark.finkle)
(Assignee)

Comment 9

6 years ago
(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 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.
Attachment #752921 - Flags: review?(mark.finkle) → review+
tracking-fennec: ? → 22+
https://hg.mozilla.org/mozilla-central/rev/b86033ece2e4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Should we nom this for beta/aurora?
Flags: needinfo?(bnicholson)
(Assignee)

Comment 14

6 years ago
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
Attachment #752921 - Flags: approval-mozilla-beta?
Attachment #752921 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bnicholson)
Attachment #752921 - Flags: approval-mozilla-beta?
Attachment #752921 - Flags: approval-mozilla-beta+
Attachment #752921 - Flags: approval-mozilla-aurora?
Attachment #752921 - Flags: approval-mozilla-aurora+
(Reporter)

Updated

5 years ago
Summary: java.lang.IllegalArgumentException: bytes.length 0 must be a positive number at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java) → 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)
You need to log in before you can comment on or make changes to this bug.