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)
Tracking
(firefox22+ fixed, firefox23+ fixed, firefox24 fixed, fennec22+)
RESOLVED
FIXED
Firefox 24
People
(Reporter: scoobidiver, Assigned: bnicholson)
References
Details
(Keywords: crash, regression, topcrash, Whiteboard: [native-crash][startupcrash])
Crash Data
Attachments
(1 file)
3.87 KB,
patch
|
mfinkle
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•12 years ago
|
status-firefox23:
--- → affected
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Comment 1•12 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•12 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?
Comment 3•12 years ago
|
||
I see one URL on crash-stats : http://www.dmi.dk/dmi/danmark/regionaludsigten/kbhnsj.htm
Reporter | ||
Comment 4•12 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
Comment 5•12 years ago
|
||
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
Comment 6•12 years ago
|
||
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•12 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•12 years ago
|
tracking-firefox23:
--- → +
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #752921 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 9•12 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 10•12 years ago
|
||
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+
Updated•12 years ago
|
tracking-fennec: ? → 22+
Assignee | ||
Comment 11•12 years ago
|
||
status-firefox24:
--- → fixed
Comment 12•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Should we nom this for beta/aurora?
Flags: needinfo?(bnicholson)
Assignee | ||
Comment 14•12 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)
Updated•12 years ago
|
Attachment #752921 -
Flags: approval-mozilla-beta?
Attachment #752921 -
Flags: approval-mozilla-beta+
Attachment #752921 -
Flags: approval-mozilla-aurora?
Attachment #752921 -
Flags: approval-mozilla-aurora+
Comment 15•12 years ago
|
||
Reporter | ||
Updated•12 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)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•