Reintroduce reading-list table checks in BrowserDatabaseHelper

RESOLVED FIXED in Firefox 47

Status

()

Firefox for Android
Data Providers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahunt, Unassigned)

Tracking

({topcrash})

Trunk
Firefox 48
topcrash
Points:
---

Firefox Tracking Flags

(firefox47+ fixed, firefox48+ fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Crash Signature: FennecAndroid 47.0a2 Crash Report [@ android.database.sqlite.SQLiteException: at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method) ]
status-firefox47: --- → affected
Depends on: 1219323
(Reporter)

Comment 1

2 years ago
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)
(Reporter)

Comment 2

2 years ago
We should also add a selection of older DBs to testBrowserDatabaseHelperUpgrades to catch these sorts of issues in future.
(Reporter)

Comment 3

2 years ago
[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).
tracking-firefox47: --- → ?
tracking-firefox48: --- → ?
(Reporter)

Comment 4

2 years ago
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).
(Reporter)

Comment 5

2 years ago
Created attachment 8738647 [details]
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/
Attachment #8738647 - Flags: review?(nalexander)
Attachment #8738648 - Flags: review?(nalexander)
(Reporter)

Comment 6

2 years ago
Created attachment 8738648 [details]
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 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`?
Attachment #8738647 - Flags: review?(nalexander) → review+
Attachment #8738648 - Flags: review?(nalexander) → review+
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.
(Reporter)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a6717d1419e
(Reporter)

Comment 10

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/546029898181
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(Reporter)

Comment 12

2 years ago
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.
Attachment #8738647 - Flags: approval-mozilla-aurora?

Updated

2 years ago
Keywords: topcrash
Tracked since it's a top crash.
tracking-firefox47: ? → +
tracking-firefox48: ? → +
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+
Attachment #8738647 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/90c1f55fcb3f
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.