975841 - crash in org.mozilla.gecko.sqlite.SQLiteBridgeException: Can''t prepare statement: no such column: filter at org.mozilla.gecko.sqlite.SQLiteBridge.sqliteCallWithDb(Native Method)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-88342b4c-12c3-4368-8ff6-1a1282140222.
=============================================================

org.mozilla.gecko.sqlite.SQLiteBridgeException: Can't prepare statement: no such column: filter
	at org.mozilla.gecko.sqlite.SQLiteBridge.sqliteCallWithDb(Native Method)
	at org.mozilla.gecko.sqlite.SQLiteBridge.internalQuery(SQLiteBridge.java:235)
	at org.mozilla.gecko.sqlite.SQLiteBridge.rawQuery(SQLiteBridge.java:128)
	at org.mozilla.gecko.sqlite.SQLiteBridge.query$18cfc304(SQLiteBridge.java:118)
	at org.mozilla.gecko.db.SQLiteBridgeContentProvider.query(SQLiteBridgeContentProvider.java:377)
	at org.mozilla.gecko.db.HomeProvider.query(HomeProvider.java:83)
	at android.content.ContentProvider.query(ContentProvider.java:756)
	at android.content.ContentProvider$Transport.query(ContentProvider.java:211)
	at android.content.ContentResolver.query(ContentResolver.java:428)
	at android.content.ContentResolver.query(ContentResolver.java:371)
	at org.mozilla.gecko.home.DynamicPanel$PanelDatasetLoader.loadCursor(DynamicPanel.java:237)
	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:49)
	at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:31)
	at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:240)
	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:51)
	at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:40)
	at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:123)
	at java.util.concurrent.FutureTask.run(FutureTask.java:234)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1080)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:573)
	at java.lang.Thread.run(Thread.java:849)

Comment 1

[Lucas Rocha (:lucasr)]

We added a column to the table in 942295 without any schema migration (I just assumed it's fine to just break stuff for now). But maybe we should do a proper migration here for convenience in Nightly? Margaret, thoughts?

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Bootstrap schema upgrade path in HomeProvide.jsm

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8380625 [details] [diff] [review]
Bootstrap schema upgrade path in HomeProvide.jsm

Here's an initial take on the schema migration path for HomeProvider.jsm.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8380625 [details] [diff] [review]
Bootstrap schema upgrade path in HomeProvide.jsm

Review of attachment 8380625 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good, I'm just a bit concerned about some of the promises logic.

I would also love to see a testcase for this added to testHomeProvider.js. As a basic test, we could manually create an old database, then test to make sure the schema version is changed after making a basic storage API call. To be more advanced, we could check to make sure the new schema has the columns we expect.

Also, as a more general comment, I didn't originally think we should support schema migrations on Nightly, but this blow-it-away-and-start-over approach is nice, and we can just keep doing this if the schema continues to change.

::: mobile/android/modules/HomeProvider.jsm
@@ +230,5 @@
> +      // version is undefined.
> +      if (dbVersion === 0) {
> +        yield createDatabase(db);
> +      } else if (dbVersion < SCHEMA_VERSION) {
> +        yield upgradeDatabase(db, dbVersion, SCHEMA_VERSION);

These helper methods don't return promises, so I don't think it's correct to use yield here like this. Instead, we should be using yield where we actually make the db.execute calls, since it looks like we can end up with unresolved promises in there.

To make this less confusing, we could also have the helper methods themselves return new tasks.

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Bootstrap schema upgrade path in HomeProvide.jsm

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I wouldn't mind using this to test migrations, but we could drop it for initial release. No need to add a migration pre-release.

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 8380625 [details] [diff] [review]
> Bootstrap schema upgrade path in HomeProvide.jsm
> 
> Review of attachment 8380625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking good, I'm just a bit concerned about some of the promises
> logic.
> 
> I would also love to see a testcase for this added to testHomeProvider.js.
> As a basic test, we could manually create an old database, then test to make
> sure the schema version is changed after making a basic storage API call. To
> be more advanced, we could check to make sure the new schema has the columns
> we expect.

I'd love to add tests for our migrations here (same for BrowserProvider stuff). But where would we place such database file in order to copy it to the expected location before the test runs? The test infrastructure side of things is usually the most painful ones unfortunately...

> Also, as a more general comment, I didn't originally think we should support
> schema migrations on Nightly, but this blow-it-away-and-start-over approach
> is nice, and we can just keep doing this if the schema continues to change.

Yeah, I think we should put this in the add-on API docs: your add-on should be able to re-fetch data as needed if HomeProvider gets cleared for some reason.

> ::: mobile/android/modules/HomeProvider.jsm
> @@ +230,5 @@
> > +      // version is undefined.
> > +      if (dbVersion === 0) {
> > +        yield createDatabase(db);
> > +      } else if (dbVersion < SCHEMA_VERSION) {
> > +        yield upgradeDatabase(db, dbVersion, SCHEMA_VERSION);
> 
> These helper methods don't return promises, so I don't think it's correct to
> use yield here like this. Instead, we should be using yield where we
> actually make the db.execute calls, since it looks like we can end up with
> unresolved promises in there.
> 
> To make this less confusing, we could also have the helper methods
> themselves return new tasks.

I actually intended to do this (helper methods to return new tasks) originally but forgot to do so before submitting the patch :-)

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> I wouldn't mind using this to test migrations, but we could drop it for
> initial release. No need to add a migration pre-release.

Yeah, the only reason I wrote this patch is because this is causing crashers for people who are testing add-ons. And re-installing Nightly can be a little annoying. And this code should be useful in the long-term to implement whatever actual migrations we end up needing to do.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8384674 [details] [diff] [review]
Bootstrap schema upgrade path in HomeProvide.jsm

Review of attachment 8384674 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

::: mobile/android/modules/HomeProvider.jsm
@@ +17,4 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  /*
>   * XXX: Add migration logic to getDatabaseConnection if you ever rev SCHEMA_VERSION.

You can probably get rid of this, now that we're introducing a migration path.

@@ +19,5 @@
>  /*
>   * XXX: Add migration logic to getDatabaseConnection if you ever rev SCHEMA_VERSION.
>   *
>   * SCHEMA_VERSION history:
>   *   1: Create HomeProvider (bug 942288)

You should add a note in here about why we created version 2 :)

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #10)
> Comment on attachment 8384674 [details] [diff] [review]
> Bootstrap schema upgrade path in HomeProvide.jsm
> 
> Review of attachment 8384674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great!
> 
> ::: mobile/android/modules/HomeProvider.jsm
> @@ +17,4 @@
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  
> >  /*
> >   * XXX: Add migration logic to getDatabaseConnection if you ever rev SCHEMA_VERSION.
> 
> You can probably get rid of this, now that we're introducing a migration
> path.

Done.

> @@ +19,5 @@
> >  /*
> >   * XXX: Add migration logic to getDatabaseConnection if you ever rev SCHEMA_VERSION.
> >   *
> >   * SCHEMA_VERSION history:
> >   *   1: Create HomeProvider (bug 942288)
> 
> You should add a note in here about why we created version 2 :)

Done.

Comment 12

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/da6253259487

Comment 13

[Ryan VanderMeulen (PTO, back 6-April)]

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