734624 - java.lang.RuntimeException: An error occured while executing doInBackground() at android.os.AsyncTask$3.done(AsyncTask.java) caused by: android.database.sqlite.SQLiteConstraintException: error code 19: constraint failed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

It first appeared in 13.0a1/20120309050057 and has been hit by 5 different users.
The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2f6368ca605e&tochange=ead9016b4102

It's #2 top crasher in Fennec 13.0a1 over the last day.

java.lang.RuntimeException: An error occured while executing doInBackground()
	at android.os.AsyncTask$3.done(AsyncTask.java:200)
	at java.util.concurrent.FutureTask$Sync.innerSetException(FutureTask.java:274)
	at java.util.concurrent.FutureTask.setException(FutureTask.java:125)
	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:308)
	at java.util.concurrent.FutureTask.run(FutureTask.java:138)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1088)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:581)
	at java.lang.Thread.run(Thread.java:1019)
Caused by: android.database.sqlite.SQLiteConstraintException: error code 19: constraint failed
	at android.database.sqlite.SQLiteStatement.native_execute(Native Method)
	at android.database.sqlite.SQLiteStatement.execute(SQLiteStatement.java:61)
	at android.database.sqlite.SQLiteDatabase.insertWithOnConflict(SQLiteDatabase.java:1582)
	at android.database.sqlite.SQLiteDatabase.insertOrThrow(SQLiteDatabase.java:1452)
	at org.mozilla.fennec.db.BrowserProvider.insertInTransaction(BrowserProvider.java:965)
	at org.mozilla.fennec.db.BrowserProvider.insert(BrowserProvider.java:868)
	at android.content.ContentProvider$Transport.insert(ContentProvider.java:198)
	at android.content.ContentResolver.insert(ContentResolver.java:613)
	at org.mozilla.gecko.db.LocalBrowserDB.updateFaviconForUrl(LocalBrowserDB.java:528)
	at org.mozilla.gecko.db.BrowserDB.updateFaviconForUrl(BrowserDB.java:180)
	at org.mozilla.gecko.Favicons$LoadFaviconTask.saveFaviconToDb(Favicons.java:261)
	at org.mozilla.gecko.Favicons$LoadFaviconTask.downloadFavicon(Favicons.java:312)
	at org.mozilla.gecko.Favicons$LoadFaviconTask.doInBackground(Favicons.java:368)
	at org.mozilla.gecko.Favicons$LoadFaviconTask.doInBackground(Favicons.java:225)
	at android.os.AsyncTask$2.call(AsyncTask.java:185)
	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:306)
	... 4 more

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.RuntimeException%3A%20An%20error%20occured%20while%20executing%20doInBackground%28%29%20at%20android.os.AsyncTask%243.done%28AsyncTask.java%29

Comment 1

[Scoobidiver (away)]

It represents 15% of crashes over the last 3 days making it #1 top crasher.

Comment 2

[Chris Peterson [:cpeterson]]

This bug is related to bug 716729 and 712791.

Bug 712791 was "fixed" with a try/catch in AndroidBrowserDB.java. This crash suggests that the try/catch should also be added to LocalBrowserDB.java (until bug 716729 identifies the root cause of the SQLiteConstraintException).

Comment 3

[Scoobidiver (away)]

Which bug removed the try/catch in the regression range? It should be backed out from 13.0.

Comment 4

[Cristian Nicolae (:xti)]

This issue occurred on the latest Maple build while trying to open a crash report: https://crash-stats.mozilla.com/report/index/bp-765d0f49-9917-44cd-9381-6c4322120314

--
Maple (2012-03-14)
Device: Samsung Nexus S
OS: Android 2.3.6

Comment 5

[Chris Peterson [:cpeterson]]

> Which bug removed the try/catch in the regression range? It should be backed out from 13.0.

No code was removed. Bug 712791 added a try/catch to AndroidBrowserDB.java. This bug's crash is in LocalBrowserDB.java. The two files share a lot of copy/pasted code.

Comment 6

[Chris Peterson [:cpeterson]]

margaret, can you recommend someone to investigate this LocalBrowserDB.java crash?

After reviewing the LocalBrowserDB.java code, I do not think we should use the try/catch workaround from AndroidBrowserDB.java. At the time, we thought we were hacking around a SQL schema change in Google's Bookmarks/Favicon content provider. But the LocalBrowserDB.java is entirely our own code and schema, so this SQLiteConstraintException is our own bug.

Maybe there is a size limit on the favicon blob column?

Comment 7

[:Margaret Leibovic]

I believe Lucas worked on this. I haven't had time to look into this yet, but perhaps he has some insight.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this is a bit of a stab in the dark, but one thing that could cause that exception is the GUID not being unique. 

Another possibility is a null value in values, this patch doesn't try to guard against that.

Comment 9

[Chris Peterson [:cpeterson]]

Generating duplicate random GUIDs seems unlikely.

The clue I think is most interesting is that these SQLiteConstraintExceptions ONLY happen when inserting favicon images (same as the many exceptions reported in AndroidBrowserDB.java bug 712791).

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 606303 [details] [diff] [review]
patch

Review of attachment 606303 [details] [diff] [review]:
-----------------------------------------------------------------

The bug is definitely not a failed constraint on GUID because there's no UNIQUE constraint there. The root cause is most likely that there's a race in updateFaviconForUrl() where a we're trying to insert a favicon when there's already an entry for that URL.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to sync save favicon

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to wrap insert with a try/catch

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 606603 [details] [diff] [review]
patch to sync save favicon

Review of attachment 606603 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch.

