Bug 1219323 removed seemingly unnecessary table-creation checks from BrowserDatabaseHelper. These checks are actually needed, at least for the reading-list table (we should probably double-check the rest of that commit too). If a DB is being upgraded from <= 17 to >= 23: 1) upgradeDatabaseFrom17to18 calls createReadingListTable 2) upgradeDatabaseFrom22to23 modifies/migrates the same table, it expects the table to be in the <= 22 format, i.e. include a "read" column. If we just created this table in the 17to18 migration, then the table is already in the new (>=23) format, there's no read column, and we crash.

Here's the interesting part of the crash-report, which is happening at: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java#1018 android.database.sqlite.SQLiteException: no such column: read (code 1): , while compiling: INSERT INTO tmp_rl (_id, url, title, resolved_title, resolved_url, excerpt, is_unread, is_deleted, guid, client_last_modified, added_on, content_status, marked_read_by, added_by) SELECT _id, url, title, CASE content_status WHEN 4 THEN title ELSE NULL END, CASE content_status WHEN 4 THEN url ELSE NULL END, excerpt, CASE read WHEN 1 THEN 0 ELSE 1 END, 0, NULL, modified, created, content_status, CASE read WHEN 1 THEN ? ELSE NULL END, ? FROM reading_list WHERE deleted = 0 at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method) at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:893) at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:504) at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:588) at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:58) at android.database.sqlite.SQLiteStatement.<init>(SQLiteStatement.java:31) at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:1697) at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:1678) at org.mozilla.gecko.db.BrowserDatabaseHelper.onUpgrade(BrowserDatabaseHelper.java:1210)

We should also add a selection of older DBs to testBrowserDatabaseHelperUpgrades to catch these sorts of issues in future.

[Tracking Requested - why for this release]: This will affect any users upgrading from really old versions of fennec (and it seems we got at least a few of those on aurora).

I've decided just to backout the entire change: only the reading-list changes were fundamentally incorrect, however having these checks is a pattern that we have to enforce if we want to avoid future crashes on migration. The migration story looks messy in general for these tables - I wonder if for every schema change we should add an _additional_ method that creates the new schema, i.e. we should keep the original schema to create in the migration step that first added a table, or the new schema for use in onCreate. This would avoid us having to add conditions to individual migrations. E.g. if we created a new RL table in 11->12: - implement createRLv12(), use this in upgrade11to12() _and_ onCreate() Then we change the schema in 14->15: - implement createRLv15(), run this in onCreate(), but upgrade11to12() still runs createRLv12(), and upgrade14to15() only has to care about the v12->v15 changes for that table (and doesn't have to care about whether the table was created in v15 format).

Removing these checks causes crashes when trying to upgrade a <= 17 db to >= 23: (A) upgradeDatabaseFrom17to18 calls createReadingListTable, and we create the table using the new (>=23 schema). This schema has no "read" column. (B) upgradeDatabaseFrom22to23 migrates the same table. As part of the migration it tries to select the "read" column, and we crash because that doesn't exist. This was prevented by an early return if didCreateReadingListTable was set. It looks like removing the didCreateTablsTable checks is OK because the migration only adds a foreign-key constraint, and doesn't depend on any columns that didn't exist in the initial version of the migration. However it seems wasteful to run the migration on a table that is already in the expected state. Moreover not having table-created checks is not safe in most cases, and having these checks should be the default pattern - especially in case any future migrations affecting the same table are added. Review commit: https://reviewboard.mozilla.org/r/44645/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44645/

Review commit: https://reviewboard.mozilla.org/r/44647/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44647/

Comment on attachment 8738647 [details] MozReview Request: Bug 1261907 - Reintroduce (necessary) table-created checks in BrowserDatabaseHelper r?nalexander https://reviewboard.mozilla.org/r/44645/#review41343 I'm fine with this approach. Fold the future comments in. There's a small typo in the commit message: "didCreateTablsTable" has an extra 'l'. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:199 (Diff revision 1) > BrowserContract.Clients.LAST_MODIFIED + " INTEGER," + > BrowserContract.Clients.DEVICE_TYPE + " TEXT" + > ");"); > } > > + private boolean didCreateTabsTable = false; nit: putting these close to the functions is not our style. I understand why in this case, but lift them to the top, like all the other flags. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:407 (Diff revision 1) > createFaviconsTable(db); > createThumbnailsTable(db); > createClientsTable(db); > createLocalClient(db); > createTabsTable(db, TABLE_TABS); > + didCreateTabsTable = true; Why are these two not just set in `createTabsTable`?

Comment on attachment 8738648 [details] MozReview Request: Bug 1261907 - Post: add comments explaining why we need table-created checks r?nalexander https://reviewboard.mozilla.org/r/44647/#review41347 Fold.

https://hg.mozilla.org/integration/fx-team/rev/546029898181bdafe1599a35226b23f9c2997856 Bug 1261907 - Reintroduce (necessary) table-created checks in BrowserDatabaseHelper r=nalexander

Comment on attachment 8738647 [details] MozReview Request: Bug 1261907 - Reintroduce (necessary) table-created checks in BrowserDatabaseHelper r?nalexander Approval Request Comment [Feature/regressing bug #]: Bug 1219323 [User impact if declined]: Startup crash if trying to upgrade from sufficiently old version of firefox (no recovery possible, profile would need to be wiped to recover). [Describe test coverage new/current, TreeHerder]: Manual testing. Patch landed on nightly 3 days ago. [Risks and why]: Low risk. This patch is a backout of Bug 1219323. [String/UUID change made/needed]: None Note: this is the 2nd top crasher on Aurora. There are at least some users who skipped a bunch of intermediate updates on Aurora, and I suspect there might be more on beta or release.

Tracked since it's a top crash.

Comment on attachment 8738647 [details] MozReview Request: Bug 1261907 - Reintroduce (necessary) table-created checks in BrowserDatabaseHelper r?nalexander Top-crash fix that has baked on Nightly for a few days, Aurora47+