883500 - 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)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

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

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

This is the top crash for Firefox for Android 26b1.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 3

[Lucas Rocha (:lucasr)]

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.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to [e-mail] [: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).

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 7

[Lucas Rocha (:lucasr)]

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

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 9

[Robert Kaiser]

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

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: Update query avoid bookmarks with null URLs (r=mfinkle)

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 832954 [details] [diff] [review]
Update query avoid bookmarks with null URLs (r=mfinkle)

I assume you wanted a review on this

Comment 13

[Richard Newman [:rnewman]]

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.

Comment 14

[Richard Newman [:rnewman]]

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?

Comment 15

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d2a075d27ae

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/2d2a075d27ae

Comment 17

[Lucas Rocha (:lucasr)]

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

Comment 18

[Lucas Rocha (:lucasr)]

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

Comment 19

[bhavana bajaj [:bajaj]]

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

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

"This is a tentative fix for bug 930163. We need it in Aurora too."

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

... and bug 930163 is #6 crasher on Fx27

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7d968603fad1

Comment 23

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa324ac537dd