Closed Bug 867058 Opened 12 years ago Closed 12 years ago

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)

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

22 Branch
ARM
Android
defect
Not set
critical

Tracking

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

RESOLVED FIXED
Firefox 24
Tracking Status
firefox22 + fixed
firefox23 + fixed
firefox24 --- fixed
fennec 22+ ---

People

(Reporter: scoobidiver, Assigned: bnicholson)

References

Details

(Keywords: crash, regression, topcrash, Whiteboard: [native-crash][startupcrash])

Crash Data

Attachments

(1 file)

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
Keywords: regression
Version: Trunk → Firefox 22
It's currently #1 top crasher in 22.0b1 with many duplicates.
tracking-fennec: --- → ?
Keywords: topcrash
Whiteboard: [native-crash] → [native-crash][startupcrash]
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?
(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
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.
Attachment #752921 - Flags: review?(mark.finkle)
(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+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Should we nom this for beta/aurora?
Flags: needinfo?(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
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+
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)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: