Closed Bug 1265908 Opened 8 years ago Closed 8 years ago

Potential NullPointerException while querying BrowserProvider for BOOKMARKS

Categories

(Firefox for Android Graveyard :: Data Providers, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: Grisha, Unassigned)

Details

Attachments

(1 file)

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
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)
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+
https://hg.mozilla.org/mozilla-central/rev/59ce73318110
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.