Bug 1261907 - Reintroduce (necessary) table-created checks in BrowserDatabaseHelper r?nalexander draft
authorAndrzej Hunt <ahunt@mozilla.com>
Wed, 06 Apr 2016 10:48:00 -0700
changeset 348126 639e600432ea68d09c9d10810fb5a3acfa7054e6
parent 348125 419922c1f15f5f3edeb516e9922928e1d6b02910
child 348127 8d19d5ebd29561232d252762d426a909c429d497
push id14755
push userahunt@mozilla.com
push dateWed, 06 Apr 2016 17:49:19 +0000
reviewersnalexander
bugs1261907
milestone48.0a1
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. MozReview-Commit-ID: 4j1PlQc6LLN
mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
@@ -191,16 +191,17 @@ public final class BrowserDatabaseHelper
         db.execSQL("CREATE TABLE " + TABLE_CLIENTS + "(" +
                 BrowserContract.Clients.GUID + " TEXT PRIMARY KEY," +
                 BrowserContract.Clients.NAME + " TEXT," +
                 BrowserContract.Clients.LAST_MODIFIED + " INTEGER," +
                 BrowserContract.Clients.DEVICE_TYPE + " TEXT" +
                 ");");
     }
 
+    private boolean didCreateTabsTable = false;
     private void createTabsTable(SQLiteDatabase db, final String tableName) {
         debug("Creating tabs.db: " + db.getPath());
         debug("Creating " + tableName + " table");
 
         // Table for each tab on any client.
         db.execSQL("CREATE TABLE " + tableName + "(" +
                 BrowserContract.Tabs._ID + " INTEGER PRIMARY KEY AUTOINCREMENT," +
                 BrowserContract.Tabs.CLIENT_GUID + " TEXT," +
@@ -398,29 +399,31 @@ public final class BrowserDatabaseHelper
 
         createBookmarksTable(db);
         createHistoryTable(db);
         createFaviconsTable(db);
         createThumbnailsTable(db);
         createClientsTable(db);
         createLocalClient(db);
         createTabsTable(db, TABLE_TABS);
+        didCreateTabsTable = true;
         createTabsTableIndices(db, TABLE_TABS);
 
 
         createBookmarksWithFaviconsView(db);
         createHistoryWithFaviconsView(db);
         createCombinedViewOn19(db);
 
         createOrUpdateSpecialFolder(db, Bookmarks.PLACES_FOLDER_GUID,
             R.string.bookmarks_folder_places, 0);
 
         createOrUpdateAllSpecialFolders(db);
         createSearchHistoryTable(db);
         createReadingListTable(db, TABLE_READING_LIST);
+        didCreateCurrentReadingListTable = true;      // Mostly correct, in the absence of transactions.
         createReadingListIndices(db, TABLE_READING_LIST);
         createUrlAnnotationsTable(db);
         createNumbersTable(db);
 
         createDeletedLoginsTable(db, TABLE_DELETED_LOGINS);
         createDisabledHostsTable(db, TABLE_DISABLED_HOSTS);
         createLoginsTable(db, TABLE_LOGINS);
         createLoginsTableIndices(db, TABLE_LOGINS);
@@ -430,16 +433,17 @@ public final class BrowserDatabaseHelper
      * Copies the tabs and clients tables out of the given tabs.db file and into the destinationDB.
      *
      * @param tabsDBFile Path to existing tabs.db.
      * @param destinationDB The destination database.
      */
     public void copyTabsDB(File tabsDBFile, SQLiteDatabase destinationDB) {
         createClientsTable(destinationDB);
         createTabsTable(destinationDB, TABLE_TABS);
+        didCreateTabsTable = true;
         createTabsTableIndices(destinationDB, TABLE_TABS);
 
         SQLiteDatabase oldTabsDB = null;
         try {
             oldTabsDB = SQLiteDatabase.openDatabase(tabsDBFile.getPath(), null, SQLiteDatabase.OPEN_READONLY);
 
             if (!DBUtils.copyTable(oldTabsDB, TABLE_CLIENTS, destinationDB, TABLE_CLIENTS)) {
                 Log.e(LOGTAG, "Failed to migrate table clients; ignoring.");
@@ -465,16 +469,17 @@ public final class BrowserDatabaseHelper
                     SearchHistory.QUERY + " TEXT UNIQUE NOT NULL, " +
                     SearchHistory.DATE_LAST_VISITED + " INTEGER, " +
                     SearchHistory.VISITS + " INTEGER ) ");
 
         db.execSQL("CREATE INDEX idx_search_history_last_visited ON " +
                 SearchHistory.TABLE_NAME + "(" + SearchHistory.DATE_LAST_VISITED + ")");
     }
 
+    private boolean didCreateCurrentReadingListTable = false;
     private void createReadingListTable(final SQLiteDatabase db, final String tableName) {
         debug("Creating " + TABLE_READING_LIST + " table");
 
         db.execSQL("CREATE TABLE " + tableName + "(" +
                    ReadingListItems._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " +
                    ReadingListItems.GUID + " TEXT UNIQUE, " +                          // Server-assigned.
 
                    ReadingListItems.CONTENT_STATUS + " TINYINT NOT NULL DEFAULT " + ReadingListItems.STATUS_UNFETCHED + ", " +
@@ -968,16 +973,17 @@ public final class BrowserDatabaseHelper
                       Bookmarks._ID + " = ? ",
                       new String[] { String.valueOf(Bookmarks.FIXED_READING_LIST_ID) });
 
             // Create indices.
             createReadingListIndices(db, TABLE_READING_LIST);
 
             // Done.
             db.setTransactionSuccessful();
+            didCreateCurrentReadingListTable = true;
 
         } catch (SQLException e) {
             Log.e(LOGTAG, "Error migrating reading list items", e);
         } finally {
             db.endTransaction();
         }
     }
 
@@ -994,16 +1000,21 @@ public final class BrowserDatabaseHelper
                    " WHERE " + Bookmarks.TYPE + " IS NULL");
     }
 
     private void upgradeDatabaseFrom19to20(SQLiteDatabase db) {
         createSearchHistoryTable(db);
     }
 
     private void upgradeDatabaseFrom21to22(SQLiteDatabase db) {
+        if (didCreateCurrentReadingListTable) {
+            debug("No need to add CONTENT_STATUS to reading list; we just created with the current schema.");
+            return;
+        }
+
         debug("Adding CONTENT_STATUS column to reading list table.");
 
         try {
             db.execSQL("ALTER TABLE " + TABLE_READING_LIST +
                        " ADD COLUMN " + ReadingListItems.CONTENT_STATUS +
                        " TINYINT DEFAULT " + ReadingListItems.STATUS_UNFETCHED);
 
             db.execSQL("CREATE INDEX reading_list_content_status ON " + TABLE_READING_LIST + "("
@@ -1011,16 +1022,21 @@ public final class BrowserDatabaseHelper
         } catch (SQLiteException e) {
             // 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.
             Log.e(LOGTAG, "Error upgrading database from 21 to 22", e);
         }
     }
 
     private void upgradeDatabaseFrom22to23(SQLiteDatabase db) {
+        if (didCreateCurrentReadingListTable) {
+            debug("No need to rev reading list schema; we just created with the current schema.");
+            return;
+        }
+
         debug("Rewriting reading list table.");
         createReadingListTable(db, "tmp_rl");
 
         // Remove indexes. We don't need them now, and we'll be throwing away the table.
         db.execSQL("DROP INDEX IF EXISTS reading_list_url");
         db.execSQL("DROP INDEX IF EXISTS reading_list_guid");
         db.execSQL("DROP INDEX IF EXISTS reading_list_content_status");
 
@@ -1080,16 +1096,21 @@ public final class BrowserDatabaseHelper
                 FileUtils.delete(file);
             } catch (Exception e) {
                 Log.e(LOGTAG, "Exception occurred while trying to delete " + file.getPath() + "; ignoring.", e);
             }
         }
     }
 
     private void upgradeDatabaseFrom24to25(SQLiteDatabase db) {
+        if (didCreateTabsTable) {
+            debug("No need to rev tabs schema; foreign key constraint exists.");
+            return;
+        }
+
         debug("Rewriting tabs table.");
         createTabsTable(db, "tmp_tabs");
 
         // Remove indexes. We don't need them now, and we'll be throwing away the table.
         db.execSQL("DROP INDEX IF EXISTS " + TabsProvider.INDEX_TABS_GUID);
         db.execSQL("DROP INDEX IF EXISTS " + TabsProvider.INDEX_TABS_POSITION);
 
         db.execSQL("INSERT INTO tmp_tabs (" +
@@ -1106,16 +1127,17 @@ public final class BrowserDatabaseHelper
                         "SELECT " +
                         "_id, client_guid, title, url, history, favicon, last_used, position" +
                         " FROM " + TABLE_TABS);
 
         // Now switch these tables over and recreate the indices.
         db.execSQL("DROP TABLE " + TABLE_TABS);
         db.execSQL("ALTER TABLE tmp_tabs RENAME TO " + TABLE_TABS);
         createTabsTableIndices(db, TABLE_TABS);
+        didCreateTabsTable = true;
     }
 
     private void upgradeDatabaseFrom25to26(SQLiteDatabase db) {
         debug("Dropping unnecessary indices");
         db.execSQL("DROP INDEX IF EXISTS clients_guid_index");
         db.execSQL("DROP INDEX IF EXISTS thumbnails_url_index");
         db.execSQL("DROP INDEX IF EXISTS favicons_url_index");
     }