987340 - crash in java.lang.NullPointerException: at org.mozilla.gecko.favicons.decoders.LoadFaviconResult.getBytesForDatabaseStorage(LoadFaviconResult.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-1efb1e8f-d385-4b47-9ee2-183322140323.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.favicons.decoders.LoadFaviconResult.getBytesForDatabaseStorage(LoadFaviconResult.java:57)
	at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground$2d4c763b(LoadFaviconTask.java:379)
	at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground$42af7916(LoadFaviconTask.java:42)
	at org.mozilla.gecko.util.UiAsyncTask$BackgroundTaskRunnable.run(UiAsyncTask.java:48)
	at android.os.Handler.handleCallback(Handler.java:615)
	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)

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Prevent favicon decoder from exploding when faced with corrupt bitmaps.

Looks like `favicon` is null. That seems to suggest the bitmapsDecoded iterator just contains a null.

That's supposed to be impossible - a failure to load a favicon should return a null LoadFaviconResult, not a LoadFaviconResult with one entry (null), which seems to be what we have here.

The only place we construct SingleBitmapIterators is here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/decoders/FaviconDecoder.java#98

We blindly encapsulate the result of BitmapUtils.decodeByteArray in the SingleBitmapIterator. That'll result in a null if the icon is corrupt (or decodeByteArray otherwise fails to do its thing).

So, it looks like this happens whenever you encounter a favicon that is not an ICO, that does have the correct magic numbers for a non-ICO image format we can decode, but that, for some reason, fails to decode in BitmapUtils.decodeByteArray (most likely due to it being corrupt).
Funkah!


Seems a pretty straightforward fix - check if the decode worked and, if not, abort.
I feel slightly bad dumping all the favicon-oriented reviews on rnewman... Are there any alternative victims for this sort of thing?

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8395924 [details] [diff] [review]
Prevent favicon decoder from exploding when faced with corrupt bitmaps.

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

::: mobile/android/base/favicons/decoders/FaviconDecoder.java
@@ +94,5 @@
>              result.isICO = false;
>  
> +            Bitmap decodedImage = BitmapUtils.decodeByteArray(buffer, offset, length);
> +            if (decodedImage == null) {
> +                // What we got wasn't decodable after all. Probably corrupt.

Corrupt, or we got a muffled OOM.

Comment 3

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/8704a02247d8

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8704a02247d8