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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

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]

Attachment: 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)]

(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)]

(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)]

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]

(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]

Attachment: patch with catch()

Comment 7

[Lucas Rocha (:lucasr)]

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]

(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 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f886c5bfd273

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f886c5bfd273

Comment 11

[:Margaret Leibovic]

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]

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]

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.