Merge "SQLiteOpenHelper should discard closed singleton database objects"

5 files changed, 59 insertions, 15 deletions

diff --git a/core/java/android/database/sqlite/SQLiteDatabase.java b/core/java/android/database/sqlite/SQLiteDatabase.java
index 54feb6d..b83d5ad 100644
--- a/core/java/android/database/sqlite/SQLiteDatabase.java
+++ b/core/java/android/database/sqlite/SQLiteDatabase.java
@@ -2066,11 +2066,9 @@ public class SQLiteDatabase extends SQLiteClosable {
      * mapping is NOT replaced with the new mapping).
      */
     /* package */ void addToCompiledQueries(String sql, SQLiteCompiledSql compiledStatement) {
-        SQLiteCompiledSql compiledSql = null;
         synchronized(mCompiledQueries) {
             // don't insert the new mapping if a mapping already exists
-            compiledSql = mCompiledQueries.get(sql);
-            if (compiledSql != null) {
+            if (mCompiledQueries.containsKey(sql)) {
                 return;
             }
 
diff --git a/core/java/android/database/sqlite/SQLiteOpenHelper.java b/core/java/android/database/sqlite/SQLiteOpenHelper.java
index d8cce21..ccf8d68 100644
--- a/core/java/android/database/sqlite/SQLiteOpenHelper.java
+++ b/core/java/android/database/sqlite/SQLiteOpenHelper.java
@@ -121,8 +121,13 @@ public abstract class SQLiteOpenHelper {
      * @return a read/write database object valid until {@link #close} is called
      */
     public synchronized SQLiteDatabase getWritableDatabase() {
-        if (mDatabase != null && mDatabase.isOpen() && !mDatabase.isReadOnly()) {
-            return mDatabase;  // The database is already open for business
+        if (mDatabase != null) {
+            if (!mDatabase.isOpen()) {
+                // darn! the user closed the database by calling mDatabase.close()
+                mDatabase = null;
+            } else if (!mDatabase.isReadOnly()) {
+                return mDatabase;  // The database is already open for business
+            }
         }
 
         if (mIsInitializing) {
@@ -207,8 +212,13 @@ public abstract class SQLiteOpenHelper {
      *     or {@link #close} is called.
      */
     public synchronized SQLiteDatabase getReadableDatabase() {
-        if (mDatabase != null && mDatabase.isOpen()) {
-            return mDatabase;  // The database is already open for business
+        if (mDatabase != null) {
+            if (!mDatabase.isOpen()) {
+                // darn! the user closed the database by calling mDatabase.close()
+                mDatabase = null;
+            } else {
+                return mDatabase;  // The database is already open for business
+            }
         }
 
         if (mIsInitializing) {
diff --git a/core/java/android/database/sqlite/SQLiteProgram.java b/core/java/android/database/sqlite/SQLiteProgram.java
index 231d8e6..9b7d823 100644
--- a/core/java/android/database/sqlite/SQLiteProgram.java
+++ b/core/java/android/database/sqlite/SQLiteProgram.java
@@ -158,18 +158,18 @@ public abstract class SQLiteProgram extends SQLiteClosable {
 
     @Override
     protected void onAllReferencesReleased() {
-        releaseCompiledSqlIfNotInCache();
+        release();
         mDatabase.removeSQLiteClosable(this);
         mDatabase.releaseReference();
     }
 
     @Override
     protected void onAllReferencesReleasedFromContainer() {
-        releaseCompiledSqlIfNotInCache();
+        release();
         mDatabase.releaseReference();
     }
 
-    /* package */ synchronized void releaseCompiledSqlIfNotInCache() {
+    /* package */ synchronized void release() {
         if (mCompiledSql == null) {
             return;
         }
diff --git a/core/java/android/database/sqlite/SQLiteStatement.java b/core/java/android/database/sqlite/SQLiteStatement.java
index 14de60f..dc07db0 100644
--- a/core/java/android/database/sqlite/SQLiteStatement.java
+++ b/core/java/android/database/sqlite/SQLiteStatement.java
@@ -265,7 +265,7 @@ public class SQLiteStatement extends SQLiteProgram
         clearBindings();
         // release the compiled sql statement so that the caller's SQLiteStatement no longer
         // has a hard reference to a database object that may get deallocated at any point.
-        releaseCompiledSqlIfNotInCache();
+        release();
         // restore the database connection handle to the original value
         mDatabase = mOrigDb;
         nHandle = mDatabase.mNativeHandle;
diff --git a/core/tests/coretests/src/android/database/sqlite/SQLiteDatabaseTest.java b/core/tests/coretests/src/android/database/sqlite/SQLiteDatabaseTest.java
index dc5613e..86eda71 100644
--- a/core/tests/coretests/src/android/database/sqlite/SQLiteDatabaseTest.java
+++ b/core/tests/coretests/src/android/database/sqlite/SQLiteDatabaseTest.java
@@ -19,8 +19,11 @@ package android.database.sqlite;
 import android.content.ContentValues;
 import android.content.Context;
 import android.database.Cursor;
+import android.database.DatabaseErrorHandler;
 import android.database.DatabaseUtils;
+import android.database.DefaultDatabaseErrorHandler;
 import android.database.sqlite.SQLiteDatabase;
+import android.database.sqlite.SQLiteDatabase.CursorFactory;
 import android.database.sqlite.SQLiteStatement;
 import android.test.AndroidTestCase;
 import android.test.suitebuilder.annotation.LargeTest;
@@ -41,6 +44,7 @@ public class SQLiteDatabaseTest extends AndroidTestCase {
     private static final int INSERT = 1;
     private static final int UPDATE = 2;
     private static final int DELETE = 3;
+    private static final String DB_NAME = "database_test.db";
 
     @Override
     protected void setUp() throws Exception {
@@ -61,7 +65,7 @@ public class SQLiteDatabaseTest extends AndroidTestCase {
 
     private void dbSetUp() throws Exception {
         File dbDir = getContext().getDir(this.getClass().getName(), Context.MODE_PRIVATE);
-        mDatabaseFile = new File(dbDir, "database_test.db");
+        mDatabaseFile = new File(dbDir, DB_NAME);
         if (mDatabaseFile.exists()) {
             mDatabaseFile.delete();
         }
@@ -860,7 +864,7 @@ public class SQLiteDatabaseTest extends AndroidTestCase {
                     "select count(*) from " + TEST_TABLE, null));
             // query in a different thread. but since the transaction is started using
             // execSQ() instead of beginTransaction(), cursor's query is considered part of
-            // the same ransaction - and hence it should see the above inserted row
+            // the same transaction - and hence it should see the above inserted row
             Thread t = new Thread() {
                 @Override public void run() {
                     c1.requery();
@@ -878,11 +882,11 @@ public class SQLiteDatabaseTest extends AndroidTestCase {
 
     /**
      * This test is same as {@link #testTransactionAndWalInterplay2()} except the following:
-     * instead of commiting the data, do rollback and make sure the data seen by the query
+     * instead of committing the data, do rollback and make sure the data seen by the query
      * within the transaction is now gone.
      */
     @SmallTest
-    public void testTransactionAndWalInterplay3() throws InterruptedException {
+    public void testTransactionAndWalInterplay3() {
         createTableAndClearCache();
         mDatabase.execSQL("INSERT into " + TEST_TABLE + " values(10, 999);");
         String sql = "select * from " + TEST_TABLE;
@@ -909,4 +913,36 @@ public class SQLiteDatabaseTest extends AndroidTestCase {
                 "select count(*) from " + TEST_TABLE, null));
         c.close();
     }
+
+    /**
+     * http://b/issue?id=2943028
+     * SQLiteOpenHelper maintains a Singleton even if it is in bad state.
+     */
+    @SmallTest
+    public void testCloseAndReopen() {
+        mDatabase.close();
+        TestOpenHelper helper = new TestOpenHelper(getContext(), DB_NAME, null,
+                CURRENT_DATABASE_VERSION, new DefaultDatabaseErrorHandler());
+        mDatabase = helper.getWritableDatabase();
+        createTableAndClearCache();
+        mDatabase.execSQL("INSERT into " + TEST_TABLE + " values(10, 999);");
+        Cursor c = mDatabase.query(TEST_TABLE, new String[]{"i", "j"}, null, null, null, null, null);
+        assertEquals(1, c.getCount());
+        c.close();
+        mDatabase.close();
+        assertFalse(mDatabase.isOpen());
+        mDatabase = helper.getReadableDatabase();
+        assertTrue(mDatabase.isOpen());
+        c = mDatabase.query(TEST_TABLE, new String[]{"i", "j"}, null, null, null, null, null);
+        assertEquals(1, c.getCount());
+        c.close();
+    }
+    private class TestOpenHelper extends SQLiteOpenHelper {
+        public TestOpenHelper(Context context, String name, CursorFactory factory, int version,
+                DatabaseErrorHandler errorHandler) {
+            super(context, name, factory, version, errorHandler);
+        }
+        @Override public void onCreate(SQLiteDatabase db) {}
+        @Override public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {}
+    }
 }