Closed
Bug 765486
Opened 13 years ago
Closed 13 years ago
java.lang.NullPointerException: at org.mozilla.gecko.db.LocalBrowserDB.isBookmark(LocalBrowserDB.java)
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox14 fixed, firefox15 fixed)
RESOLVED
FIXED
Firefox 16
People
(Reporter: scoobidiver, Assigned: Margaret)
Details
(Keywords: crash, Whiteboard: [native-crash][startupcrash])
Crash Data
Attachments
(1 file, 1 obsolete file)
2.24 KB,
patch
|
lucasr
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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)
Comment 2•13 years ago
|
||
(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•13 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 4•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #634048 -
Attachment is obsolete: true
Attachment #634048 -
Flags: review?(lucasr.at.mozilla)
Attachment #634555 -
Flags: review?(lucasr.at.mozilla)
Comment 7•13 years ago
|
||
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•13 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•13 years ago
|
||
Target Milestone: --- → Firefox 16
Comment 10•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•13 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 12•13 years ago
|
||
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•13 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
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•