Merge branch 'fix-265--database-locked-crash' into 'master'

Fix 265  database locked crash (I'm pretty sure)

Previously, all of the various subclasses of FDroidProvider would create
their own database open helper in their respective `onCreate()` methods.
This seemed to be the cause of the multiple database locked exceptions.

Various online articles/SO posts/etc helped come to this conclusion:
 * http://stackoverflow.com/a/3689883
 * http://stackoverflow.com/a/8888606
 * https://web.archive.org/web/20150709074733/http://www.dmytrodanylyk.com/pages/blog/concurrent-database.html

This should fix #265.

In the process, also did away with the two `read()` and `write()` methods
that returned a "readable" and "writeable" database respectively. It turns
out that it doesn't quite do what I originally thought. There is not much
benefit to specifying to the database helper that you want a readable/writeable
database. In fact, it is often the case that a call to `read()` would most
likely have returned the same instance that is returned by `write()`. The
semantics of them were therefore broken, and they've been replaced with
`db()`.

See merge request !196

F-Droid/src/org/fdroid/fdroid/data/ApkProvider.java

@@ -486,7 +486,7 @@ public class ApkProvider extends FDroidProvider {
         queryBuilder.addSelection(query.getSelection());
         queryBuilder.addOrderBy(sortOrder);
 
-        Cursor cursor = read().rawQuery(queryBuilder.toString(), query.getArgs());
+        Cursor cursor = db().rawQuery(queryBuilder.toString(), query.getArgs());
         cursor.setNotificationUri(getContext().getContentResolver(), uri);
         return cursor;
     }
@@ -507,7 +507,7 @@ public class ApkProvider extends FDroidProvider {
     public Uri insert(Uri uri, ContentValues values) {
         removeRepoFields(values);
         validateFields(DataColumns.ALL, values);
-        write().insertOrThrow(getTableName(), null, values);
+        db().insertOrThrow(getTableName(), null, values);
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }
@@ -553,7 +553,7 @@ public class ApkProvider extends FDroidProvider {
                 throw new UnsupportedOperationException("Invalid URI for apk content provider: " + uri);
         }
 
-        int rowsAffected = write().delete(getTableName(), query.getSelection(), query.getArgs());
+        int rowsAffected = db().delete(getTableName(), query.getSelection(), query.getArgs());
         getContext().getContentResolver().notifyChange(uri, null);
         return rowsAffected;
 
@@ -574,7 +574,7 @@ public class ApkProvider extends FDroidProvider {
         QuerySelection query = new QuerySelection(where, whereArgs);
         query = query.add(querySingle(uri));
 
-        int numRows = write().update(getTableName(), values, query.getSelection(), query.getArgs());
+        int numRows = db().update(getTableName(), values, query.getSelection(), query.getArgs());
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }

F-Droid/src/org/fdroid/fdroid/data/AppProvider.java

@@ -822,7 +822,7 @@ public class AppProvider extends FDroidProvider {
         query.addFields(projection); // TODO: Make the order of addFields/addSelection not dependent on each other...
         query.addOrderBy(sortOrder);
 
-        Cursor cursor = read().rawQuery(query.toString(), selection.getArgs());
+        Cursor cursor = db().rawQuery(query.toString(), selection.getArgs());
         cursor.setNotificationUri(getContext().getContentResolver(), uri);
         return cursor;
     }
@@ -842,14 +842,14 @@ public class AppProvider extends FDroidProvider {
 
         }
 
-        int count = write().delete(getTableName(), query.getSelection(), query.getArgs());
+        int count = db().delete(getTableName(), query.getSelection(), query.getArgs());
         getContext().getContentResolver().notifyChange(uri, null);
         return count;
     }
 
     @Override
     public Uri insert(Uri uri, ContentValues values) {
-        write().insertOrThrow(getTableName(), null, values);
+        db().insertOrThrow(getTableName(), null, values);
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }
@@ -873,7 +873,7 @@ public class AppProvider extends FDroidProvider {
                 throw new UnsupportedOperationException("Update not supported for " + uri + ".");
 
         }
-        int count = write().update(getTableName(), values, query.getSelection(), query.getArgs());
+        int count = db().update(getTableName(), values, query.getSelection(), query.getArgs());
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }
@@ -884,7 +884,7 @@ public class AppProvider extends FDroidProvider {
         updateCompatibleFlags();
         updateSuggestedFromUpstream();
         updateSuggestedFromLatest();
-        updateIconUrls(getContext(), write(), getTableName(), getApkTableName());
+        updateIconUrls(getContext(), db(), getTableName(), getApkTableName());
     }
 
     /**
@@ -911,7 +911,7 @@ public class AppProvider extends FDroidProvider {
                 " FROM " + apk +
                 " WHERE " + apk + ".id = " + app + ".id );";
 
-        write().execSQL(updateSql);
+        db().execSQL(updateSql);
     }
 
     /**
@@ -952,7 +952,7 @@ public class AppProvider extends FDroidProvider {
                     " ( " + app + ".compatible = 0 OR " + apk + ".compatible = 1 ) ) " +
                 " WHERE upstreamVercode > 0 ";
 
-        write().execSQL(updateSql);
+        db().execSQL(updateSql);
     }
 
     /**
@@ -990,7 +990,7 @@ public class AppProvider extends FDroidProvider {
                     " ( " + app + ".compatible = 0 OR " + apk + ".compatible = 1 ) ) " +
                 " WHERE upstreamVercode = 0 OR upstreamVercode IS NULL OR suggestedVercode IS NULL ";
 
-        write().execSQL(updateSql);
+        db().execSQL(updateSql);
     }
 
     /**

F-Droid/src/org/fdroid/fdroid/data/FDroidProvider.java

@@ -5,11 +5,15 @@ import android.content.ContentProvider;
 import android.content.ContentProviderOperation;
 import android.content.ContentProviderResult;
 import android.content.ContentValues;
+import android.content.Context;
 import android.content.OperationApplicationException;
 import android.content.UriMatcher;
 import android.database.sqlite.SQLiteDatabase;
 import android.net.Uri;
 import android.os.Build;
+import android.support.annotation.NonNull;
+
+import org.fdroid.fdroid.Utils;
 
 import java.util.ArrayList;
 import java.util.HashSet;
@@ -18,12 +22,14 @@ import java.util.Set;
 
 public abstract class FDroidProvider extends ContentProvider {
 
+    private static final String TAG = "FDroidProvider";
+
     public static final String AUTHORITY = "org.fdroid.fdroid.data";
 
     protected static final int CODE_LIST   = 1;
     protected static final int CODE_SINGLE = 2;
 
-    private DBHelper dbHelper;
+    private static DBHelper dbHelper;
 
     private boolean isApplyingBatch;
 
@@ -48,42 +54,50 @@ public abstract class FDroidProvider extends ContentProvider {
         return this.isApplyingBatch;
     }
 
+    @NonNull
     @Override
-    public ContentProviderResult[] applyBatch(ArrayList<ContentProviderOperation> operations)
+    public ContentProviderResult[] applyBatch(@NonNull ArrayList<ContentProviderOperation> operations)
         throws OperationApplicationException {
         ContentProviderResult[] result = null;
         isApplyingBatch = true;
-        write().beginTransaction();
+        final SQLiteDatabase db = db();
+        db.beginTransaction();
         try {
             result = super.applyBatch(operations);
-            write().setTransactionSuccessful();
+            db.setTransactionSuccessful();
         } finally {
-            write().endTransaction();
+            db.endTransaction();
             isApplyingBatch = false;
         }
         return result;
     }
 
-    @Override
+    /**
-    public boolean onCreate() {
+     * Only used for testing. Not quite sure how to mock a singleton variable like this.
-        dbHelper = new DBHelper(getContext());
+     */
-        return true;
+    public static void clearDbHelperSingleton() {
+        dbHelper = null;
     }
 
