1261907 - Reintroduce reading-list table checks in BrowserDatabaseHelper

Status: RESOLVED FIXED

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

Description

[Andrzej Hunt :ahunt]

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.

Comment 1

[Andrzej Hunt :ahunt]

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)

Comment 2

[Andrzej Hunt :ahunt]

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

Comment 3

[Andrzej Hunt :ahunt]

[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).

Comment 4

[Andrzej Hunt :ahunt]

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).

Comment 5

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1261907 - Reintroduce (necessary) table-created checks in BrowserDatabaseHelper r?nalexander

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/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1261907 - Post: add comments explaining why we need table-created checks r?nalexander

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

Comment 7

[Nick Alexander :nalexander [he/him]]

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 8

[Nick Alexander :nalexander [he/him]]

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.

Comment 9

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a6717d1419e

Comment 10

[Andrzej Hunt :ahunt]

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

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Andrzej Hunt :ahunt]

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.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked since it's a top crash.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90c1f55fcb3f