consolidate: prevent concurrent calls of step 2

4 files changed, 115 insertions, 91 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/KeychainApplication.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/KeychainApplication.java
index 964a15d58..ead29e229 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/KeychainApplication.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/KeychainApplication.java
@@ -86,21 +86,26 @@ public class KeychainApplication extends Application {
         setupAccountAsNeeded(this);
 
         // Update keyserver list as needed
-        Preferences prefs = Preferences.getPreferences(this);
-
-        prefs.updatePreferences();
+        Preferences.getPreferences(this).updatePreferences();
 
         TlsHelper.addStaticCA("pool.sks-keyservers.net", getAssets(), "sks-keyservers.netCA.cer");
 
         TemporaryStorageProvider.cleanUp(this);
 
+        checkConsolidateRecovery();
+
+    }
+
+    public void checkConsolidateRecovery() {
+
         // restart consolidate process if it has been interruped before
-        if (prefs.getCachedConsolidate()) {
+        if (Preferences.getPreferences(this).getCachedConsolidate()) {
             // do something which calls ProviderHelper.consolidateDatabaseStep2 with a progressable
             Intent consolidateIntent = new Intent(this, ConsolidateDialogActivity.class);
             consolidateIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
             startActivity(consolidateIntent);
         }
+
     }
 
     public static void setupAccountAsNeeded(Context context) {
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
index 125f33ded..0d6e83749 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
@@ -961,111 +961,128 @@ public class ProviderHelper {
         return consolidateDatabaseStep2(progress, true);
     }
 
+    private static boolean mConsolidateCritical = false;
+
     private ConsolidateResult consolidateDatabaseStep2(Progressable progress, boolean recovery) {
 
-        Preferences prefs = Preferences.getPreferences(mContext);
+        synchronized (ProviderHelper.class) {
+            if (mConsolidateCritical) {
+                log(LogLevel.ERROR, LogType.MSG_CON_ERROR_CONCURRENT);
+                return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
+            }
+            mConsolidateCritical = true;
+        }
 
-        // Set flag that we have a cached consolidation here
-        int numSecrets = prefs.getCachedConsolidateNumSecrets();
-        int numPublics = prefs.getCachedConsolidateNumPublics();
+        try {
+            Preferences prefs = Preferences.getPreferences(mContext);
 
-        if (recovery) {
-            if (numSecrets >= 0 && numPublics >= 0) {
-                log(LogLevel.START, LogType.MSG_CON_RECOVER, numSecrets, numPublics);
-            } else {
-                log(LogLevel.START, LogType.MSG_CON_RECOVER_UNKNOWN);
+            // Set flag that we have a cached consolidation here
+            int numSecrets = prefs.getCachedConsolidateNumSecrets();
+            int numPublics = prefs.getCachedConsolidateNumPublics();
+
+            if (recovery) {
+                if (numSecrets >= 0 && numPublics >= 0) {
+                    log(LogLevel.START, LogType.MSG_CON_RECOVER, numSecrets, numPublics);
+                } else {
+                    log(LogLevel.START, LogType.MSG_CON_RECOVER_UNKNOWN);
+                }
+                mIndent += 1;
             }
-            mIndent += 1;
-        }
 
-        if ( ! prefs.getCachedConsolidate()) {
-            log(LogLevel.ERROR, LogType.MSG_CON_ERROR_BAD_STATE);
-            return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
-        }
+            if (!prefs.getCachedConsolidate()) {
+                log(LogLevel.ERROR, LogType.MSG_CON_ERROR_BAD_STATE);
+                return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
+            }
 
-        // 2. wipe database (IT'S DANGEROUS)
-        log(LogLevel.DEBUG, LogType.MSG_CON_DB_CLEAR);
-        mContentResolver.delete(KeyRings.buildUnifiedKeyRingsUri(), null, null);
+            // 2. wipe database (IT'S DANGEROUS)
+            log(LogLevel.DEBUG, LogType.MSG_CON_DB_CLEAR);
+            mContentResolver.delete(KeyRings.buildUnifiedKeyRingsUri(), null, null);
 
-        // debug: break if this isn't recovery
-        if (!recovery) {
-            return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
-        }
+            // debug: break if this isn't recovery
+            if (!recovery) {
+                return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
+            }
 
-        FileImportCache<ParcelableKeyRing> cacheSecret =
-                new FileImportCache<ParcelableKeyRing>(mContext, "consolidate_secret.pcl");
-        FileImportCache<ParcelableKeyRing> cachePublic =
-                new FileImportCache<ParcelableKeyRing>(mContext, "consolidate_public.pcl");
+            FileImportCache<ParcelableKeyRing> cacheSecret =
+                    new FileImportCache<ParcelableKeyRing>(mContext, "consolidate_secret.pcl");
+            FileImportCache<ParcelableKeyRing> cachePublic =
+                    new FileImportCache<ParcelableKeyRing>(mContext, "consolidate_public.pcl");
 
-        // 3. Re-Import secret keyrings from cache
-        if (numSecrets > 0) try {
-            log(LogLevel.DEBUG, LogType.MSG_CON_REIMPORT_SECRET, numSecrets);
-            mIndent += 1;
+            // 3. Re-Import secret keyrings from cache
+            if (numSecrets > 0) try {
+                log(LogLevel.DEBUG, LogType.MSG_CON_REIMPORT_SECRET, numSecrets);
+                mIndent += 1;
 
-            new PgpImportExport(mContext, this,
-                new ProgressFixedScaler(progress, 10, 25, 100, R.string.progress_con_reimport))
-                    .importKeyRings(cacheSecret.readCache(false), numSecrets);
-        } catch (IOException e) {
-            Log.e(Constants.TAG, "error importing secret", e);
-            log(LogLevel.ERROR, LogType.MSG_CON_ERROR_SECRET);
-            return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
-        } finally {
-            mIndent -= 1;
-        } else {
-            log(LogLevel.DEBUG, LogType.MSG_CON_REIMPORT_SECRET_SKIP);
-        }
+                new PgpImportExport(mContext, this,
+                        new ProgressFixedScaler(progress, 10, 25, 100, R.string.progress_con_reimport))
+                        .importKeyRings(cacheSecret.readCache(false), numSecrets);
+            } catch (IOException e) {
+                Log.e(Constants.TAG, "error importing secret", e);
+                log(LogLevel.ERROR, LogType.MSG_CON_ERROR_SECRET);
+                return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
+            } finally {
+                mIndent -= 1;
+            }
+            else {
+                log(LogLevel.DEBUG, LogType.MSG_CON_REIMPORT_SECRET_SKIP);
+            }
 
-        // 4. Re-Import public keyrings from cache
-        if (numPublics > 0) try {
-            log(LogLevel.DEBUG, LogType.MSG_CON_REIMPORT_PUBLIC, numPublics);
-            mIndent += 1;
+            // 4. Re-Import public keyrings from cache
+            if (numPublics > 0) try {
+                log(LogLevel.DEBUG, LogType.MSG_CON_REIMPORT_PUBLIC, numPublics);
+                mIndent += 1;
 
-            new PgpImportExport(mContext, this,
-                new ProgressFixedScaler(progress, 25, 99, 100, R.string.progress_con_reimport))
-                    .importKeyRings(cachePublic.readCache(false), numPublics);
-        } catch (IOException e) {
-            Log.e(Constants.TAG, "error importing public", e);
-            log(LogLevel.ERROR, LogType.MSG_CON_ERROR_PUBLIC);
-            return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
-        } finally {
-            mIndent -= 1;
-        } else {
-            log(LogLevel.DEBUG, LogType.MSG_CON_REIMPORT_PUBLIC_SKIP);
-        }
+                new PgpImportExport(mContext, this,
+                        new ProgressFixedScaler(progress, 25, 99, 100, R.string.progress_con_reimport))
+                        .importKeyRings(cachePublic.readCache(false), numPublics);
+            } catch (IOException e) {
+                Log.e(Constants.TAG, "error importing public", e);
+                log(LogLevel.ERROR, LogType.MSG_CON_ERROR_PUBLIC);
+                return new ConsolidateResult(ConsolidateResult.RESULT_ERROR, mLog);
+            } finally {
+                mIndent -= 1;
+            }
+            else {
+                log(LogLevel.DEBUG, LogType.MSG_CON_REIMPORT_PUBLIC_SKIP);
+            }
 
-        log(LogLevel.INFO, LogType.MSG_CON_CRITICAL_OUT);
-        Preferences.getPreferences(mContext).setCachedConsolidate(false);
+            log(LogLevel.INFO, LogType.MSG_CON_CRITICAL_OUT);
+            Preferences.getPreferences(mContext).setCachedConsolidate(false);
 
-        // 5. Delete caches
-        try {
-            log(LogLevel.DEBUG, LogType.MSG_CON_DELETE_SECRET);
-            mIndent += 1;
-            cacheSecret.delete();
-        } catch (IOException e) {
-            // doesn't /really/ matter
-            Log.e(Constants.TAG, "IOException during delete of secret cache", e);
-            log(LogLevel.WARN, LogType.MSG_CON_WARN_DELETE_SECRET);
-        } finally {
-            mIndent -= 1;
-        }
+            // 5. Delete caches
+            try {
+                log(LogLevel.DEBUG, LogType.MSG_CON_DELETE_SECRET);
+                mIndent += 1;
+                cacheSecret.delete();
+            } catch (IOException e) {
+                // doesn't /really/ matter
+                Log.e(Constants.TAG, "IOException during delete of secret cache", e);
+                log(LogLevel.WARN, LogType.MSG_CON_WARN_DELETE_SECRET);
+            } finally {
+                mIndent -= 1;
+            }
 
-        try {
-            log(LogLevel.DEBUG, LogType.MSG_CON_DELETE_PUBLIC);
-            mIndent += 1;
-            cachePublic.delete();
-        } catch (IOException e) {
-            // doesn't /really/ matter
-            Log.e(Constants.TAG, "IOException during deletion of public cache", e);
-            log(LogLevel.WARN, LogType.MSG_CON_WARN_DELETE_PUBLIC);
-        } finally {
+            try {
+                log(LogLevel.DEBUG, LogType.MSG_CON_DELETE_PUBLIC);
+                mIndent += 1;
+                cachePublic.delete();
+            } catch (IOException e) {
+                // doesn't /really/ matter
+                Log.e(Constants.TAG, "IOException during deletion of public cache", e);
+                log(LogLevel.WARN, LogType.MSG_CON_WARN_DELETE_PUBLIC);
+            } finally {
+                mIndent -= 1;
+            }
+
+            progress.setProgress(100, 100);
+            log(LogLevel.OK, LogType.MSG_CON_SUCCESS);
             mIndent -= 1;
-        }
 
-        progress.setProgress(100, 100);
-        log(LogLevel.OK, LogType.MSG_CON_SUCCESS);
-        mIndent -= 1;
+            return new ConsolidateResult(ConsolidateResult.RESULT_OK, mLog);
 
-        return new ConsolidateResult(ConsolidateResult.RESULT_OK, mLog);
+        } finally {
+            mConsolidateCritical = false;
+        }
 
     }
 
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
index 91939739a..7fe712534 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
@@ -396,6 +396,7 @@ public class OperationResultParcel implements Parcelable {
         MSG_CON_DELETE_PUBLIC (R.string.msg_con_delete_public),
         MSG_CON_DELETE_SECRET (R.string.msg_con_delete_secret),
         MSG_CON_ERROR_BAD_STATE (R.string.msg_con_error_bad_state),
+        MSG_CON_ERROR_CONCURRENT(R.string.msg_con_error_concurrent),
         MSG_CON_ERROR_DB (R.string.msg_con_error_db),
         MSG_CON_ERROR_IO_PUBLIC (R.string.msg_con_error_io_public),
         MSG_CON_ERROR_IO_SECRET (R.string.msg_con_error_io_secret),
diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml
index 64c743ead..3175d679b 100644
--- a/OpenKeychain/src/main/res/values/strings.xml
+++ b/OpenKeychain/src/main/res/values/strings.xml
@@ -673,7 +673,8 @@
 
     <!-- Consolidate -->
     <string name="msg_con">Consolidating database</string>
-    <string name="msg_con_error_bad_state">Consolidation started while no database was cached! This is probably a programming error, please file a bug report.</string>
+    <string name="msg_con_error_bad_state">Consolidation was started while no database was cached! This is probably a programming error, please file a bug report.</string>
+    <string name="msg_con_error_concurrent">Consolidation aborted, already running on other thread!</string>
     <string name="msg_con_save_secret">Saving secret keyrings</string>
     <string name="msg_con_save_public">Saving public keyrings</string>
     <string name="msg_con_db_clear">Clearing database</string>