::: mobile/android/base/Favicons.java
@@ +257,5 @@
>          // Runs in background thread
>          private void saveFaviconToDb(BitmapDrawable favicon) {
> +            // since the Async task can run this on any number of threads in the
> +            // pool, we need to protect against inserting the same url twice
> +            synchronized(mDbHelper) {

Any reason for not making the whole method synchronized instead?

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 606605 [details] [diff] [review]
patch to wrap insert with a try/catch

Review of attachment 606605 [details] [diff] [review]:
-----------------------------------------------------------------

Wouldn't this hide actual bugs on insert() calls? Would those exceptions be reported in crash-stats and the like? My impression is that if insert() fails here it's probably due to a bug on the caller side that needs fixing and we should not hide it inside try/catch blocks. I'd like to be convinced that this change is needed and won't hide bugs before giving r+.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #13)
> Comment on attachment 606603 [details] [diff] [review]
> patch to sync save favicon
> 
> Review of attachment 606603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good catch.
> 
> ::: mobile/android/base/Favicons.java
> @@ +257,5 @@
> >          // Runs in background thread
> >          private void saveFaviconToDb(BitmapDrawable favicon) {
> > +            // since the Async task can run this on any number of threads in the
> > +            // pool, we need to protect against inserting the same url twice
> > +            synchronized(mDbHelper) {
> 
> Any reason for not making the whole method synchronized instead?

making the method synchronized will synchronize on this, which is pretty useless because we create a new object for every task.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #14)
> Comment on attachment 606605 [details] [diff] [review]
> patch to wrap insert with a try/catch
> 
> Review of attachment 606605 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wouldn't this hide actual bugs on insert() calls? Would those exceptions be
> reported in crash-stats and the like? My impression is that if insert()
> fails here it's probably due to a bug on the caller side that needs fixing
> and we should not hide it inside try/catch blocks. I'd like to be convinced
> that this change is needed and won't hide bugs before giving r+.

The bugs will be logged to logcat. This should not be a fatal condition. You can continue running along, viewing web pages happily if saving a favicon to the DB fails.  IMO it is a never acceptable to let the user crash just to get data.

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] from comment #16)
> (In reply to Lucas Rocha (:lucasr) from comment #14)
> > Comment on attachment 606605 [details] [diff] [review]
> > patch to wrap insert with a try/catch
> > 
> > Review of attachment 606605 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Wouldn't this hide actual bugs on insert() calls? Would those exceptions be
> > reported in crash-stats and the like? My impression is that if insert()
> > fails here it's probably due to a bug on the caller side that needs fixing
> > and we should not hide it inside try/catch blocks. I'd like to be convinced
> > that this change is needed and won't hide bugs before giving r+.
> 
> The bugs will be logged to logcat. This should not be a fatal condition. You
> can continue running along, viewing web pages happily if saving a favicon to
> the DB fails.  IMO it is a never acceptable to let the user crash just to
> get data.

I agree that failing to save a *favicon* should not crash Fennec. But your patch makes any DB insertion crash-safe which is a bit too far IMO (DB inserts happen in very different contexts). I'd feel more comfortable if you simply enclosed the saveFaviconToDb call in try/catch block.

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] from comment #15)
> (In reply to Lucas Rocha (:lucasr) from comment #13)
> > Comment on attachment 606603 [details] [diff] [review]
> > patch to sync save favicon
> > 
> > Review of attachment 606603 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Good catch.
> > 
> > ::: mobile/android/base/Favicons.java
> > @@ +257,5 @@
> > >          // Runs in background thread
> > >          private void saveFaviconToDb(BitmapDrawable favicon) {
> > > +            // since the Async task can run this on any number of threads in the
> > > +            // pool, we need to protect against inserting the same url twice
> > > +            synchronized(mDbHelper) {
> > 
> > Any reason for not making the whole method synchronized instead?
> 
> making the method synchronized will synchronize on this, which is pretty
> useless because we create a new object for every task.

Oh, right, I thought saveFaviconToDb() was a Favicons method. Never mind.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c6a75b84c1f

Comment 20

[Marco Bonardo [:mak]]

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

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 606605 [details] [diff] [review]
patch to wrap insert with a try/catch

mfinkle, over to you for a re-request

Comment 22

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

Comment on attachment 606605 [details] [diff] [review]
patch to wrap insert with a try/catch

Looks OK. We were try/catching around the honeycomb transaction branch, but not the older non-transaction branch. We do now, but we still log errors to logcat. I am satisfied.

Comment 23

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #22)
> Comment on attachment 606605 [details] [diff] [review]
> patch to wrap insert with a try/catch
> 
> Looks OK. We were try/catching around the honeycomb transaction branch, but
> not the older non-transaction branch. We do now, but we still log errors to
> logcat. I am satisfied.

No, we are not catching exceptions anywhere in this code. We're simply using finally to ensure we end the transaction no matter what happens to the DB command. This patch goes too far and makes all our DB inserts crash-safe which is not a good idea. We have quite a few ContentProvider callers that expect DB commands to just work. This patch would make a serious bug much less visible. e.g. when you add a bookmark, we show a toast notification even if the DB insert failed. User would think the operation was successful when it wasn't. And this is just one small example.

For a lot of people, a crasher is what makes them look at logcat. I've seen several exceptions stay unfiled as bugs because we were just logging them and not crashing. This bug is about making the favicon update operation non-fatal. Hence it should only catch exceptions for this particular case (although I'm pretty confident that pushed patch has fixed the DB crash).

DB errors should be fatal by default. We can and should add exceptions to this rule where applicable and not just bury every DB error in logcat.

Comment 24

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/4e4076eb6524

Comment 25

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

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