-    protected final DBHelper db() {
+    private static synchronized DBHelper getOrCreateDb(Context context) {
+        if (dbHelper == null) {
+            Utils.debugLog(TAG, "First time accessing database, creating new helper");
+            dbHelper = new DBHelper(context);
+        }
         return dbHelper;
     }
 
-    protected final SQLiteDatabase read() {
+    @Override
-        return db().getReadableDatabase();
+    public boolean onCreate() {
+        return true;
     }
 
-    protected final SQLiteDatabase write() {
+    protected final synchronized SQLiteDatabase db() {
-        return db().getWritableDatabase();
+        return getOrCreateDb(getContext().getApplicationContext()).getWritableDatabase();
     }
 
     @Override
-    public String getType(Uri uri) {
+    public String getType(@NonNull Uri uri) {
         String type;
         switch (getMatcher().match(uri)) {
             case CODE_LIST:

F-Droid/src/org/fdroid/fdroid/data/InstalledAppProvider.java

@@ -180,7 +180,7 @@ public class InstalledAppProvider extends FDroidProvider {
                 throw new UnsupportedOperationException(message);
         }
 
-        Cursor cursor = read().query(getTableName(), projection, selection.getSelection(), selection.getArgs(), null, null, sortOrder);
+        Cursor cursor = db().query(getTableName(), projection, selection.getSelection(), selection.getArgs(), null, null, sortOrder);
         cursor.setNotificationUri(getContext().getContentResolver(), uri);
         return cursor;
     }
@@ -195,7 +195,7 @@ public class InstalledAppProvider extends FDroidProvider {
         QuerySelection query = new QuerySelection(where, whereArgs);
         query = query.add(queryApp(uri.getLastPathSegment()));
 
-        int count = write().delete(getTableName(), query.getSelection(), query.getArgs());
+        int count = db().delete(getTableName(), query.getSelection(), query.getArgs());
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }
@@ -210,7 +210,7 @@ public class InstalledAppProvider extends FDroidProvider {
         }
 
         verifyVersionNameNotNull(values);
-        write().replaceOrThrow(getTableName(), null, values);
+        db().replaceOrThrow(getTableName(), null, values);
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }

F-Droid/src/org/fdroid/fdroid/data/RepoProvider.java

@@ -305,8 +305,7 @@ public class RepoProvider extends FDroidProvider {
                 throw new UnsupportedOperationException("Invalid URI for repo content provider: " + uri);
         }
 
-        Cursor cursor = read().query(getTableName(), projection, selection,
+        Cursor cursor = db().query(getTableName(), projection, selection, selectionArgs, null, null, sortOrder);
-                selectionArgs, null, null, sortOrder);
         cursor.setNotificationUri(getContext().getContentResolver(), uri);
         return cursor;
     }
@@ -342,7 +341,7 @@ public class RepoProvider extends FDroidProvider {
             values.put(DataColumns.NAME, Repo.addressToName(address));
         }
 
-        long id = write().insertOrThrow(getTableName(), null, values);
+        long id = db().insertOrThrow(getTableName(), null, values);
         Utils.debugLog(TAG, "Inserted repo. Notifying provider change: '" + uri + "'.");
         getContext().getContentResolver().notifyChange(uri, null);
         return getContentUri(id);
@@ -366,7 +365,7 @@ public class RepoProvider extends FDroidProvider {
                 throw new UnsupportedOperationException("Invalid URI for repo content provider: " + uri);
         }
 
-        int rowsAffected = write().delete(getTableName(), where, whereArgs);
+        int rowsAffected = db().delete(getTableName(), where, whereArgs);
         Utils.debugLog(TAG, "Deleted repos. Notifying provider change: '" + uri + "'.");
         getContext().getContentResolver().notifyChange(uri, null);
         return rowsAffected;
@@ -374,7 +373,7 @@ public class RepoProvider extends FDroidProvider {
 
     @Override
     public int update(Uri uri, ContentValues values, String where, String[] whereArgs) {
-        int numRows = write().update(getTableName(), values, where, whereArgs);
+        int numRows = db().update(getTableName(), values, where, whereArgs);
         Utils.debugLog(TAG, "Updated repo. Notifying provider change: '" + uri + "'.");
         getContext().getContentResolver().notifyChange(uri, null);
         return numRows;

F-Droid/src/org/fdroid/fdroid/data/TempApkProvider.java

@@ -3,6 +3,7 @@ package org.fdroid.fdroid.data;
 import android.content.ContentValues;
 import android.content.Context;
 import android.content.UriMatcher;
+import android.database.sqlite.SQLiteDatabase;
 import android.net.Uri;
 import android.util.Log;
 
@@ -112,7 +113,7 @@ public class TempApkProvider extends ApkProvider {
                 throw new UnsupportedOperationException("Invalid URI for apk content provider: " + uri);
         }
 
-        int rowsAffected = write().delete(getTableName(), query.getSelection(), query.getArgs());
+        int rowsAffected = db().delete(getTableName(), query.getSelection(), query.getArgs());
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }
@@ -121,11 +122,12 @@ public class TempApkProvider extends ApkProvider {
     }
 
     private void initTable() {
-        write().execSQL("DROP TABLE IF EXISTS " + getTableName());
+        final SQLiteDatabase db = db();
-        write().execSQL("CREATE TABLE " + getTableName() + " AS SELECT * FROM " + DBHelper.TABLE_APK);
+        db.execSQL("DROP TABLE IF EXISTS " + getTableName());
-        write().execSQL("CREATE INDEX IF NOT EXISTS apk_vercode on " + getTableName() + " (vercode);");
+        db.execSQL("CREATE TABLE " + getTableName() + " AS SELECT * FROM " + DBHelper.TABLE_APK);
-        write().execSQL("CREATE INDEX IF NOT EXISTS apk_id on " + getTableName() + " (id);");
+        db.execSQL("CREATE INDEX IF NOT EXISTS apk_vercode on " + getTableName() + " (vercode);");
-        write().execSQL("CREATE INDEX IF NOT EXISTS apk_compatible ON " + getTableName() + " (compatible);");
+        db.execSQL("CREATE INDEX IF NOT EXISTS apk_id on " + getTableName() + " (id);");
+        db.execSQL("CREATE INDEX IF NOT EXISTS apk_compatible ON " + getTableName() + " (compatible);");
     }
 
 }

F-Droid/src/org/fdroid/fdroid/data/TempAppProvider.java

@@ -104,7 +104,7 @@ public class TempAppProvider extends AppProvider {
                 throw new UnsupportedOperationException("Update not supported for " + uri + ".");
         }
 
-        int count = write().update(getTableName(), values, query.getSelection(), query.getArgs());
+        int count = db().update(getTableName(), values, query.getSelection(), query.getArgs());
         if (!isApplyingBatch()) {
             getContext().getContentResolver().notifyChange(uri, null);
         }
@@ -112,15 +112,16 @@ public class TempAppProvider extends AppProvider {
     }
 
     private void initTable() {
-        write().execSQL("DROP TABLE IF EXISTS " + getTableName());
+        final SQLiteDatabase db = db();
-        write().execSQL("CREATE TABLE " + getTableName() + " AS SELECT * FROM " + DBHelper.TABLE_APP);
+        db.execSQL("DROP TABLE IF EXISTS " + getTableName());
-        write().execSQL("CREATE INDEX IF NOT EXISTS app_id ON " + getTableName() + " (id);");
+        db.execSQL("CREATE TABLE " + getTableName() + " AS SELECT * FROM " + DBHelper.TABLE_APP);
-        write().execSQL("CREATE INDEX IF NOT EXISTS app_upstreamVercode ON " + getTableName() + " (upstreamVercode);");
+        db.execSQL("CREATE INDEX IF NOT EXISTS app_id ON " + getTableName() + " (id);");
-        write().execSQL("CREATE INDEX IF NOT EXISTS app_compatible ON " + getTableName() + " (compatible);");
+        db.execSQL("CREATE INDEX IF NOT EXISTS app_upstreamVercode ON " + getTableName() + " (upstreamVercode);");
+        db.execSQL("CREATE INDEX IF NOT EXISTS app_compatible ON " + getTableName() + " (compatible);");
     }
 
     private void commitTable() {
-        final SQLiteDatabase db = write();
+        final SQLiteDatabase db = db();
         try {
             db.beginTransaction();
 

F-Droid/test/src/android/test/ProviderTestCase2MockContext.java

@@ -20,6 +20,7 @@ import android.annotation.TargetApi;
 import android.content.ContentProvider;
 import android.content.ContentResolver;
 import android.content.Context;
+import android.content.ContextWrapper;
 import android.content.res.Resources;
 import android.database.DatabaseUtils;
 import android.os.Build;
@@ -136,12 +137,20 @@ public abstract class ProviderTestCase2MockContext<T extends ContentProvider> ex
 
         mResolver = new MockContentResolver();
         final String filenamePrefix = "test.";
-        RenamingDelegatingContext targetContextWrapper = new
+        final RenamingDelegatingContext targetContextWrapper = new
                 RenamingDelegatingContext(
                 createMockContext(new MockContext2()), // The context that most methods are delegated to
                 getContext(), // The context that file methods are delegated to
                 filenamePrefix);
-        mProviderContext = new IsolatedContext(mResolver, targetContextWrapper);
+
+        mProviderContext = new IsolatedContext(mResolver, new ContextWrapper(targetContextWrapper) {
+            // The FDroidProvider class needs access to an application context in order to initialize
+            // the singleton DBHelper instance.
+            @Override
+            public Context getApplicationContext() {
+                return targetContextWrapper;
+            }
+        });
 
         mProvider = mProviderClass.newInstance();
         mProvider.attachInfo(mProviderContext, null);

F-Droid/test/src/mock/MockContextSwappableComponents.java

@@ -1,5 +1,6 @@
 package mock;
 
+import android.content.Context;
 import android.content.pm.PackageManager;
 import android.content.res.Resources;
 import android.test.mock.MockContentResolver;
@@ -8,7 +9,6 @@ import android.test.mock.MockContext;
 public class MockContextSwappableComponents extends MockContext {
 
     private PackageManager packageManager;
-
     private Resources resources;
     private MockContentResolver contentResolver;
 

F-Droid/test/src/org/fdroid/fdroid/FDroidProviderTest.java

@@ -45,6 +45,8 @@ public abstract class FDroidProviderTest<T extends FDroidProvider> extends Provi
     public void setUp() throws Exception {
         super.setUp();
 
+        FDroidProvider.clearDbHelperSingleton();
+
         // Instantiate all providers other than the one which was already created by the base class.
         // This is because F-Droid providers tend to perform joins onto tables managed by other
         // providers, and so we need to be able to insert into those other providers for these

F-Droid/test/src/org/fdroid/fdroid/MultiRepoUpdaterTest.java

@@ -18,6 +18,7 @@ import org.fdroid.fdroid.RepoUpdater.UpdateException;
 import org.fdroid.fdroid.data.Apk;
 import org.fdroid.fdroid.data.ApkProvider;
 import org.fdroid.fdroid.data.AppProvider;
+import org.fdroid.fdroid.data.FDroidProvider;
 import org.fdroid.fdroid.data.Repo;
 import org.fdroid.fdroid.data.RepoProvider;
 import org.fdroid.fdroid.data.TempApkProvider;
@@ -122,12 +123,20 @@ public class MultiRepoUpdaterTest extends InstrumentationTestCase {
         public File getDatabasePath(String name) {
             return new File(getInstrumentation().getContext().getFilesDir(), "fdroid_test.db");
         }
+
+        @Override
+        public Context getApplicationContext() {
+            // Used by the DBHelper singleton instance.
+            return this;
+        }
     }
 
     @Override
     public void setUp() throws Exception {
         super.setUp();
 
+        FDroidProvider.clearDbHelperSingleton();
+
         context = new TestContext();
 
         testFilesDir = TestUtils.getWriteableDir(getInstrumentation());