890515 - java.lang.IllegalArgumentException: bytes.length 0 must be a positive number at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java) at org.mozilla.(gecko.ThumbnailHelper.setTabThumbnail|widget.TopSitesView.getThumbnailsFromCursor)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

Bug 867058 and bug 878424 are not fully fixed.

Here is a crash report: bp-10307980-7d3a-418b-96b9-818e92130705.

java.lang.IllegalArgumentException: bytes.length 0 must be a positive number
	at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:36)
	at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:31)
	at org.mozilla.gecko.ThumbnailHelper.setTabThumbnail(ThumbnailHelper.java:212)
	at org.mozilla.gecko.ThumbnailHelper.getAndProcessThumbnailFor(ThumbnailHelper.java:71)
	at org.mozilla.gecko.Tabs$4.run(Tabs.java:488)
	at android.os.Handler.handleCallback(Handler.java:725)
	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)

and another one, bp-c745c3f3-2d96-447b-8d03-6ed192130704:

java.lang.IllegalArgumentException: bytes.length 0 must be a positive number
	at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:33)
	at org.mozilla.gecko.gfx.BitmapUtils.decodeByteArray(BitmapUtils.java:28)
	at org.mozilla.gecko.widget.TopSitesView.getThumbnailsFromCursor(TopSitesView.java:319)
	at org.mozilla.gecko.widget.TopSitesView$5.doInBackground(TopSitesView.java:341)
	at org.mozilla.gecko.widget.TopSitesView$5.doInBackground(TopSitesView.java:338)
	at org.mozilla.gecko.util.UiAsyncTask$BackgroundTaskRunnable.run(UiAsyncTask.java:48)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:130)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

Based on the stack trace and the first affected version (22.0), it's likely a regression from bug 863288.

More reports at:
https://crash-stats.mozilla.com/report/list?product=FennecAndroid&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)]

The first stack trace spiked in 25.0a1/20130705. The regression range for the spike is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dcbbfcdf7bb4&tochange=17fe59f6c54a
I suspect bug 803299 in this range.

Comment 2

[Scoobidiver (away)]

It's #4 crasher in 25.0a1 over the last three days.

Comment 3

[Scoobidiver (away)]

It's #2 crasher in 25.0a1.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Purge 0-byte favicons and thumbnails from the database

We keep adding these bytes.length checks as band-aids to fix these crashers, but we always end up finding more, and we add new code that assumes bytes.length won't be 0 (which is a reasonable assumption). Rather than another band-aid fix, this patch purges 0-byte data from the database, adds checks wherever we compress a bitmap that could potentially fail, and also checks the values in the CP as an extra layer of safety.

We also discussed adding a constraint check to the tables for the data column, but I was too lazy to do that since it's not supported as a simple ALTER operation. Let me know if you think it's still worth adding.

I haven't been able to find a real-world favicon that causes this problem on my device, but I manually added a 0-byte array to the database to reproduce the crash. I can verify that this patch fixed the crash.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Purge 0-byte favicons and thumbnails from the database, v2

Ack, that patch was built on top of another WIP patch. Trying again.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 776785 [details] [diff] [review]
Purge 0-byte favicons and thumbnails from the database, v2

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

A few safety concerns. Feel free to ask for re-review if you're not comfortable with treating these as mechanical changes.

