Potential NullPointerException while querying BrowserProvider for BOOKMARKS

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Grisha, Unassigned)

Tracking

unspecified
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
If BrowserProvider was queried for BOOKMARKS with the following being true:
- selection=null
- shouldShowDeleted(uri) === true

Then we will crash here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java#1007

This change was introduced here: https://hg.mozilla.org/mozilla-central/rev/d5064e39467c

An actual crash log from today:

java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.String.contains(java.lang.CharSequence)' on a null object reference
                                                                           at org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:1054)
                                                                           at android.content.ContentProvider.query(ContentProvider.java:1017)
                                                                           at android.content.ContentProvider$Transport.query(ContentProvider.java:238)
                                                                           at android.content.ContentResolver.query(ContentResolver.java:491)
                                                                           at android.content.ContentResolver.query(ContentResolver.java:434)
                                                                           at org.mozilla.gecko.sync.repositories.android.RepoUtils$QueryHelper.safeQuery(RepoUtils.java:51)
                                                                           at org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositoryDataAccessor.fetchAll(AndroidBrowserRepositoryDataAccessor.java:124)
                                                                           at org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositorySession.createRecordToGuidMap(AndroidBrowserRepositorySession.java:681)
                                                                           at org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositorySession.putRecordToGuidMap(AndroidBrowserRepositorySession.java:747)
                                                                           at org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositorySession.updateBookkeeping(AndroidBrowserRepositorySession.java:757)
                                                                           at org.mozilla.gecko.sync.repositories.android.AndroidBrowserBookmarksRepositorySession.updateBookkeeping(AndroidBrowserBookmarksRepositorySession.java:825)
                                                                           at org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositorySession.replace(AndroidBrowserRepositorySession.java:575)
                                                                           at org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositorySession$1.run(AndroidBrowserRepositorySession.java:514)
                                                                           at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
                                                                           at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
                                                                           at java.lang.Thread.run(Thread.java:818)https://pastebin.mozilla.org/8868293
Reporter

Comment 1

3 years ago
We should be covering CP code with junit4 tests, to avoid issues like this. Example of what BrowserProvider tests might look like (in that case, for history and visits): https://reviewboard.mozilla.org/r/40955/diff/13#3
It is possible and valid to have a null selection. All other manipulations
are null-safe, and we need to be able to handle the null-case when testing for
annotations being part of the selection.

Review commit: https://reviewboard.mozilla.org/r/47611/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47611/
Attachment #8743157 - Flags: review?(gkruglov)
Reporter

Comment 3

3 years ago
Comment on attachment 8743157 [details]
MozReview Request: Bug 1265908 - Avoid NPE with empty selection r?grisha

https://reviewboard.mozilla.org/r/47611/#review44521
Attachment #8743157 - Flags: review?(gkruglov) → review+

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59ce73318110
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.