929025 - crash in java.lang.NullPointerException: at org.mozilla.gecko.favicons.cache.FaviconCache.cullIfRequired(FaviconCache.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-cc9f957d-eeeb-4fe4-963d-db82c2131021.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.favicons.cache.FaviconCache.cullIfRequired(FaviconCache.java:617)
	at org.mozilla.gecko.favicons.cache.FaviconCache.putSingleFavicon(FaviconCache.java:537)
	at org.mozilla.gecko.favicons.Favicons.putFaviconInMemCache(Favicons.java:250)
	at org.mozilla.gecko.favicons.LoadFaviconTask.onPostExecute(LoadFaviconTask.java:320)

* Samsung Galaxy Nexus	2
* LGE Nexus 4

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Reset cache fullness counter when evictAll is called.

This appears to be caused by the favicon cache trying to remove elements to reclaim space when there are no elements to remove. (ie. mOrdering is empty).

It also appears that a call to evictAll doesn't lead to mCurrentSize being reset - this would lead to the next cull when full attempting to remove nonexistent elements to free up space that is already freed, resulting in this crash.

This crash might also be caused by another, undetected problem causesing mCurrentSize to become inconsistent with the actual cache contents.

Still, the evictAll problem seems like a likely candidate for causing this problem. Have a patch.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 820191 [details] [diff] [review]
Reset cache fullness counter when evictAll is called.

Do we not need to take a lock here? (Reviewing from phone, else I'd pull up the source.)

Comment 3

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #2)
> Comment on attachment 820191 [details] [diff] [review]
> Reset cache fullness counter when evictAll is called.
> 
> Do we not need to take a lock here? (Reviewing from phone, else I'd pull up
> the source.)

Yes - the appropriate lock was already taken in earlier code. The patch adds.an extra line of work while the lock is held.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 820191 [details] [diff] [review]
Reset cache fullness counter when evictAll is called.

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

Well, at least we know our eviction code is being called!

Comment 5

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #4)
> Well, at least we know our eviction code is being called!

That, or my hypothesis is incorrect. :P

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/fx-team/rev/59608352c11e

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/59608352c11e