929010 - crash in java.lang.NullPointerException: at org.mozilla.gecko.favicons.cache.FaviconsForURL.ensureDominantColor(FaviconsForURL.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

Looks to be a device independent crash. 

More reports at https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A%20at%20org.mozilla.gecko.favicons.cache.FaviconsForURL.ensureDominantColor%28FaviconsForURL.java%29

This bug was filed from the Socorro interface and is 
report bp-ada13e56-1fd2-4ddf-8a8e-7f0262131021.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.favicons.cache.FaviconsForURL.ensureDominantColor(FaviconsForURL.java:141)
	at org.mozilla.gecko.favicons.cache.FaviconCache.getDominantColor(FaviconCache.java:439)
	at org.mozilla.gecko.favicons.Favicons.getFaviconColor(Favicons.java:308)
	at org.mozilla.gecko.home.TopSitesGridItemView.displayFavicon(TopSitesGridItemView.java:189)
	at org.mozilla.gecko.home.TopSitesPage$TopSitesGridAdapter$1.onFaviconLoaded(TopSitesPage.java:577)
	at org.mozilla.gecko.favicons.Favicons$1.run(Favicons.java:78)
	at android.os.Handler.handleCallback(Handler.java:730)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:5303)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:525)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:739)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:555)
	at de.robv.android.xposed.XposedBridge.main(XposedBridge.java:115)
	at dalvik.system.NativeStart.main(Native Method)

Comment 1

[Aaron Train [:aaronmt]]

Sample device selection

From https://crash-analysis.mozilla.com/rkaiser/2013-10-20/2013-10-20.fennecandroid.nightly.devices.weekly.html#sigs
and https://crash-analysis.mozilla.com/rkaiser/2013-10-19/2013-10-19.fennecandroid.nightly.devices.html#sigs

* HTC One	4
* Samsung Galaxy Nexus	3
* Samsung GT-I9300	3
* Samsung SGH-T889	2
* Samsung Nexus 10	2
* Samsung GT-N7100	1
* Samsung GT-I9505G	1
* Samsung SGH-I337	1
* Samsung GT-P3113	1
* Samsung GT-N7100	1
* Sony C6603	1
* Amazon KFTT

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

You don't need to use crash-analysis. That same data is in the 'Signature Summary' tab of the link in comment 0 under 'mobile devices'.

Comment 3

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

#1 crash on Nightly

Comment 4

[:Margaret Leibovic]

Comments say getNextPrimary can return null, but we're not checking for that:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/cache/FaviconsForURL.java#97
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/cache/FaviconsForURL.java#141

I'm not familiar with this code... when do we expect getNextPrimary to return null? In that case would it make sense for mDominantColor to remain unset? Is this just a symptom of some other problem?

Comment 5

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

FaviconsForURL.ensureDominantColor uses FaviconsForURL.getNextPrimary(0), which can return null.

We should check for a null primary and return 0xFFFFFF like we do here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/cache/FaviconCache.java#438

Or make callers of ensureDominantColor try/catch or callers of getNextPrimary properly handle the null return. It looks like ensureDominantColor is the only failed getNextPrimary caller though.

FaviconCache.getFaviconForDimensions is also at risk:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/cache/FaviconCache.java#406

Hardening ensureDominantColor would fix FaviconCache.getFaviconForDimensions

Comment 6

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

Note: This appears to be a crash from a null return for an invalid cache element, see:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/cache/FaviconsForURL.java#124

Because when I look at the logcat of the crash I do not see:
"No primaries found in Favicon cache structure. This is madness!"

Comment 7

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

Attachment: simple fix v1

Simply checks for null and returns a color we seem to use in other "can't find color" situations.

Comment 8

[Chris Kitching [:ckitching]]

Attachment: faviconColours.patch

Sorry for not responding to this sooner.

Some background:

A "primay" is an untouched image that has been served by a page. A "secondary" is a primary that has been scaled for our purposes. For instance, if a page serves up a 32x32px favicon, then some code requests an 16x16px one, the system will downscale the 32px primary to produce the 16px result, caching that as a "secondary".

The purpose of making the distinction is to ensure we don't ever scale an already-scaled but cached favicon to satisfy a request - that would harm quality.

A page can potentially serve multiple primaries - an ICO file may contain several sizes, each of which will get entered into the cache as a primary. We need to not forget the existence of a particular primary for a particular favicon URL, or we may later select a less-appropriate primary to scale for a particular request.
Also, we want to exhibit proper least-recently-used behaviour, so we can't just keep all the primaries in memory all the time.

So, the system that culls infrequently used things from the cache doesn't treat primaries/secondaries any different when it comes to choosing them for elimination, to ensure good cache behaviour.
But, in order to not forget about primaries, when a primary is culled from the cache we set the invalided flag on it, delete its payload (Freeing up the memory used to hold it), but keep the container object around. That way, when a request comes in that would be best satisfied by that primary it is still returned. The effect of this is that, when such a request is handled, it knows that to satisfy itself it needs to fetch the icon from the database/network again - in order to find this missing primary.

Unfortunately, I didn't think about how this works with dominant colour caching - the intent is to cache the dominant colour once for each favicon URL (Assuming it's not really going to be meaningfully different for the different images. Not always certainly strictly true, but close enough).

Finkle's approach will fix the crash, but will lead to favicons not getting the dominant colour in some slightly obscure cases when it really should be possible. The best approach, I think, is to iterate mFavicons looking for any entry (primary or secondary) that does not have the invalidated flag set, and use that. Ideally we want to use a primary for the dominant colour calculation, but it doesn't really matter very much. There's a performance argument for trying to select the very smallest image for this purpose.

So, here's an (untested) patch that does that. Perhaps worth consideration.

Comment 9

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

Comment on attachment 820955 [details] [diff] [review]
faviconColours.patch

Let's go with this

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/27d18354871e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27d18354871e