Trying to use an old phone that's been unused for a while. Looks like our DB upgrade paths are broken. D/GeckoAppShell(29713): [OPENFILE] org.mozilla.fennec_morbo(29713) : /data/data/org.mozilla.fennec_morbo/files/mozilla/e96k0c8a.default/.parentlock E/SQLiteLog(29713): (1) duplicate column name: content_status W/dalvikvm(29713): threadid=11: thread exiting with uncaught exception (group=0x41c8a700) E/GeckoCrashHandler(29713): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 3236 ("GeckoBackgroundThread") E/GeckoCrashHandler(29713): android.database.sqlite.SQLiteException: duplicate column name: content_status (code 1): , while compiling: ALTER TABLE reading_list ADD COLUMN content_status TINYINT DEFAULT 0 E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:1118) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:691) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:588) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:58) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteStatement.<init>(SQLiteStatement.java:31) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:1794) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:1725) E/GeckoCrashHandler(29713): at org.mozilla.gecko.db.BrowserDatabaseHelper.onUpgrade(BrowserDatabaseHelper.java:843) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:257) E/GeckoCrashHandler(29713): at android.database.sqlite.SQLiteOpenHelper.getReadableDatabase(SQLiteOpenHelper.java:188) E/GeckoCrashHandler(29713): at org.mozilla.gecko.db.AbstractPerProfileDatabaseProvider.getReadableDatabase(AbstractPerProfileDatabaseProvider.java:43) E/GeckoCrashHandler(29713): at org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:656) E/GeckoCrashHandler(29713): at android.content.ContentProvider.query(ContentProvider.java:744) E/GeckoCrashHandler(29713): at android.content.ContentProvider$Transport.query(ContentProvider.java:199) E/GeckoCrashHandler(29713): at android.content.ContentResolver.query(ContentResolver.java:417) E/GeckoCrashHandler(29713): at android.content.ContentResolver.query(ContentResolver.java:360) E/GeckoCrashHandler(29713): at org.mozilla.gecko.db.LocalBrowserDB.isBookmark(LocalBrowserDB.java:819) E/GeckoCrashHandler(29713): at org.mozilla.gecko.Tab$4.run(Tab.java:489) E/GeckoCrashHandler(29713): at android.os.Handler.handleCallback(Handler.java:730) E/GeckoCrashHandler(29713): at android.os.Handler.dispatchMessage(Handler.java:92) E/GeckoCrashHandler(29713): at android.os.Looper.loop(Looper.java:176) E/GeckoCrashHandler(29713): at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43) E/GeckoCrashHandler(29713): Main thread (1) stack: E/GeckoCrashHandler(29713): android.os.MessageQueue.nativePollOnce(Native Method) E/GeckoCrashHandler(29713): android.os.MessageQueue.next(MessageQueue.java:132) E/GeckoCrashHandler(29713): android.os.Looper.loop(Looper.java:138) E/GeckoCrashHandler(29713): android.app.ActivityThread.main(ActivityThread.java:5419) E/GeckoCrashHandler(29713): java.lang.reflect.Method.invokeNative(Native Method) E/GeckoCrashHandler(29713): java.lang.reflect.Method.invoke(Method.java:525) E/GeckoCrashHandler(29713): com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1046) E/GeckoCrashHandler(29713): com.android.internal.os.ZygoteInit.main(ZygoteInit.java:862) E/GeckoCrashHandler(29713): dalvik.system.NativeStart.main(Native Method)

My guess is upgrading from < DB version 17, because: private void upgradeDatabaseFrom17to18(SQLiteDatabase db) { debug("Moving reading list items from 'bookmarks' table to 'reading_list' table"); ... // Create 'reading_list' table createReadingListTable(db); which is wrong.

This will screw updates, so I'm pretty confident in marking it tracking 37. Thanks for grabbing this, Margaret! Happy to review.

/r/3325 - Bug 1128523 - Use original logic to create reading list table in old database migration path Pull down this commit: hg pull review -r 80144e01ddc323aad4c8e91791b0020e9652fea0

I'm not sure how to test this... is there a way to do that without reverting my tree to long ago to make a very old build? Or if we think this is good enough, could we just land it and then use an old Nightly to verify?

I'll test it on my affected device.

Comment on attachment 8559396 [details] MozReview Request: bz://1128523/margaret https://reviewboard.mozilla.org/r/3323/#review2735 I think ckitching would jump in here and say "you don't need to do this". If a user will be upgrading from 17 to the current version, *they'll be running our current code*. That means the simplest fix here is to alter the 17-18 migration to populate the current RL schema (as it currently does, but maybe not with every value), and then make the 21-22 migration only apply if you're upgrading from post-18. That is, if you install 22 on top of 19, we run 21-22. If you install 22 on top of 16, we don't run that 21-22 migration. Does that make as much sense to you as it does me? It should be as simple as a small change to the 18 migration to set states etc., and then: ``` case 22: if (oldVersion <= 17) { // We just created the right table in 17to18. Do nothing here. } else { upgradeDatabaseFrom21to22(db); } break; ```

Comment on attachment 8559396 [details] MozReview Request: bz://1128523/margaret /r/3325 - Bug 1128523 - Don't alter reading list table if it was created with the new schema in an earlier migration. r=rnewman" Pull down this commit: hg pull review -r 802983638f2935b07d1a1f4981214c81ef1ed452

https://reviewboard.mozilla.org/r/3325/#review2737 What do you think about also try-catch bracing the ALTER TABLE?

(In reply to Richard Newman [:rnewman] from comment #8) > https://reviewboard.mozilla.org/r/3325/#review2737 > > What do you think about also try-catch bracing the ALTER TABLE? Can't hurt, I can do that as well.

https://reviewboard.mozilla.org/r/3325/#review2741 FWIW confirming this fixes my crasher.

Comment on attachment 8559396 [details] MozReview Request: bz://1128523/margaret /r/3325 - Bug 1128523 - Don't alter reading list table if it was created with the new schema in an earlier migration. r=rnewman Pull down this commit: hg pull review -r 20709844cf7bc16365da67e0fdd8510ef30c3c96

(In reply to Gian-Carlo Pascutto [:gcp] from comment #10) > https://reviewboard.mozilla.org/r/3325/#review2741 > > FWIW confirming this fixes my crasher. Excellent, thanks for confirming.

Comment on attachment 8559396 [details] MozReview Request: bz://1128523/margaret https://reviewboard.mozilla.org/r/3323/#review2767 Ship It! ::: mobile/android/base/db/BrowserDatabaseHelper.java (Diff revision 3) > + Log.e(LOGTAG, "Error upgrading database from 21 to 22", e); Can we make this more specific? A SQLiteException (unless that only works on some API versions), and maybe even go so far as to add a check for .startsWith("duplicate column name")? And let's add a comment as to why we think this is safe to muffle: ``` // We're betting that an error here means that the table already has the column, so we're failing due to the duplicate column name. ```

Comment on attachment 8559396 [details] MozReview Request: bz://1128523/margaret Approval Request Comment [Feature/regressing bug #]: bug 1093172 [User impact if declined]: database migrations from much older versions of Fennec can cause crashes (I imagine this would be an infinite crash cycle) [Describe test coverage new/current, TreeHerder]: no automated test coverage, fix verified by gcp [Risks and why]: the main risk would be messing up the migration in some other way, but this patch adds pretty straightforward checks to avoid the problem, so I'm fairly confident there won't be other issues [String/UUID change made/needed]: none

Comment on attachment 8559396 [details] MozReview Request: bz://1128523/margaret lgtm. Thanks for caring about how we upgrade users on older versions. Aurora+