Sub-bug of Bug 752287. In this case, we're crashing because we have two parallel connections to the same DB, and it's already open for writing -- we're writing the history event while you try to save a reading list item. android.database.sqlite.SQLiteDatabaseLockedException: error code 5: database is locked at android.database.sqlite.SQLiteStatement.native_1x1_string(Native Method) at android.database.sqlite.SQLiteStatement.simpleQueryForString(SQLiteStatement.java:161) at android.database.DatabaseUtils.stringForQuery(DatabaseUtils.java:813) at android.database.DatabaseUtils.stringForQuery(DatabaseUtils.java:801) at android.database.sqlite.SQLiteDatabase.setJournalMode(SQLiteDatabase.java:1063) at android.database.sqlite.SQLiteDatabase.openDatabase(SQLiteDatabase.java:999) at android.database.sqlite.SQLiteDatabase.openOrCreateDatabase(SQLiteDatabase.java:1054) at android.app.ContextImpl.openOrCreateDatabase(ContextImpl.java:770) at android.content.ContextWrapper.openOrCreateDatabase(ContextWrapper.java:221) at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:157) at org.mozilla.gecko.db.TransactionalProvider.getWritableDatabase(TransactionalProvider.java:116) at org.mozilla.gecko.db.TransactionalProvider.update(TransactionalProvider.java:392) at android.content.ContentProvider$Transport.update(ContentProvider.java:219) at android.content.ContentResolver.update(ContentResolver.java:856) at org.mozilla.gecko.db.LocalBrowserDB.addReadingListItem(LocalBrowserDB.java:719) at org.mozilla.gecko.db.BrowserDB.addReadingListItem(BrowserDB.java:276) at org.mozilla.gecko.BrowserApp$5.run(BrowserApp.java:404) at android.os.Handler.handleCallback(Handler.java:605) at android.os.Handler.dispatchMessage(Handler.java:92) at android.os.Looper.loop(Looper.java:137) at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)

This refactors out a couple of abstract classes, leaving an orphaned replacement for the current TransactionalProvider as PerProfileDatabaseProvider. BrowserProvider and ReadingListProvider now both extend the same abstract class, and should thus use the same database handles.

Small tweak.

At this juncture I had two choices: eliminate PerProfileDatabaseProvider, which was presently unused, or refactor TabsProvider to eliminate the duplicate logic that existed in PPDP's hierarchy. I opted for the latter. This patch cleans up and dramatically shrinks TabsProvider.

Also, please ignore trailing whitespace. Eclipse. Will fix locally.

I added some simplicity.

Sorry, found some more simplicity to add.

testBrowserProvider passes locally. Try: https://tbpl.mozilla.org/?tree=Try&rev=baa83af32833

*sigh* omitted file.

