Ensure all DB access happens on the GeckoBackgroundThread

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
7 years ago
10 months ago

People

(Reporter: kats, Assigned: mfinkle)

Tracking

(Depends on 1 bug)

unspecified
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19 fixed, firefox20 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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
Posted patch simple wip (obsolete) — Splinter Review
This patch adds GeckoBackgroundThread.assertOnThread() and calls it from a few places in BrowserProvider. Just using this to help investigate the problem.
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)
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.
Attachment #694868 - Flags: review?(bugmail.mozilla)
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 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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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)
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.
Attachment #694868 - Flags: review?(bugmail.mozilla) → review+
Depends on: 738066
No longer depends on: 738006
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.
Attachment #694837 - Attachment is obsolete: true
Leaving open in case we find more spot fixes and we can land the assert checks.
Whiteboard: [leave-open]
Assignee: bugmail.mozilla → mark.finkle
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
Attachment #694868 - Flags: approval-mozilla-aurora?
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.
Attachment #694868 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I think it can be closed now.
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
You need to log in before you can comment on or make changes to this bug.