823550 - Ensure all DB access happens on the GeckoBackgroundThread

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

Right now we are getting some crashes [1] that may be caused by concurrent DB access from multiple threads. We also have situations (e.g. bug 738066) where we are accessing the DB from the UI thread, which is bad for responsiveness anyway. We should ensure that all DB access happens from a single thread (probably the "GeckoBackgroundThread") and enforce this using assertions.

[1] https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2012-12-20&signature=android.database.sqlite.SQLiteDatabaseLockedException%3A%20database%20is%20locked%20%28code%205%29%20at%20android.database.sqlite.SQLiteConnection.nativeExecute%28Native%20Method%29&version=FennecAndroid%3A20.0a1

Comment 1

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

Attachment: simple wip

This patch adds GeckoBackgroundThread.assertOnThread() and calls it from a few places in BrowserProvider. Just using this to help investigate the problem.

Comment 2

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

E/GeckoAppShell(18483): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1307 ("AsyncTask #4")
E/GeckoAppShell(18483): java.lang.RuntimeException: An error occured while executing doInBackground()
E/GeckoAppShell(18483): 	at android.os.AsyncTask$3.done(AsyncTask.java:299)
E/GeckoAppShell(18483): 	at java.util.concurrent.FutureTask$Sync.innerSetException(FutureTask.java:273)
E/GeckoAppShell(18483): 	at java.util.concurrent.FutureTask.setException(FutureTask.java:124)
E/GeckoAppShell(18483): 	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:307)
E/GeckoAppShell(18483): 	at java.util.concurrent.FutureTask.run(FutureTask.java:137)
E/GeckoAppShell(18483): 	at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:230)
E/GeckoAppShell(18483): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1076)
E/GeckoAppShell(18483): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:569)
E/GeckoAppShell(18483): 	at java.lang.Thread.run(Thread.java:856)
E/GeckoAppShell(18483): Caused by: java.lang.IllegalThreadStateException: XXX Expected thread 1293 ("GeckoBackgroundThread"), but running on thread 1307 ("AsyncTask #4)
E/GeckoAppShell(18483): 	at org.mozilla.gecko.util.GeckoBackgroundThread.assertOnThread(GeckoBackgroundThread.java:61)
E/GeckoAppShell(18483): 	at org.mozilla.fennec_mfinkle.db.BrowserProvider.update(BrowserProvider.java:2161)
E/GeckoAppShell(18483): 	at android.content.ContentProvider$Transport.update(ContentProvider.java:235)
E/GeckoAppShell(18483): 	at android.content.ContentResolver.update(ContentResolver.java:990)
E/GeckoAppShell(18483): 	at org.mozilla.gecko.db.LocalBrowserDB.updateFaviconForUrl(LocalBrowserDB.java:725)
E/GeckoAppShell(18483): 	at org.mozilla.gecko.db.BrowserDB.updateFaviconForUrl(BrowserDB.java:212)
E/GeckoAppShell(18483): 	at org.mozilla.gecko.Favicons$LoadFaviconTask.saveFaviconToDb(Favicons.java:217)
E/GeckoAppShell(18483): 	at org.mozilla.gecko.Favicons$LoadFaviconTask.doInBackground(Favicons.java:300)
E/GeckoAppShell(18483): 	at org.mozilla.gecko.Favicons$LoadFaviconTask.doInBackground(Favicons.java:182)
E/GeckoAppShell(18483): 	at android.os.AsyncTask$2.call(AsyncTask.java:287)
E/GeckoAppShell(18483): 	at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:305)

Comment 3

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

Attachment: Save favicons in GeckoBackgroundThread

This patch moves the DB write used for favicons into the GeckoBackgroundThread. The code was already in a background thread, just not the right one. I couldn't make LoadFaviconTask extend GeckoAsyncTask because the latter does not support canceling, which the former requires. Only the DB write needs to be in GeckoBackgroundThread anyway,so I just did a quick, simple runnable.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 694837 [details] [diff] [review]
simple wip

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

Aren't there other files in the db/ folder that do DB access? Do those need to be assert'ed as well?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 694868 [details] [diff] [review]
Save favicons in GeckoBackgroundThread

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

This look ok, but there are other functions in that file, like loadFaviconFromDb and getFaviconUrlForPageUrl which do DB access and are running on the async task's background thread. It seems like those should be moved as well, no?

Comment 6

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 694837 [details] [diff] [review]
> simple wip
> 
> Review of attachment 694837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Aren't there other files in the db/ folder that do DB access? Do those need
> to be assert'ed as well?

Yeah, we'll need to add a lot more assertion checks before we can land this approach. Currently I using this to find problems in the code and making spot fixes.

Comment 7

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Comment on attachment 694868 [details] [diff] [review]
> Save favicons in GeckoBackgroundThread
> 
> Review of attachment 694868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This look ok, but there are other functions in that file, like
> loadFaviconFromDb and getFaviconUrlForPageUrl which do DB access and are
> running on the async task's background thread. It seems like those should be
> moved as well, no?

I was looking at writeable DB code paths first. I need to verify that read-only code paths are not affected by locking (or don't cause locking in the first place)

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I think we should verify that first; if it isn't the case we'll need a different approach to fixing this, I think. Doing spot fixes for each DB read/write call probably won't scale very well.

Comment 9

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

Attachment: better assert on background patch

This patch adds the assert into the getWritableDatabase wrapper of BrowserProvider and TabsProvider. That should cover all the Fennec front-end DB access and some of Sync too.

I need to look more closely at the form history and password providers. Those are used by Sync and Gecko, but no the Fennec front-end, iirc. The interactions with Gecko could make them trickier.

Comment 10

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

Comment on attachment 694868 [details] [diff] [review]
Save favicons in GeckoBackgroundThread

https://hg.mozilla.org/integration/mozilla-inbound/rev/be3375d887c6

Comment 11

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

Leaving open in case we find more spot fixes and we can land the assert checks.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be3375d887c6

Comment 13

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

Comment on attachment 694868 [details] [diff] [review]
Save favicons in GeckoBackgroundThread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: This patch might help reduce the "DB locked" crashes
Testing completed (on m-c, etc.): It's been on m-c for a while
Risk to taking this patch (and alternatives if risky): low-risk. Testing has found no problems on m-c
String or UUID changes made by this patch: none

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 694868 [details] [diff] [review]
Save favicons in GeckoBackgroundThread

Approving for Aurora 19. We should check whether bug 752828 is resolved in FF19 on beta with these changes.

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0846fc48c969

Comment 16

[Sylvestre Ledru [:Sylvestre]]

I think it can be closed now.