926497 - NPE in FaviconCache while scrolling the bookmarks list

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

Just ran into this after updating to today's Nightly.

10-14 09:12:59.405 28982 29356 E FaviconCache: FaviconCache exception!
10-14 09:12:59.405 28982 29356 E FaviconCache: java.lang.NullPointerException
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at java.util.concurrent.ConcurrentHashMap.containsKey(ConcurrentHashMap.java:911)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at org.mozilla.gecko.favicons.cache.FaviconCache.isFailedFavicon(FaviconCache.java:215)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at org.mozilla.gecko.favicons.Favicons.isFailedFavicon(Favicons.java:368)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground(LoadFaviconTask.java:243)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground(LoadFaviconTask.java:43)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at org.mozilla.gecko.util.UiAsyncTask$BackgroundTaskRunnable.run(UiAsyncTask.java:48)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at android.os.Handler.handleCallback(Handler.java:725)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at android.os.Handler.dispatchMessage(Handler.java:92)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at android.os.Looper.loop(Looper.java:137)
10-14 09:12:59.405 28982 29356 E FaviconCache: 	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)
10-14 09:12:59.407 28982 29356 W dalvikvm: threadid=10: thread exiting with uncaught exception (group=0x41a28ac8)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 13823 ("GeckoBackgroundThread")
10-14 09:12:59.413 28982 29356 E GeckoAppShell: java.lang.IllegalArgumentException: the bind value at index 1 is null
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.database.sqlite.SQLiteProgram.bindString(SQLiteProgram.java:164)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.database.sqlite.SQLiteProgram.bindAllArgsAsStrings(SQLiteProgram.java:200)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:47)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1314)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:400)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:333)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:2687)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.content.ContentProvider.query(ContentProvider.java:652)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.content.ContentProvider$Transport.query(ContentProvider.java:189)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.content.ContentResolver.query(ContentResolver.java:372)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.content.ContentResolver.query(ContentResolver.java:315)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at org.mozilla.gecko.db.LocalBrowserDB.getFaviconForUrl(LocalBrowserDB.java:712)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at org.mozilla.gecko.db.BrowserDB.getFaviconForFaviconUrl(BrowserDB.java:242)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at org.mozilla.gecko.favicons.LoadFaviconTask.loadFaviconFromDb(LoadFaviconTask.java:93)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground(LoadFaviconTask.java:275)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground(LoadFaviconTask.java:43)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at org.mozilla.gecko.util.UiAsyncTask$BackgroundTaskRunnable.run(UiAsyncTask.java:48)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.os.Handler.handleCallback(Handler.java:725)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.os.Handler.dispatchMessage(Handler.java:92)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at android.os.Looper.loop(Looper.java:137)
10-14 09:12:59.413 28982 29356 E GeckoAppShell: 	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

Comment 1

[:Margaret Leibovic]

Actually, maybe the NPE isn't the real problem, since it looks caught. But this does crash my browser consistently whenever I try to scroll down my bookmarks list.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: exceptionMadness.patch

All of this madness looks to be caused by a null favicon URL being used in LoadFaviconTask. The null value there causes the NPE in the cache, which is handled, but should return true instead of false.
Since the exceptional state leads to it returning true, the LoadFaviconTask continues and attempts to load this null URL from the database, finally resulting in the crash (When the SQL driver is upset by this).

What's odd is that this nullity should never have occured in the first place. It appears the only way mFaviconUrl in LoadFaviconTask can be null when we reach line 243 (The call to isFailedFavicon) is if guessDefaultFaviconURL returned null, which itself only happens in the case of a URISyntaxException.

For science, see what happens with this patch - it adds printing to the URISyntaxException and makes the error handler in FaviconCache fail in a way that should prevent the crash.

Ideally, we'd like to also stop the UISyntaxException from happening, but with this patch it's *probably* the case that things will cease to crash.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 816667 [details] [diff] [review]
exceptionMadness.patch

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

::: mobile/android/base/favicons/Favicons.java
@@ +355,1 @@
>              return null;

Given that this explicitly has a null return, I'd like for the caller -- LoadFaviconTask -- to check for null at line 243.

::: mobile/android/base/favicons/cache/FaviconCache.java
@@ +240,5 @@
>  
>              // Flag to prevent finally from doubly-unlocking.
>              isAborting = true;
>              Log.e(LOGTAG, "FaviconCache exception!", unhandled);
> +            return true;

Why not also short-circuit at the top of this method? Don't make the reader (and the program!) go through this exception dance to figure out what isFailedFavicon(null) means. Document it in the Javadocs and an early return.

Comment 4

[Chris Kitching [:ckitching]]

Attachment: V2 - Deal more gracefully with URIs that don't parse.

Howzat?

Comment 5

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/3f69e71fc0cf

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f69e71fc0cf

Comment 7

[:Margaret Leibovic]

Fixed my crash. Thanks!