Closed Bug 883500 Opened 6 years ago Closed 6 years ago

java.lang.IllegalArgumentException: the bind value at index <n> is null at android.database.sqlite.SQLiteProgram.bindString(SQLiteProgram.java) at org.mozilla.gecko.db.LocalBrowserDB.getFaviconsForUrls(LocalBrowserDB.java)

Categories

(Firefox for Android :: General, defect, critical)

22 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 26
Tracking Status
firefox21 --- unaffected
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 + fixed
firefox27 --- fixed
firefox28 --- fixed
b2g-v1.2 --- fixed
fennec 26+ ---

People

(Reporter: scoobidiver, Assigned: lucasr)

References

Details

(Keywords: crash, regression, topcrash-android-armv7, Whiteboard: [native-crash][startupcrash])

Crash Data

Attachments

(1 file)

It has been by several users in 22.0b5, including bp-c9a0a165-674c-45d3-8de7-4ae592130615, and one user in 23.0a2/20130612.
In case it was a regression, the Beta regression range would be:
http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=91c83cb4a969&tochange=7af2b98bd245
It might be a regression from bug 813546.
At that time, I don't know which one of both is the highest crasher (this bug is #17 in the first hours of 22.0b5, bug 813546 is #24 in 22.0b4). My guess is that they are equivalent.

java.lang.IllegalArgumentException: the bind value at index 1 is null
	at android.database.sqlite.SQLiteProgram.bindString(SQLiteProgram.java:304)
	at android.database.sqlite.SQLiteProgram.bindAllArgsAsStrings(SQLiteProgram.java:408)
	at android.database.sqlite.SQLiteQuery.<init>(SQLiteQuery.java:55)
	at android.database.sqlite.SQLiteDirectCursorDriver.query(SQLiteDirectCursorDriver.java:74)
	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1686)
	at android.database.sqlite.SQLiteQueryBuilder.query(SQLiteQueryBuilder.java:375)
	at org.mozilla.firefox_beta.db.BrowserProvider.query(BrowserProvider.java:2763)
	at android.content.ContentProvider$Transport.query(ContentProvider.java:189)
	at android.content.ContentResolver.query(ContentResolver.java:315)
	at org.mozilla.gecko.db.LocalBrowserDB.getFaviconsForUrls(LocalBrowserDB.java:753)
	at org.mozilla.gecko.db.BrowserDB.getFaviconsForUrls(BrowserDB.java:233)
	at org.mozilla.gecko.AllPagesTab$9.doInBackground(AllPagesTab.java:859)
	at org.mozilla.gecko.AllPagesTab$9.doInBackground(AllPagesTab.java:856)
	at org.mozilla.gecko.util.UiAsyncTask$BackgroundTaskRunnable.run(UiAsyncTask.java:48)
	at android.os.Handler.handleCallback(Handler.java:608)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:156)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.IllegalArgumentException%3A+the+bind+value+at+index+1+is+null+at+android.database.sqlite.SQLiteProgram.bindString%28SQLiteProgram.java%29