Comment on attachment 8394523 [details] [diff] [review] Part 1: ReadingListProvider and BrowserProvider should share DB accessors. v5 Review of attachment 8394523 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I have a few questions that require me thinking and us talking. Tomorrow! ::: mobile/android/base/db/AbstractPerProfileDatabaseProvider.java @@ +57,5 @@ > + if (uri != null) { > + profile = uri.getQueryParameter(BrowserContract.PARAM_PROFILE); > + } > + > + return getDatabases().getDatabaseHelperForProfile(profile, isTest(uri)).getWritableDatabase(); I checked, and isTest(null) will NPE. But this looks like it is intended to succeed with uri == null? ::: mobile/android/base/db/AbstractTransactionalProvider.java @@ +13,5 @@ > +import android.os.Build; > +import android.text.TextUtils; > +import android.util.Log; > + > +public abstract class AbstractTransactionalProvider extends ContentProvider { nit: the class comment for TransactionalProvider was useful. The absence here is a little confusing. I'm particularly confused by |getWriteableDatabase| versus |getWritableDatabaseForProfile|. It's not clear why a TransactionalProvider would necessarily know or not know about profiles at all. @@ +18,5 @@ > + private static final String LOGTAG = "GeckoTransProvider"; > + > + private static boolean logVerbose = Log.isLoggable(LOGTAG, Log.VERBOSE); > + > + protected Context context; Get rid of this; it's not consistently used (most places are already using getContext()). @@ +165,5 @@ > + * We can do this without using selection arguments because Long isn't > + * vulnerable to injection. > + */ > + protected static String computeSQLInClauseFromLongs(final Cursor cursor, > + String field) { nit: this indentation is funky. @@ +180,5 @@ > + builder.append(")"); > + return builder.toString(); > + } > + > + private static boolean logDebug = Log.isLoggable(LOGTAG, Log.DEBUG); nit: move this next to its sibling. @@ +297,5 @@ > + * > + * If not called in an existing transaction, no new explicit transaction > + * will be begun. > + */ > + protected void cleanupSomeDeletedRecords(Uri fromUri, Uri targetUri, String tableName) { nit: cleanUp. But 0fg. @@ +309,5 @@ > + // predefined max age. It's important not be too greedy here and > + // remove only a few old deleted records at a time. > + > + // Maximum age of deleted records to be cleaned up (20 days in ms) > + final long MAX_AGE_OF_DELETED_RECORDS = 86400000 * 20; This is part of an abstract class, right? These configuration options should be a parameter, or a method, or final fields. @@ +346,5 @@ > + * Indicates whether a query should include deleted fields > + * based on the URI. > + * @param uri query URI > + */ > + public static boolean shouldShowDeleted(Uri uri) { nit: consider making these helpers not be public. @@ +366,5 @@ > + * Indicates whether query is a test based on the URI. > + * @param uri query URI > + */ > + public static boolean isTest(Uri uri) { > + String isTest = uri.getQueryParameter(BrowserContract.PARAM_IS_TEST); nit: null checks all around. ::: mobile/android/base/db/SharedBrowserDatabaseProvider.java @@ +15,5 @@ > + * If multiple ContentProvider classes wish to share a database, it's > + * vitally important that they use the same SQLiteOpenHelpers for access. > + * > + * Failure to do so can cause accidental concurrent writes, with the result > + * being unexpected SQLITE_BUSY errors. I'm not a huge fan of bug links, but if there's more than just this bug to link to, let's do it. (This one will turn up in blame.)

Comment on attachment 8394498 [details] [diff] [review] Part 2: switch TabsProvider to use PerProfileDatabaseProvider. v1 Review of attachment 8394498 [details] [diff] [review]: ----------------------------------------------------------------- Oh snap, that's some nice deletion win. Run this against android-sync integration tests before landing, plz. ::: mobile/android/base/db/TabsProvider.java @@ +162,5 @@ > } > > // From Honeycomb on, it's possible to run several db > // commands in parallel using multiple connections. > + if (shouldUseTransactions()) { Hmm, if this is equivalent to the old code, the comment should explain as much or be updated. @@ +167,4 @@ > db.enableWriteAheadLogging(); > db.setLockingEnabled(false); > } else { > // Pre-Honeycomb, we can do some lesser optimizations. ditto.

(In reply to Nick Alexander :nalexander from comment #9) > Comment on attachment 8394523 [details] [diff] [review] > Part 1: ReadingListProvider and BrowserProvider should share DB accessors. v5 > > Review of attachment 8394523 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good, but I have a few questions that require me thinking and us > talking. Tomorrow! To elaborate: there's not much here to say how this should be used; and reading the code to figure out how it is used is not as illuminating as I expected. That suggests either a top-level overview comment, perhaps rooted to AbstractTP; or even an .rst file documenting how this is expected to work. There's a good deal of "introduction to DBs and CPs in Fennec" that's encoded in these changes that is worth teasing out.

Comment on attachment 8394523 [details] [diff] [review] Part 1: ReadingListProvider and BrowserProvider should share DB accessors. v5 Review of attachment 8394523 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/PerProfileDatabaseProvider.java @@ +26,5 @@ > + } > + > + protected abstract String getDatabaseName(); > + > + /* nit: Javadoc?

(In reply to Nick Alexander :nalexander from comment #9) > I checked, and isTest(null) will NPE. But this looks like it is intended to > succeed with uri == null? Honestly, no CP method should ever be called with a null URI. But presumably it was there for a reason (because that's a really obvious thing), so I've amended isTest. > nit: the class comment for TransactionalProvider was useful. The absence > here is a little confusing. I will add a class comment. > I'm particularly confused by > |getWriteableDatabase| versus |getWritableDatabaseForProfile|. It's not > clear why a TransactionalProvider would necessarily know or not know about > profiles at all. I just killed get*ForProfile; it doesn't belong here. The profile-aware provider introduces that. But yeah, this is the usual single inheritance bullshit. There's no reason why these two things can't be somewhat orthogonal. > > + protected Context context; > > Get rid of this; it's not consistently used (most places are already using > getContext()). Done. > > + // Maximum age of deleted records to be cleaned up (20 days in ms) > > + final long MAX_AGE_OF_DELETED_RECORDS = 86400000 * 20; > > This is part of an abstract class, right? These configuration options > should be a parameter, or a method, or final fields. Moved the cleanup method to the browser.db-specific class. > I'm not a huge fan of bug links, but if there's more than just this bug to > link to, let's do it. (This one will turn up in blame.) This one is a great start to the breadcrumb trail.

This test had an unused import that broke when we killed TransactionalProvider. And the test name triggered a "looks like a constructor" warning.

Review comments addressed.

Review comments addressed.

Comment on attachment 8394562 [details] [diff] [review] Pre: clean up testReadingListProvider. v1 Review of attachment 8394562 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/testReadingListProvider.java @@ +70,5 @@ > mTests.add(test); > } > } > > + public void testReadingListProviderTests() throws Exception { I know you are avoiding the warning, and I support this, but the convention is to name the test function like a constructor.

Comment on attachment 8394563 [details] [diff] [review] Part 1: ReadingListProvider and BrowserProvider should share DB accessors. v5 Review of attachment 8394563 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/AbstractTransactionalProvider.java @@ +25,5 @@ > + * These are all called expecting a transaction to be established, so failed > + * modifications can be rolled-back, and work batched. > + * > + * If no transaction is established, that's not a problem. Transaction nesting > + * can be avoided by using {@link #beginWrite(SQLiteDatabase)}. nit: trailing ws. @@ +34,5 @@ > + * which we don't handle well. Better to avoid starting a transaction too soon! > + * > + * You are probably interested in some subclasses: > + * > + * * {@link AbstractPerProfileDatabaseProvider} provides a simple abstraction for Good comment. ::: mobile/android/base/db/PerProfileDatabaseProvider.java @@ +25,5 @@ > + protected abstract String getDatabaseName(); > + > + /** > + * Creates and returns an instance of a DB helper. Given a > + * context and a path to the DB file Sentences, do you use ::: mobile/android/base/db/SharedBrowserDatabaseProvider.java @@ +68,5 @@ > + * > + * If not called in an existing transaction, no new explicit transaction > + * will be begun. > + */ > + protected void cleanUpSomeDeletedRecords(Uri fromUri, Uri targetUri, String tableName) { The targetUri parameter is not used. Also, it's strange to include a URI and a table name: doesn't the former determine the latter?

Comment on attachment 8394564 [details] [diff] [review] Part 2: switch TabsProvider to use PerProfileDatabaseProvider. v2 Review of attachment 8394564 [details] [diff] [review]: ----------------------------------------------------------------- If it's green, it looks fine to me.

 https://hg.mozilla.org/integration/fx-team/rev/f1d1a28b0f5a https://hg.mozilla.org/integration/fx-team/rev/d9e1c755f554 https://hg.mozilla.org/integration/fx-team/rev/c75b0b78ebbf

Backed out in https://hg.mozilla.org/integration/fx-team/rev/b24540f3c2c1 since bug 986872 is holding everything on fx-team hostage (or, worse yet, someone would decide to merge it around despite the 50% bustage).

Will continue investigation in Bug 986872, then reland. Thanks, philor!

I rebuilt testBrowserProviderPerf as a JUnit3 BrowserInstrumentationTest, and set it up to time both insertion and querying. Before my patches: 03-24 20:54:08.358 9449 9462 I GeckoTest: Test took 49482, 195, got 100 results. 03-24 20:55:08.978 9638 9651 I GeckoTest: Test took 45716, 184, got 100 results. 03-24 21:07:15.978 10039 10052 I GeckoTest: Test took 47021, 185, got 100 results. After: 03-24 21:11:58.858 10293 10306 I GeckoTest: Test took 44781, 191, got 100 results. 03-24 21:12:53.348 10378 10391 I GeckoTest: Test took 44182, 205, got 100 results. 03-24 21:15:22.768 10462 10475 I GeckoTest: Test took 48255, 261, got 100 results. So: total time is broadly unchanged (slightly lower, even), but query time is slightly elevated.

Comparing profiles, the entirety of the ~2% difference is in SQLiteConnection.nativeExecuteForCursorWindow -- that is, actually running in sqlite-land. We're talking about a 4msec difference in query time. As such: with respect to raw BrowserProvider performance, this patch does not regress. That doesn't explain the huge talos regression. From that, I deduce that something else is going on here: that when this test runs with the browser activity live, something else is affecting performance. Perhaps that's as simple as change notifications triggering activity redisplay while we're trying to time a 200msec query. More tomorrow.

I reached 1.5 metric mfinkles of annoyance, and decided to eliminate ContentProviderTest. Done. In so doing, I decided that BaseTest wasn't really a BaseTest at all, so I refactored out BaseRobocopTest, which doesn't do all of the Activity-related nonsense. These could do with a rename, I imagine. Later. Try push is all green: https://tbpl.mozilla.org/?tree=Try&rev=35b9336ebecd And last time I ran this locally, it *dramatically* reduced the runtime of the test, down to ridiculous levels. Thoughts?

I should point out: I verified, with profiles, that my change did not regress real perf.

Comment on attachment 8396918 [details] [diff] [review] Pre: introduce BaseRobocopTest, rework testBrowserProviderPerf. v1 Review of attachment 8396918 [details] [diff] [review]: ----------------------------------------------------------------- So, bombs away on Base{Robocop}Test. SHIP IT. I'm pretty confused by the testBrowserProviderPerf changes, re: the test profile creation. A few well chosen comments might suffice... ::: mobile/android/base/tests/AboutHomeTest.java @@ +39,3 @@ > super.setUp(); > > if (aboutHomeTabs.size() < 4) { Not your problem, but this is awful. ::: mobile/android/base/tests/BaseTest.java @@ +60,5 @@ > private static final int GECKO_READY_WAIT_MS = 180000; > public static final int MAX_WAIT_BLOCK_FOR_EVENT_DATA_MS = 90000; > > private static Class<Activity> mLauncherActivityClass; > + Activity mActivity; What? Scope everything. @@ +324,3 @@ > public void SqliteCompare(Cursor c, ContentValues[] cvs) { > mAsserter.is(c.getCount(), cvs.length, "List is correct length"); > if (c.moveToFirst()) { nit: while (c.moveToNext()) { } works, no? ::: mobile/android/base/tests/ContentProviderTest.java @@ +210,5 @@ > mProvider.attachInfo(mProviderContext, null); > > mResolver.addProvider(mProviderAuthority, mProvider); > } > I don't see this change being needed, but whatever, I'm not checking in full detail. ::: mobile/android/base/tests/testBrowserProviderPerf.java @@ +22,5 @@ > /* > * This test is meant to exercise the performance of Fennec's > * history and bookmarks content provider. > + * > + * It does not extent ContentProviderTest because CPT is unable nit: extend. Not a fan of the acronym, either: repeat, or "that class". @@ +25,5 @@ > + * > + * It does not extent ContentProviderTest because CPT is unable > + * to accurately assess the performance of the ContentProvider -- > + * it's a second instance of a class that's only supposed to > + * exist once, wrapped in a bunch of junk. * Instead, we ... @@ +31,5 @@ > +@SuppressWarnings("unchecked") > +public class testBrowserProviderPerf extends BaseRobocopTest { > + private static final String LAUNCH_ACTIVITY_FULL_CLASSNAME = TestConstants.ANDROID_PACKAGE_NAME + ".App"; > + > + private static Class<Activity> mLauncherActivityClass; Oh, man, I really need to nuke this pattern. @@ +234,5 @@ > + public void tearDown() { > + ContentProvider cp = mResolver.acquireContentProviderClient(mBookmarksURI).getLocalContentProvider(); > + BrowserProvider bp = ((BrowserProvider) cp); > + SQLiteDatabase db = bp.getWritableDatabaseForTesting(mBookmarksURI); > + db.close(); Please be more careful. If anything fails here, I really, really, really want the delTree to happen. @@ +241,5 @@ > + > + public Uri prepUri(Uri uri) { > + return uri.buildUpon() > + .appendQueryParameter("profile", mProfile) > + .appendQueryParameter(BrowserContract.PARAM_IS_TEST, "1") Something's not adding up here. PARAM_IS_TEST ends up setting the DB name to not have to do with the profile. Why are you going to lengths to arrange the profile when it's not being used? @@ +245,5 @@ > + .appendQueryParameter(BrowserContract.PARAM_IS_TEST, "1") > + .build(); > + } > + > + public void testBrowserProviderQueryPerf() throws Exception { Give me a brief JavaDoc explaining the shenanigans. /** * Insert a lot of entries into browser.db, then time a known query into that database. */ @@ +250,1 @@ > BrowserDB.initialize(GeckoProfile.DEFAULT_PROFILE); Why does this happen here and in |setUp|?

Comments addressed, and added some checks for the count of results.

Additional comments re changes: > Not your problem, but this is awful. Will file a follow-up. > > public void SqliteCompare(Cursor c, ContentValues[] cvs) { > > mAsserter.is(c.getCount(), cvs.length, "List is correct length"); > > if (c.moveToFirst()) { > > nit: > > while (c.moveToNext()) { > } > > works, no? I elected not to touch this, 'cos I don't want any more chance of breaking things :) > > +@SuppressWarnings("unchecked") > > +public class testBrowserProviderPerf extends BaseRobocopTest { > > + private static final String LAUNCH_ACTIVITY_FULL_CLASSNAME = TestConstants.ANDROID_PACKAGE_NAME + ".App"; > > + > > + private static Class<Activity> mLauncherActivityClass; > > Oh, man, I really need to nuke this pattern. Yeah, pretty awful.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=27024757ca27 Filed Bug 989178 for AboutHomeTest fragility.

Comment on attachment 8398215 [details] [diff] [review] Pre: introduce BaseRobocopTest, rework testBrowserProviderPerf. vN Review of attachment 8398215 [details] [diff] [review]: ----------------------------------------------------------------- SHIP IT ::: mobile/android/base/tests/testBrowserProviderPerf.java @@ +304,5 @@ > + // We don't have a good way for it to only watch changes related to > + // its current profile, nor is it convenient for us to launch a different > + // activity that doesn't depend on the DB. > + // We can fix this by: > + // * Adjusting the provider interface to allow a "don't notify" param. Great comment.

https://hg.mozilla.org/integration/fx-team/rev/88688294fe2f https://hg.mozilla.org/integration/fx-team/rev/a8af8fc51007 https://hg.mozilla.org/integration/fx-team/rev/49ca7b29bc93 https://hg.mozilla.org/integration/fx-team/rev/87148610fd2a

https://hg.mozilla.org/mozilla-central/rev/88688294fe2f https://hg.mozilla.org/mozilla-central/rev/a8af8fc51007 https://hg.mozilla.org/mozilla-central/rev/49ca7b29bc93 https://hg.mozilla.org/mozilla-central/rev/87148610fd2a

Comment on attachment 8394563 [details] [diff] [review] Part 1: ReadingListProvider and BrowserProvider should share DB accessors. v5 **Bulk uplift comment for all parts* [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 959290. User impact if declined: Occasional crashes due to a locked database, if some user actions are taken close together (e.g., loading a page then adding it to the reading list). Testing completed (on m-c, etc.): Been baking for a while. All tests pass. Risk to taking this patch (and alternatives if risky): The patch itself is pretty much an Eclipse-aided refactor, with one core logic change: the introduction of an intermediate class in the hierarchy to share a single Map between two ContentProviders. I had to do a little work to make the Talos test pass, because it wasn't testing the right thing. As such, I'd call this relatively low risk: it just looks scary. String or IDL/UUID changes made by this patch: None.

Comment on attachment 8394563 [details] [diff] [review] Part 1: ReadingListProvider and BrowserProvider should share DB accessors. v5 Trusting your call on risk, this is Aurora so we can backout in the coming weeks if we see any fallout (or in first week of Beta once more users are hitting this).

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e0063d89bb9 https://hg.mozilla.org/releases/mozilla-aurora/rev/1c429fbf41e5 https://hg.mozilla.org/releases/mozilla-aurora/rev/e38906f990ee https://hg.mozilla.org/releases/mozilla-aurora/rev/7a2168f9b7a0