Closed Bug 1128523 Opened 5 years ago Closed 5 years ago

Duplicate column name: content_status while compiling: ALTER TABLE reading_list

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed
fennec 37+ ---

People

(Reporter: gcp, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1093172
tracking-fennec: --- → ?
Hardware: ARM → All
Assignee: nobody → margaret.leibovic
This will screw updates, so I'm pretty confident in marking it tracking 37.

Thanks for grabbing this, Margaret! Happy to review.
Status: NEW → ASSIGNED
tracking-fennec: ? → 37+
/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
Attachment #8559396 - Flags: review?(rnewman)
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;
```
Attachment #8559396 - Flags: review?(rnewman)
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
Attachment #8559396 - Flags: review?(rnewman)
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.
Attachment #8559396 - Flags: review?(rnewman) → review+
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.
```
https://hg.mozilla.org/mozilla-central/rev/2b2130aec3cd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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
Attachment #8559396 - Flags: approval-mozilla-aurora?
Comment on attachment 8559396 [details]
MozReview Request: bz://1128523/margaret

lgtm. Thanks for caring about how we upgrade users on older versions. Aurora+
Attachment #8559396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8559396 - Attachment is obsolete: true
Attachment #8619288 - Flags: review+
You need to log in before you can comment on or make changes to this bug.