Closed Bug 926497 Opened 6 years ago Closed 6 years ago

NPE in FaviconCache while scrolling the bookmarks list

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 27

People

(Reporter: Margaret, Assigned: ckitching)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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)
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.
Attached patch exceptionMadness.patch (obsolete) — Splinter Review
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.
Assignee: nobody → chriskitching
Status: NEW → ASSIGNED
Attachment #816667 - Flags: review?(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.
Attachment #816667 - Flags: review?(rnewman)
Howzat?
Attachment #816667 - Attachment is obsolete: true
Attachment #816684 - Flags: review?(rnewman)
Attachment #816684 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/integration/fx-team/rev/3f69e71fc0cf
Whiteboard: [fixed in fx-team][qa-]
Target Milestone: --- → Firefox 27
https://hg.mozilla.org/mozilla-central/rev/3f69e71fc0cf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team][qa-] → [qa-]
Fixed my crash. Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.