Closed
Bug 1261907
Opened 9 years ago
Closed 9 years ago
Reintroduce reading-list table checks in BrowserDatabaseHelper
Categories
(Firefox for Android Graveyard :: Data Providers, defect)
Firefox for Android Graveyard
Data Providers
Tracking
(firefox47+ fixed, firefox48+ fixed)
RESOLVED
FIXED
Firefox 48
People
(Reporter: ahunt, Unassigned)
References
Details
(Keywords: topcrash)
Crash Data
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
nalexander
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
nalexander
:
review+
|
Details |
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•9 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•9 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•9 years ago
|
||
We should also add a selection of older DBs to testBrowserDatabaseHelperUpgrades to catch these sorts of issues in future.
Reporter | ||
Comment 3•9 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•9 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•9 years ago
|
||
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•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/44647/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44647/
Comment 7•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8738648 -
Flags: review?(nalexander) → review+
Comment 8•9 years ago
|
||
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•9 years ago
|
||
Reporter | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/546029898181bdafe1599a35226b23f9c2997856
Bug 1261907 - Reintroduce (necessary) table-created checks in BrowserDatabaseHelper r=nalexander
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Reporter | ||
Comment 12•9 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?
Keywords: topcrash
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+
Attachment #8738647 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•9 years ago
|
||
bugherder uplift |
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•