::: mobile/android/base/AwesomeBar.java
@@ +646,4 @@
>                  }
>  
>                  Bitmap bitmap = null;
> +                if (b != null) {

Propagate this change forward to Fig?

::: mobile/android/base/ProfileMigrator.java
@@ +892,4 @@
>                  if (image != null) {
>                      Bitmap bitmap = image.getBitmap();
>                      ByteArrayOutputStream stream = new ByteArrayOutputStream();
> +                    if (bitmap.compress(Bitmap.CompressFormat.PNG, 100, stream)) {

This code is about to go away in Bug 886587. You probably just want to get rid of these changes, save a merge conflict!

::: mobile/android/base/db/BrowserProvider.java
@@ +1218,5 @@
> +                data = stream.toByteArray();
> +            } else {
> +                Log.w(LOGTAG, "Favicon compression failed for URL: " + url);
> +            }
> +            iconValues.put(Favicons.DATA, data);

putNull, again.

@@ +1798,5 @@
>  
> +        private void upgradeDatabaseFrom16to17(SQLiteDatabase db) {
> +            // Purge any 0-byte favicons/thumbnails
> +            db.execSQL("DELETE FROM " + TABLE_FAVICONS
> +                    + " WHERE length(" + Favicons.DATA + ") = 0");

Nit: + at end of line, not start.

@@ +1800,5 @@
> +            // Purge any 0-byte favicons/thumbnails
> +            db.execSQL("DELETE FROM " + TABLE_FAVICONS
> +                    + " WHERE length(" + Favicons.DATA + ") = 0");
> +            db.execSQL("DELETE FROM " + TABLE_THUMBNAILS
> +                    + " WHERE length(" + Thumbnails.DATA + ") = 0");

I encourage you to wrap this method in a try..catch, because otherwise sadness could ensue.

@@ +3228,5 @@
> +     * Verifies that 0-byte arrays aren't added as favicon or thumbnail data.
> +     * @param values        ContentValues of query
> +     * @param columnName    Name of data column to verify
> +     */
> +    private void checkImageData(ContentValues values, String columnName) {

This might be better named `stripEmptyByteArray`. You could also name it `stripInvalidBlob`, but see below...

@@ +3231,5 @@
> +     */
> +    private void checkImageData(ContentValues values, String columnName) {
> +        if (values.containsKey(columnName)) {
> +            byte[] data = values.getAsByteArray(columnName);
> +            if (data.length == 0) {

A test:

  ContentValues v = new ContentValues();
  v.put("x", "foo");
  checkImageData(v, "x");
  // => NPE.

getAsByteArray will return null if the value isn't a byte array. You need to check whether it's null, and probably clear it if so:

  if (data == null || data.length == 0) {
    Log.w(LOGTAG, "Tried to insert an empty or non-byte-array image. Ignoring.");
    values.putNull(columnName);
  }

::: mobile/android/base/db/LocalBrowserDB.java
@@ +807,5 @@
> +        ByteArrayOutputStream stream = new ByteArrayOutputStream();
> +        if (favicon.compress(Bitmap.CompressFormat.PNG, 100, stream)) {
> +            data = stream.toByteArray();
> +        } else {
> +            Log.w(LOGTAG, "Favicon compression failed for URL: " + pageUri);

Personally identifiable information... don't log page URIs to the Android log!

@@ +809,5 @@
> +            data = stream.toByteArray();
> +        } else {
> +            Log.w(LOGTAG, "Favicon compression failed for URL: " + pageUri);
> +        }
> +        values.put(Favicons.DATA, data);

That won't work. You need to do

  if (...) {
    values.put(..., data);
  } else {
    values.putNull(...);
  }

@@ +837,5 @@
> +        } else {
> +            Log.w(LOGTAG, "Favicon compression failed for URL: " + uri);
> +        }
> +
> +        final byte[] bytes = stream.toByteArray();

I don't think you want to do that if compression to that stream failed, no?

@@ +846,3 @@
>  
>          ContentValues values = new ContentValues();
> +        values.put(Thumbnails.DATA, data);

Same point about NULL.

Comment 7

[Brian Nicholson (:bnicholson)]

Just some questions before posting a new patch:

> @@ +1798,5 @@
> >  
> > +        private void upgradeDatabaseFrom16to17(SQLiteDatabase db) {
> > +            // Purge any 0-byte favicons/thumbnails
> > +            db.execSQL("DELETE FROM " + TABLE_FAVICONS
> > +                    + " WHERE length(" + Favicons.DATA + ") = 0");
> 
> Nit: + at end of line, not start.

Isn't the convention normally to break before the operator (see section 4.2 of http://www.oracle.com/technetwork/java/codeconventions-150003.pdf). I can change it since I don't have a strong preference, but I'm curious why you prefer breaking after the operator.

> @@ +1800,5 @@
> > +            // Purge any 0-byte favicons/thumbnails
> > +            db.execSQL("DELETE FROM " + TABLE_FAVICONS
> > +                    + " WHERE length(" + Favicons.DATA + ") = 0");
> > +            db.execSQL("DELETE FROM " + TABLE_THUMBNAILS
> > +                    + " WHERE length(" + Thumbnails.DATA + ") = 0");
> 
> I encourage you to wrap this method in a try..catch, because otherwise
> sadness could ensue.

When would we want to continue in the event of a failure? This patch guarantees that 0-byte favicons/thumbnails will no longer exist in the database. If it's a temporary failure, I'd be concerned about continuing because it means the upgrade could complete without purging the bad data, breaking that guarantee. If it's a persistent failure, I think we're in trouble anyway since it probably means the DB is corrupt.

> @@ +809,5 @@
> > +            data = stream.toByteArray();
> > +        } else {
> > +            Log.w(LOGTAG, "Favicon compression failed for URL: " + pageUri);
> > +        }
> > +        values.put(Favicons.DATA, data);
> 
> That won't work.

Can you elaborate? It would suprise me if putting null here won't work here. I assumed putNull() was just a convenience for situations where you want to insert a null directly without casting it to some arbitrary type for put().

Comment 8

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #7)

> Isn't the convention normally to break before the operator (see section 4.2
> of http://www.oracle.com/technetwork/java/codeconventions-150003.pdf). I can
> change it since I don't have a strong preference, but I'm curious why you
> prefer breaking after the operator.

We don't really follow Sun's Java coding conventions :D

Historically I think we've done op-at-end because this:

  let foo = "abcdef " +
            "ghijkl";

is neater and more column-smart than

  let foo = "abcdef "
            + "ghijkl";

and this isn't really done by anyone, probably because it only works for single-character operators:

  let foo = "abcdef "
          + "ghijkl";

Non-column-aligned, of course, is just outright evil, and should be rejected by all civilized peoples. :P


> When would we want to continue in the event of a failure? This patch
> guarantees that 0-byte favicons/thumbnails will no longer exist in the
> database. If it's a temporary failure, I'd be concerned about continuing
> because it means the upgrade could complete without purging the bad data,
> breaking that guarantee. If it's a persistent failure, I think we're in
> trouble anyway since it probably means the DB is corrupt.

My thinking is this:

* This migration code will run for all users, even those without empty favicons.
* If the migration fails for some reason, opening the database will throw, and their browser is useless.
* By contrast, if migration fails and doesn't purge all of the thumbnails, but we catch the exception, the DB will open successfully, and will be no worse than it was before.
* So we're left with affected users (intersection(upgrade failed, had empty favicons)), rather than (upgrade failed). Probably both sets are empty, but the former is strictly better than the latter.

Also, while I'm here: do you want to delete thumbnails with null data? LENGTH(NULL) => NULL, not 0.

 
> Can you elaborate? It would suprise me if putting null here won't work here.
> I assumed putNull() was just a convenience for situations where you want to
> insert a null directly without casting it to some arbitrary type for put().

Discussed this on IRC.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Purge 0-byte favicons and thumbnails from the database, v3

Addressed review comments.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Richard Newman [:rnewman] from comment #6)
> ::: mobile/android/base/AwesomeBar.java
> @@ +646,4 @@
> >                  }
> >  
> >                  Bitmap bitmap = null;
> > +                if (b != null) {
> 
> Propagate this change forward to Fig?

Filed bug 895702.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 778171 [details] [diff] [review]
Purge 0-byte favicons and thumbnails from the database, v3

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

Looks good!

::: mobile/android/base/db/BrowserProvider.java
@@ +3235,5 @@
> +     */
> +    private void stripEmptyByteArray(ContentValues values, String columnName) {
> +        if (values.containsKey(columnName)) {
> +            byte[] data = values.getAsByteArray(columnName);
> +            if (data != null && data.length == 0) {

Should this be || instead?

That is, right now you'll accept strings or numbers instead of byte arrays.

Comment 12

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

ping

Comment 13

[Brian Nicholson (:bnicholson)]

Thanks, got distracted.

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

Comment 14

[Richard Newman [:rnewman]]

Brian: planning to track this for uplift to 24?

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Brian Nicholson (:bnicholson)]

Comment on attachment 778171 [details] [diff] [review]
Purge 0-byte favicons and thumbnails from the database, v3

This is a top crash in 25 because of the 24-bit color changes. It's not nearly as big of a problem in 24, but it would still be nice to have the crash fix.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Caused by bug 863288, but spiked after bug 803299
User impact if declined: Crashes on pages with corrupt thumbnails
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): Low risk, mostly just purges bad data from the database
String or IDL/UUID changes made by this patch: none

Comment 17

[bhavana bajaj [:bajaj]]

Comment on attachment 778171 [details] [diff] [review]
Purge 0-byte favicons and thumbnails from the database, v3

low risk nice to have crash fix.

Comment 18

[:Margaret Leibovic]

Brian, I just ran into some conflicts while merging this into fig, and I'm not 100% confident I did the merge correctly. Can you double check it for me when you get the chance?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/79d58dbe9336

Comment 20

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #18)
> Brian, I just ran into some conflicts while merging this into fig, and I'm
> not 100% confident I did the merge correctly. Can you double check it for me
> when you get the chance?

Looks good to me. I think there's one more check we can remove, which I put up a patch for in bug 895702.