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

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Scoobidiver (away), Assigned: Margaret)

Tracking

({crash})

14 Branch
Firefox 16
ARM
Android
crash
Points:
---

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
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?
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?
(Reporter)

Comment 3

5 years ago
(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...
(Assignee)

Comment 5

5 years ago
(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...
(Assignee)

Comment 6

5 years ago
Created attachment 634555 [details] [diff] [review]
patch with catch()
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+
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f886c5bfd273
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/f886c5bfd273
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

5 years ago
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+
(Assignee)

Comment 13

5 years ago
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.
status-firefox14: --- → fixed
status-firefox15: --- → fixed
You need to log in before you can comment on or make changes to this bug.