Whiteboard: [native-crash] → [native-crash][startupcrash]
Hardware: ARM → All
Crash Signature: [@ java.lang.IllegalArgumentException: the bind value at index 1 is null at android.database.sqlite.SQLiteProgram.bindString(SQLiteProgram.java)] → [@ java.lang.IllegalArgumentException: the bind value at index 1 is null at android.database.sqlite.SQLiteProgram.bindString(SQLiteProgram.java)] [@ java.lang.IllegalArgumentException: the bind value at index 2 is null at android.database.sqlite.SQLitePr…
Summary: java.lang.IllegalArgumentException: the bind value at index 1 is null at android.database.sqlite.SQLiteProgram.bindString(SQLiteProgram.java) at org.mozilla.gecko.db.LocalBrowserDB.getFaviconsForUrls(LocalBrowserDB.java) → java.lang.IllegalArgumentException: the bind value at index <n> is null at android.database.sqlite.SQLiteProgram.bindString(SQLiteProgram.java) at org.mozilla.gecko.db.LocalBrowserDB.getFaviconsForUrls(LocalBrowserDB.java)
This is the top crash for Firefox for Android 26b1.
tracking-fennec: --- → ?
Flags: needinfo?(mark.finkle)
Some notes:
* The original callstack is Fx25.
* The code has changed a lot in Fx26, but still has a crash.
* The code is gone from Fx27+

In Fx26, the call that crashes seems to be the binding in this function:
http://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/db/LocalBrowserDB.java#710

The only binding is the "uri", so does that mean it's null?

Adding Lucas
Flags: needinfo?(mark.finkle) → needinfo?(lucasr.at.mozilla)
Had a look at this. This bug seems to have the same root cause than bug 930163 i.e. null URLs coming from the DB. The crash just happens at a different point here because in we're using TextUtils.equals() in Beta, instead of url.equals().

The right thing to do here is to investigate why we're getting null URLs in the DB, as rnewman suggested in bug 930163. Based on the crash comments, this seems to be happening more often with bookmarks.
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #3)
> Had a look at this. This bug seems to have the same root cause than bug
> 930163 i.e. null URLs coming from the DB. The crash just happens at a
> different point here because in we're using TextUtils.equals() in Beta,
> instead of url.equals().
> 
> The right thing to do here is to investigate why we're getting null URLs in
> the DB, as rnewman suggested in bug 930163. Based on the crash comments,
> this seems to be happening more often with bookmarks.

If this is only in 26 because the code is still present there and is gone from 27 is it possible to do a null check for Beta to close the loop on this particular topcrash? (and if yes, might Lucas be the person who could put up such a potential fix so we can try it out and collect feedback early in the beta cycle?)
Flags: needinfo?(lucasr.at.mozilla)
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #4)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> > Had a look at this. This bug seems to have the same root cause than bug
> > 930163 i.e. null URLs coming from the DB. The crash just happens at a
> > different point here because in we're using TextUtils.equals() in Beta,
> > instead of url.equals().
> > 
> > The right thing to do here is to investigate why we're getting null URLs in
> > the DB, as rnewman suggested in bug 930163. Based on the crash comments,
> > this seems to be happening more often with bookmarks.
> 
> If this is only in 26 because the code is still present there and is gone
> from 27 is it possible to do a null check for Beta to close the loop on this
> particular topcrash? (and if yes, might Lucas be the person who could put up
> such a potential fix so we can try it out and collect feedback early in the
> beta cycle?)

Adding a null check here will likely just move the crash somewhere else i.e. 27+ is very likely affected too, just in a different way (see 930163). We assume the URL in cursors is non-null in many parts of our code. Also, null URLs here will lead to slightly broken UI state (e.g. empty rows in bookmark list, rows that cannot be opened, or something like this).
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #5)

> Adding a null check here will likely just move the crash somewhere else i.e.
> 27+ is very likely affected too, just in a different way (see 930163). We
> assume the URL in cursors is non-null in many parts of our code. Also, null
> URLs here will lead to slightly broken UI state (e.g. empty rows in bookmark
> list, rows that cannot be opened, or something like this).

I just wanted to note that I would happily trade a crash that kills the application for a bandaid fix that shows a broken UI state, especially if the broken state is a missing favicon or thumbnail.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Lucas Rocha (:lucasr) from comment #5)
> 
> > Adding a null check here will likely just move the crash somewhere else i.e.
> > 27+ is very likely affected too, just in a different way (see 930163). We
> > assume the URL in cursors is non-null in many parts of our code. Also, null
> > URLs here will lead to slightly broken UI state (e.g. empty rows in bookmark
> > list, rows that cannot be opened, or something like this).
> 
> I just wanted to note that I would happily trade a crash that kills the
> application for a bandaid fix that shows a broken UI state, especially if
> the broken state is a missing favicon or thumbnail.

It's not just missing favicon/thumbnail. It's more like an empty row in the list which causes the throbber to spin (but doesn't load anything).

Maybe one possible (and more solid?) workaround here could be to tweak our queries to avoid empty/null URLs entirely? Thoughts?
(In reply to Lucas Rocha (:lucasr) from comment #7)

> Maybe one possible (and more solid?) workaround here could be to tweak our
> queries to avoid empty/null URLs entirely? Thoughts?

That makes sense. We could be looking at corrupt DBs or just data that was saved poorly. We could try to clean the DB in a DB update too.

But being defensive and excluding bad rows seems like a good idea too.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> I just wanted to note that I would happily trade a crash that kills the
> application for a bandaid fix that shows a broken UI state

Yes. IMHO (from the point of the stability program), that should be our default answer to such issues. Just put yourself in the position of a user: Having your whole browser go away is definitely worse than some UI breakage. In many cases, I think a two-pronged approach makes sense: mitigate the symptoms of data weirdness to be more robust *and* try to find the actual source to eliminate the most likely source of data becoming weird.
tracking-fennec: ? → 26+
(In reply to Lucas Rocha (:lucasr) from comment #7)

> Maybe one possible (and more solid?) workaround here could be to tweak our
> queries to avoid empty/null URLs entirely? Thoughts?

Assigning to Lucas
Assignee: nobody → lucasr.at.mozilla
Comment on attachment 832954 [details] [diff] [review]
Update query avoid bookmarks with null URLs (r=mfinkle)

I assume you wanted a review on this
Attachment #832954 - Flags: review+
Comment on attachment 832954 [details] [diff] [review]
Update query avoid bookmarks with null URLs (r=mfinkle)

Nominating to make today's beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Old code.
 
User impact if declined: 
  Crashes due to unsafe access of potentially null data.

Testing completed (on m-c, etc.): 
  I assume that lucasr tested the positive case with this -- that it continues to show the expected data. There are no tests to verify the failure case, and this is a beta-only patch, so no baking.

Risk to taking this patch (and alternatives if risky): 
  Very low. This is the SQL equivalent of a null check.

String or IDL/UUID changes made by this patch:
  None.
Attachment #832954 - Flags: approval-mozilla-beta?
Have we audited the current codebase to look for similar assumptions?

Does anyone have an opinion on a good way to report or handle this dirty data going forward?

-- Add a pair of telemetry probes: null bookmarks with/without Sync set up?
-- Add this kind of thing to FHR?
-- Just clean it up with a versioned fixup step, and move on with our lives?
Status: NEW → ASSIGNED
Attachment #832954 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/2d2a075d27ae
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I suspect this patch might fix/workaround bug 930163 too (which is a top crasher in Firefox 27/28 right now). It's just a different symptom caused by the same issue (null URL coming from cursor).

Need to see how this affects the crashers in Nightly. Pushed:
https://hg.mozilla.org/integration/fx-team/rev/7d968603fad1
Blocks: 930163
Comment on attachment 832954 [details] [diff] [review]
Update query avoid bookmarks with null URLs (r=mfinkle)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: Top crasher in 27
Testing completed (on m-c, etc.): This is a tentative fix for bug 930163. We need it in Aurora too.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: n/a
Attachment #832954 - Flags: approval-mozilla-aurora?
(In reply to Lucas Rocha (:lucasr) from comment #18)
> Comment on attachment 832954 [details] [diff] [review]
> Update query avoid bookmarks with null URLs (r=mfinkle)
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): unknown
> User impact if declined: Top crasher in 27
> Testing completed (on m-c, etc.): This is a tentative fix for bug 930163. We
> need it in Aurora too.
> Risk to taking this patch (and alternatives if risky): none
> String or IDL/UUID changes made by this patch: n/a

Looks like firefox27 is unaffected here, so would we still need this ? IS this is the top-crasher in 27 ?
"This is a tentative fix for bug 930163. We need it in Aurora too."
... and bug 930163 is #6 crasher on Fx27
Attachment #832954 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.