Last Comment Bug 765486 - java.lang.NullPointerException: at org.mozilla.gecko.db.LocalBrowserDB.isBookmark(LocalBrowserDB.java)
: java.lang.NullPointerException: at org.mozilla.gecko.db.LocalBrowserDB.isBook...
Status: RESOLVED FIXED
[native-crash][startupcrash]
: crash
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 14 Branch
: ARM Android
: -- critical (vote)
: Firefox 16
Assigned To: :Margaret Leibovic
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-16 05:30 PDT by Scoobidiver (away)
Modified: 2012-06-22 17:34 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
patch (2.12 KB, patch)
2012-06-18 08:20 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch with catch() (2.24 KB, patch)
2012-06-19 12:53 PDT, :Margaret Leibovic
lucasr.at.mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-06-16 05:30:03 PDT
There are 17 crashes over the last 4 weeks across all versions, including  bp-59ba21ee-4237-42dd-ab5b-4102c2120615.

java.lang.NullPointerException
	at org.mozilla.gecko.db.LocalBrowserDB.isBookmark(LocalBrowserDB.java:388)
	at org.mozilla.gecko.db.BrowserDB.isBookmark(BrowserDB.java:134)
	at org.mozilla.gecko.Tab$4.run(Tab.java:353)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at org.mozilla.gecko.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+org.mozilla.gecko.db.LocalBrowserDB.isBookmark%28LocalBrowserDB.java%29
Comment 1 :Margaret Leibovic 2012-06-18 08:20:11 PDT
Created attachment 634048 [details] [diff] [review]
patch

This patch adds a try block around the query call, similar to how we do it in desktopBookmarksExist/readingListItemsExist.

However, I'm curious as to when the query call is returning null. Maybe it's because a bad uri can cause a malformed query? I just want to make sure we're not going to be eating a valid exception. Perhaps we should add some logging in here to report when we come across the problem. Also, Lucas, do you remember why we added these try blocks around the queries in those other methods?
Comment 2 Lucas Rocha (:lucasr) 2012-06-19 01:19:38 PDT
(In reply to Margaret Leibovic [:margaret] from comment #1)
> Created attachment 634048 [details] [diff] [review]
> patch
> 
> This patch adds a try block around the query call, similar to how we do it
> in desktopBookmarksExist/readingListItemsExist.
> 
> However, I'm curious as to when the query call is returning null. Maybe it's
> because a bad uri can cause a malformed query? I just want to make sure
> we're not going to be eating a valid exception. Perhaps we should add some
> logging in here to report when we come across the problem. Also, Lucas, do
> you remember why we added these try blocks around the queries in those other
> methods?

I wonder if this is caused by a bad profile name being sent (the query arg) to the provider? There has been some changes to how we pick the profile to use recently. Maybe this is regression? Mark, Wes, any idea?
Comment 3 Scoobidiver (away) 2012-06-19 01:39:04 PDT
(In reply to Lucas Rocha (:lucasr) from comment #2)
> Maybe this is regression?
I don't think so as it happens in 14.0 Beta and some trunk builds.
Comment 4 Lucas Rocha (:lucasr) 2012-06-19 07:42:38 PDT
Comment on attachment 634048 [details] [diff] [review]
patch

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

Is this patch actually protecting against NPE? It doesn't catch any exceptions...
Comment 5 :Margaret Leibovic 2012-06-19 12:41:43 PDT
(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 634048 [details] [diff] [review]
> patch
> 
> Review of attachment 634048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this patch actually protecting against NPE? It doesn't catch any
> exceptions...

Er, whoops, this patch is wrong. I was confused by the try/finally blocks in the previous methods. Thinking about them now, it seems like they don't actually serve any purpose...
Comment 6 :Margaret Leibovic 2012-06-19 12:53:49 PDT
Created attachment 634555 [details] [diff] [review]
patch with catch()
Comment 7 Lucas Rocha (:lucasr) 2012-06-20 03:04:40 PDT
Comment on attachment 634555 [details] [diff] [review]
patch with catch()

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +389,5 @@
> +                                Bookmarks.URL);
> +            count = c.getCount();
> +            c.close();
> +        } catch (NullPointerException e) {
> +            Log.e(LOGTAG, "NullPointerException in isBookmark for " + uri);

My main concern with this patch is that we might be hiding a bug somewhere else. Hopefully we'll find out what's actually going on using this log message. I assume you haven't been able to reproduce this bug?
Comment 8 :Margaret Leibovic 2012-06-20 18:08:57 PDT
(In reply to Lucas Rocha (:lucasr) from comment #7)

> My main concern with this patch is that we might be hiding a bug somewhere
> else. Hopefully we'll find out what's actually going on using this log
> message. I assume you haven't been able to reproduce this bug?

No, I haven't been able to. I agree that I want to know what is really causing this, but in the meantime I think it's better to log the exception than to crash, especially if this is something that's been around for a long time and will probably be hard to debug.
Comment 10 Ed Morley [:emorley] 2012-06-22 03:47:27 PDT
https://hg.mozilla.org/mozilla-central/rev/f886c5bfd273
Comment 11 :Margaret Leibovic 2012-06-22 10:02:34 PDT
Comment on attachment 634555 [details] [diff] [review]
patch with catch()

Requesting approval for aurora and beta, since this crash has been showing up in all versions.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: crashes can happen, although we're uncertain how to reproduce
Testing completed (on m-c, etc.): landed on m-c 6/22
Risk to taking this patch (and alternatives if risky): low-risk, adds a null check to report an error instead of crash
String or UUID changes made by this patch: n/a
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 15:45:21 PDT
Comment on attachment 634555 [details] [diff] [review]
patch with catch()

low risk null check, go ahead and land to tip of beta (as well as aurora).
Comment 13 :Margaret Leibovic 2012-06-22 17:34:37 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/43e03ed8caef
https://hg.mozilla.org/releases/mozilla-beta/rev/88a24b966765

I had to resolve conflicts when I landed these, so I'll watch the trees to make sure there aren't problems.

Note You need to log in before you can comment on or make changes to this bug.