caching bug in SQLiteDatabase causes invalid finalizer warnings

a bug in maintaining the cache caused these warnings. when the cache
is full, caching code in SQLiteDatabase dropped an entry from the cache
to accommodate the new one. and if the just-dropped one is not in use
that object got GC'ed and caused a finalizer warning. Calendar is one app
that didn't use ? for bindargs (sometimes) and noticed this bug in that app
Fix is to not add the new enry to cache if the cache is already full.
that will cause the app's close() to release the entry.

another common case where this finalizer warning occurs is when unittests run.
if the test does not close the database in tearDown(), it will cause
database object and the compiled sql statement cache within the database
obj get GC'ed which cause finalizer warnings.

2 files changed, 23 insertions, 39 deletions

diff --git a/core/java/android/database/sqlite/SQLiteDatabase.java b/core/java/android/database/sqlite/SQLiteDatabase.java
index 297498169e16..dfd40247ff38 100644
--- a/core/java/android/database/sqlite/SQLiteDatabase.java
+++ b/core/java/android/database/sqlite/SQLiteDatabase.java
@@ -261,7 +261,7 @@ public class SQLiteDatabase extends SQLiteClosable {
     public static final int MAX_SQL_CACHE_SIZE = 250;
     private int mMaxSqlCacheSize = MAX_SQL_CACHE_SIZE; // max cache size per Database instance
     private int mCacheFullWarnings;
-    private static final int MAX_WARNINGS_ON_CACHESIZE_CONDITION = 5;
+    private static final int MAX_WARNINGS_ON_CACHESIZE_CONDITION = 1;
 
     /** maintain stats about number of cache hits and misses */
     private int mNumCacheHits;
@@ -1909,43 +1909,28 @@ public class SQLiteDatabase extends SQLiteClosable {
             }
             // add this <sql, compiledStatement> to the cache
             if (mCompiledQueries.size() == mMaxSqlCacheSize) {
-                /* reached max cachesize. before adding new entry, remove an entry from the
-                 * cache. we don't want to wipe out the entire cache because of this:
-                 * GCing {@link SQLiteCompiledSql} requires call to sqlite3_finalize
-                 * JNI method. If entire cache is wiped out, it could cause a big GC activity
-                 * just because a (rogue) process is using the cache incorrectly.
+                /*
+                 * cache size of {@link #mMaxSqlCacheSize} is not enough for this app.
+                 * log a warning MAX_WARNINGS_ON_CACHESIZE_CONDITION times
+                 * chances are it is NOT using ? for bindargs - so caching is useless.
+                 * TODO: either let the callers set max cchesize for their app, or intelligently
+                 * figure out what should be cached for a given app.
                  */
-                Log.w(TAG, "Reached MAX size for compiled-sql statement cache for database " +
-                        getPath() + "; i.e., NO space for this sql statement in cache: " +
-                        sql + ". Please change your sql statements to use '?' for " +
-                        "bindargs, instead of using actual values");
-                
-                /* increment the number of times this warnings has been printed.
-                 * if this warning is printed too many times, clear the whole cache - the app
-                 * is doing something weird or incorrect and printing more warnings will only
-                 * flood the logfile.
-                 */
-                if (++mCacheFullWarnings > MAX_WARNINGS_ON_CACHESIZE_CONDITION) {
-                    mCacheFullWarnings = 0;
-                    // clear the cache
-                    mCompiledQueries.clear();
-                    Log.w(TAG, "Compiled-sql statement cache for database: " +
-                            getPath() + " hit MAX size-limit too many times. " +
-                            "Removing all compiled-sql statements from the cache.");
-                } else {
-                    // clear just a single entry from cache
-                    Set<String> keySet = mCompiledQueries.keySet();
-                    for (String s : keySet) {
-                        mCompiledQueries.remove(s);
-                        break;
-                    }
+                if (++mCacheFullWarnings == MAX_WARNINGS_ON_CACHESIZE_CONDITION) {
+                    Log.w(TAG, "Reached MAX size for compiled-sql statement cache for database " +
+                            getPath() + "; i.e., NO space for this sql statement in cache: " +
+                            sql + ". Please change your sql statements to use '?' for " +
+                            "bindargs, instead of using actual values");
+                }
+                // don't add this entry to cache
+            } else {
+                // cache is NOT full. add this to cache.
+                mCompiledQueries.put(sql, compiledStatement);
+                if (SQLiteDebug.DEBUG_SQL_CACHE) {
+                    Log.v(TAG, "|adding_sql_to_cache|" + getPath() + "|" +
+                            mCompiledQueries.size() + "|" + sql);
                 }
             }
-            mCompiledQueries.put(sql, compiledStatement);
-        }
-        if (SQLiteDebug.DEBUG_SQL_CACHE) {
-            Log.v(TAG, "|adding_sql_to_cache|" + getPath() + "|" + mCompiledQueries.size() + "|" +
-                    sql);
         }
         return;
     }
diff --git a/core/java/android/database/sqlite/SQLiteProgram.java b/core/java/android/database/sqlite/SQLiteProgram.java
index 389e15e6935a..663647393560 100644
--- a/core/java/android/database/sqlite/SQLiteProgram.java
+++ b/core/java/android/database/sqlite/SQLiteProgram.java
@@ -58,11 +58,10 @@ public abstract class SQLiteProgram extends SQLiteClosable {
         db.addSQLiteClosable(this);
         this.nHandle = db.mNativeHandle;
 
-        // shouldn't reuse compiled-plans of PRAGMA sql statements
-        // because sqlite returns OLD values if compiled-pragma-statements are reused
-        //TODO: remove this code when sqlite fixes it (and add tests too)
+        // only cache CRUD statements
         String prefixSql = mSql.substring(0, 6);
-        if (prefixSql.toLowerCase().startsWith("pragma")) {
+        if (!prefixSql.equalsIgnoreCase("INSERT") && !prefixSql.equalsIgnoreCase("UPDATE") &&
+                !prefixSql.equalsIgnoreCase("DELETE") && !prefixSql.equalsIgnoreCase("SELECT")) {
             mCompiledSql = new SQLiteCompiledSql(db, sql);
             nStatement = mCompiledSql.nStatement;
             // since it is not in the cache, no need to acquire() it.