Closed Bug 765486 Opened 9 years ago Closed 9 years ago

java.lang.NullPointerException: at org.mozilla.gecko.db.LocalBrowserDB.isBookmark(LocalBrowserDB.java)

Categories

(Firefox for Android Graveyard :: General, defect)

14 Branch
ARM
Android
defect
Not set
critical

Tracking

(firefox14 fixed, firefox15 fixed)

RESOLVED FIXED
Firefox 16
Tracking Status
firefox14 --- fixed
firefox15 --- fixed

People

(Reporter: scoobidiver, Assigned: Margaret)

Details

(Keywords: crash, Whiteboard: [native-crash][startupcrash])

Crash Data

Attachments

(1 file, 1 obsolete file)

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
Attached patch patch (obsolete) — Splinter Review
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?
Assignee: nobody → margaret.leibovic
Attachment #634048 - Flags: review?(lucasr.at.mozilla)
(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?
(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 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...
(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...
Attachment #634048 - Attachment is obsolete: true
Attachment #634048 - Flags: review?(lucasr.at.mozilla)
Attachment #634555 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #634555 - Flags: review?(lucasr.at.mozilla) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/f886c5bfd273
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
Attachment #634555 - Flags: approval-mozilla-beta?
Attachment #634555 - Flags: approval-mozilla-aurora?
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).
Attachment #634555 - Flags: approval-mozilla-beta?
Attachment #634555 - Flags: approval-mozilla-beta+
Attachment #634555 - Flags: approval-mozilla-aurora?
Attachment #634555 - Flags: approval-mozilla-aurora+
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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.