Merge branch 'CleanCacheWorker-bug-fixes' into 'master'

CleanCacheWorker bug fixes

See merge request fdroid/fdroidclient!986

app/src/androidTest/java/org/fdroid/fdroid/work/CleanCacheWorkerTest.java

@@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue;
 
 /**
  * This test cannot run on Robolectric unfortunately since it does not support
+ * getting the timestamps from the files completely.
  * <p>
  * This is marked with {@link LargeTest} because it always fails on the emulator
  * tests on GitLab CI.  That excludes it from the test run there.
@@ -86,4 +87,24 @@ public class CleanCacheWorkerTest {
         CleanCacheWorker.clearOldFiles(nonexistent, 1);
         CleanCacheWorker.clearOldFiles(null, 1);
     }
+
+    /*
+    // TODO enable this once getImageCacheDir() can be mocked or provide a writable dir in the test
+    @Test
+    public void testDeleteOldIcons() throws IOException {
+        Context context = InstrumentationRegistry.getInstrumentation().getContext();
+        File imageCacheDir = Utils.getImageCacheDir(context);
+        imageCacheDir.mkdirs();
+        assertTrue(imageCacheDir.isDirectory());
+        File oldIcon = new File(imageCacheDir, "old.png");
+        assertTrue(oldIcon.createNewFile());
+        Assume.assumeTrue("test environment must be able to set LastModified time",
+                oldIcon.setLastModified(System.currentTimeMillis() - (DateUtils.DAY_IN_MILLIS * 370)));
+        File currentIcon = new File(imageCacheDir, "current.png");
+        assertTrue(currentIcon.createNewFile());
+        CleanCacheWorker.deleteOldIcons(context);
+        assertTrue(currentIcon.exists());
+        assertFalse(oldIcon.exists());
+    }
+     */
 }

app/src/basic/java/org/fdroid/fdroid/nearby/LocalRepoManager.java

@@ -0,0 +1,5 @@
+package org.fdroid.fdroid.nearby;
+
+public class LocalRepoManager {
+    public static final String[] WEB_ROOT_ASSET_FILES = {};
+}

app/src/full/java/org/fdroid/fdroid/nearby/LocalRepoManager.java

@@ -10,10 +10,10 @@ import android.graphics.Bitmap.Config;
 import android.graphics.Canvas;
 import android.graphics.drawable.BitmapDrawable;
 import android.graphics.drawable.Drawable;
-import androidx.annotation.NonNull;
-import androidx.annotation.Nullable;
 import android.text.TextUtils;
 import android.util.Log;
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import org.fdroid.fdroid.FDroidApp;
 import org.fdroid.fdroid.Hasher;
 import org.fdroid.fdroid.IndexUpdater;
@@ -56,7 +56,6 @@ import java.util.jar.JarOutputStream;
  * This class deals specifically with the webroot side of things, ensuring we have a valid index.jar
  * and the relevant .apk and icon files available.
  */
-@SuppressWarnings("LineLength")
 public final class LocalRepoManager {
     private static final String TAG = "LocalRepoManager";
 
@@ -65,7 +64,7 @@ public final class LocalRepoManager {
     private final AssetManager assetManager;
     private final String fdroidPackageName;
 
-    private static final String[] WEB_ROOT_ASSET_FILES = {
+    public static final String[] WEB_ROOT_ASSET_FILES = {
             "swap-icon.png",
             "swap-tick-done.png",
             "swap-tick-not-done.png",
@@ -348,7 +347,8 @@ public final class LocalRepoManager {
             serializer = XmlPullParserFactory.newInstance().newSerializer();
         }
 
-        public void build(Context context, Map<String, App> apps, OutputStream output) throws IOException, LocalRepoKeyStore.InitException {
+        public void build(Context context, Map<String, App> apps, OutputStream output)
+                throws IOException, LocalRepoKeyStore.InitException {
             serializer.setOutput(output, "UTF-8");
             serializer.startDocument(null, null);
             serializer.startTag("", "fdroid");
@@ -356,12 +356,14 @@ public final class LocalRepoManager {
             // <repo> block
             serializer.startTag("", "repo");
             serializer.attribute("", "icon", "blah.png");
-            serializer.attribute("", "name", Preferences.get().getLocalRepoName() + " on " + FDroidApp.ipAddressString);
+            serializer.attribute("", "name", Preferences.get().getLocalRepoName()
+                    + " on " + FDroidApp.ipAddressString);
             serializer.attribute("", "pubkey", Hasher.hex(LocalRepoKeyStore.get(context).getCertificate()));
             long timestamp = System.currentTimeMillis() / 1000L;
             serializer.attribute("", "timestamp", String.valueOf(timestamp));
             serializer.attribute("", "version", "10");
-            tag("description", "A local FDroid repo generated from apps installed on " + Preferences.get().getLocalRepoName());
+            tag("description", "A local FDroid repo generated from apps installed on "
+                    + Preferences.get().getLocalRepoName());
             serializer.endTag("", "repo");
 
             // <application> blocks

app/src/main/java/org/fdroid/fdroid/installer/ApkFileProvider.java

@@ -42,13 +42,16 @@ import java.io.IOException;
  * <i>"/Android/data/[app_package_name]/cache/apks"</i> (if card is mounted and app has
  * appropriate permission) or on device's file system depending incoming parameters</li>
  * <li>Before installation, the APK is copied into the private data directory of the F-Droid,
- * <i>"/data/data/[app_package_name]/files/install-$random.apk"</i></li>
+ * <i>"/data/data/[app_package_name]/files/install-$random.apk"</i> so that the install
+ * process broken if the user clears the cache while it is running.</li>
  * <li>The hash of the file is checked against the expected hash from the repository</li>
  * <li>For {@link Build.VERSION_CODES#M < android-23}, a {@code file://} {@link Uri}
  * pointing to the {@link File} is returned, for {@link Build.VERSION_CODES#M >= android-23},
  * a {@code content://} {@code Uri} is returned using support lib's
  * {@link FileProvider}</li>
  * </ol>
+ *
+ * @see org.fdroid.fdroid.work.CleanCacheWorker
  */
 public class ApkFileProvider extends FileProvider {
 

app/src/main/java/org/fdroid/fdroid/receiver/DeviceStorageReceiver.java

@@ -18,9 +18,8 @@ public class DeviceStorageReceiver extends BroadcastReceiver {
         if (Intent.ACTION_DEVICE_STORAGE_LOW.equals(action)) {
             int percentageFree = Utils.getPercent(Utils.getImageCacheDirAvailableMemory(context),
                     Utils.getImageCacheDirTotalMemory(context));
-            if (percentageFree > 2) {
+            CleanCacheWorker.force(context);
-                CleanCacheWorker.schedule(context);
+            if (percentageFree <= 2) {
-            } else {
                 DeleteCacheService.deleteAll(context);
             }
         }

app/src/main/java/org/fdroid/fdroid/views/main/SettingsView.java

@@ -2,17 +2,14 @@ package org.fdroid.fdroid.views.main;
 
 import android.annotation.TargetApi;
 import android.content.Context;
-import androidx.appcompat.app.AppCompatActivity;
 import android.util.AttributeSet;
 import android.widget.FrameLayout;
-
+import androidx.appcompat.app.AppCompatActivity;
 import androidx.fragment.app.Fragment;
 import androidx.fragment.app.FragmentTransaction;
-
 import org.fdroid.fdroid.R;
 import org.fdroid.fdroid.views.PreferencesFragment;
 
-@SuppressWarnings("LineLength")
 /**
  * When attached to the window, the {@link PreferencesFragment} will be added. When detached from
  * the window, the fragment will be removed.
@@ -52,9 +49,6 @@ public class SettingsView extends FrameLayout {
         super.onAttachedToWindow();
 
         AppCompatActivity activity = (AppCompatActivity) getContext();
-        if (activity == null) {
-            throw new IllegalArgumentException("Cannot add a SettingsView to activities which are not an AppCompatActivity");
-        }
 
         if (currentTransaction == null) {
             currentTransaction = activity.getSupportFragmentManager().beginTransaction();
@@ -71,9 +65,6 @@ public class SettingsView extends FrameLayout {
         super.onDetachedFromWindow();
 
         AppCompatActivity activity = (AppCompatActivity) getContext();
-        if (activity == null) {
-            throw new IllegalArgumentException("Cannot add a SettingsView to activities which are not an AppCompatActivity");
-        }
 
         Fragment existingFragment = activity.getSupportFragmentManager().findFragmentByTag("preferences-fragment");
         if (existingFragment == null) {

app/src/main/java/org/fdroid/fdroid/work/CleanCacheWorker.java

@@ -10,6 +10,8 @@ import androidx.annotation.NonNull;
 import androidx.annotation.RequiresApi;
 import androidx.work.Constraints;
 import androidx.work.ExistingPeriodicWorkPolicy;
+import androidx.work.ExistingWorkPolicy;
+import androidx.work.OneTimeWorkRequest;
 import androidx.work.PeriodicWorkRequest;
 import androidx.work.WorkManager;
 import androidx.work.Worker;
@@ -18,10 +20,25 @@ import org.apache.commons.io.FileUtils;
 import org.fdroid.fdroid.Preferences;
 import org.fdroid.fdroid.Utils;
 import org.fdroid.fdroid.installer.ApkCache;
+import org.fdroid.fdroid.nearby.LocalRepoManager;
 
 import java.io.File;
+import java.util.Arrays;
+import java.util.List;
 import java.util.concurrent.TimeUnit;
 
+/**
+ * Deletes the built-up cruft left over from various processes.
+ * <p>
+ * The installation process downloads APKs to the cache, then when an APK is being
+ * installed, it is copied into files for running the install. Installs can happen
+ * fully in the background, so the user might clear the cache at any time, so the
+ * APK cannot be installed from the cache. Also, F-Droid is not guaranteed to get
+ * an event after the APK is installed, so that can't be used to delete the APK
+ * from files when it is no longer needed. That's where CleanCacheWorker comes in,
+ * it runs regularly to ensure things are cleaned up. If something blocks it from
+ * running, then APKs can remain in {@link org.fdroid.fdroid.installer.ApkFileProvider}
+ */
 public class CleanCacheWorker extends Worker {
     public static final String TAG = "CleanCacheWorker";
 
@@ -30,7 +47,7 @@ public class CleanCacheWorker extends Worker {
     }
 
     /**
-     * Schedule or cancel a work request to update the app index, according to the
+     * Schedule or cancel a work request to clean up caches, according to the
      * current preferences. Should be called a) at boot, b) if the preference
      * is changed, or c) on startup, in case we get upgraded.
      */
@@ -56,15 +73,28 @@ public class CleanCacheWorker extends Worker {
         Utils.debugLog(TAG, "Scheduled periodic work for cleaning the cache.");
     }
 
+    /**
+     * Force a cache cleanup.  Since {@link #deleteOldInstallerFiles(Context)}
+     * only deletes files older than an hour, any ongoing APK install processes
+     * should not have their APKs are deleted out from under them.
+     */
+    public static void force(@NonNull final Context context) {
+        OneTimeWorkRequest cleanCache = new OneTimeWorkRequest.Builder(CleanCacheWorker.class).build();
+        WorkManager workManager = WorkManager.getInstance(context);
+        workManager.enqueueUniqueWork(TAG + ".force", ExistingWorkPolicy.KEEP, cleanCache);
+        Utils.debugLog(TAG, "Enqueued forced run for cleaning the cache.");
+    }
+
     @NonNull
     @Override
     public Result doWork() {
         Process.setThreadPriority(Process.THREAD_PRIORITY_LOWEST);
         try {
-            deleteExpiredApksFromCache();
+            final Context context = getApplicationContext();
-            deleteStrayIndexFiles();
+            deleteExpiredApksFromCache(context);
-            deleteOldInstallerFiles();
+            deleteStrayIndexFiles(context);
-            deleteOldIcons();
+            deleteOldInstallerFiles(context);
+            deleteOldIcons(context);
             return Result.success();
         } catch (Exception e) {
             return Result.failure();
@@ -76,17 +106,20 @@ public class CleanCacheWorker extends Worker {
      * specified by the user in the "Keep Cache Time" preference.  This removes
      * any APK in the cache that is older than that preference specifies.
      */
-    private void deleteExpiredApksFromCache() {
+    static void deleteExpiredApksFromCache(@NonNull Context context) {
-        File cacheDir = ApkCache.getApkCacheDir(getApplicationContext());
+        File cacheDir = ApkCache.getApkCacheDir(context);
         clearOldFiles(cacheDir, Preferences.get().getKeepCacheTime());
     }
 
     /**
      * {@link org.fdroid.fdroid.installer.Installer} instances copy the APK into
-     * a safe place before installing.  It doesn't clean up them reliably yet.
+     * a safe place before installing.  This only deletes files older than an
+     * hour to avoid deleting APKs while they are still being installed.  This
+     * also avoids deleting the nearby swap repo files since that might be
+     * actively in use.
      */
-    private void deleteOldInstallerFiles() {
+    static void deleteOldInstallerFiles(@NonNull Context context) {
-        File filesDir = getApplicationContext().getFilesDir();
+        File filesDir = context.getFilesDir();
         if (filesDir == null) {
             Utils.debugLog(TAG, "The files directory doesn't exist.");
             return;
@@ -98,8 +131,9 @@ public class CleanCacheWorker extends Worker {
             return;
         }
 
+        final List<String> webRootAssetFiles = Arrays.asList(LocalRepoManager.WEB_ROOT_ASSET_FILES);
         for (File f : files) {
-            if (f.getName().endsWith(".apk")) {
+            if (f.isFile() && !f.getName().endsWith(".html") && !webRootAssetFiles.contains(f.getName())) {
                 clearOldFiles(f, TimeUnit.HOURS.toMillis(1));
             }
         }
@@ -117,8 +151,8 @@ public class CleanCacheWorker extends Worker {
      * This also deletes temp files that are created by
      * {@link org.fdroid.fdroid.net.DownloaderFactory#create(Context, String)}, e.g. "dl-*"
      */
-    private void deleteStrayIndexFiles() {
+    static void deleteStrayIndexFiles(@NonNull Context context) {
-        File cacheDir = getApplicationContext().getCacheDir();
+        File cacheDir = context.getCacheDir();
         if (cacheDir == null) {
             Utils.debugLog(TAG, "The cache directory doesn't exist.");
             return;
@@ -143,8 +177,8 @@ public class CleanCacheWorker extends Worker {
     /**
      * Delete cached icons that have not been accessed in over a year.
      */
-    private void deleteOldIcons() {
+    static void deleteOldIcons(@NonNull Context context) {
-        clearOldFiles(Utils.getImageCacheDir(getApplicationContext()), TimeUnit.DAYS.toMillis(365));
+        clearOldFiles(Utils.getImageCacheDir(context), TimeUnit.DAYS.toMillis(365));
     }
 
     /**

app/src/test/java/org/fdroid/fdroid/TestUtils.java

@@ -8,6 +8,7 @@ import android.content.ContextWrapper;
 import android.content.pm.ProviderInfo;
 import android.net.Uri;
 import androidx.test.core.app.ApplicationProvider;
+import org.apache.commons.io.IOUtils;
 import org.fdroid.fdroid.data.Apk;
 import org.fdroid.fdroid.data.ApkProvider;
 import org.fdroid.fdroid.data.App;
@@ -190,4 +191,17 @@ public class TestUtils {
         modifiersField.setInt(field, field.getModifiers() & ~Modifier.FINAL);
         field.set(null, newValue);
     }
+
+    public static void ls(File dir) {
+        Process p = null;
+        try {
+            p = Runtime.getRuntime().exec("ls -l " + dir.getAbsolutePath());
+            p.waitFor();
+            for (String line : IOUtils.readLines(p.getInputStream())) {
+                System.out.println(line);
+            }
+        } catch (IOException | InterruptedException e) {
+            e.printStackTrace();
+        }
+    }
 }

app/src/test/java/org/fdroid/fdroid/work/CleanCacheWorkerTest.java

@@ -0,0 +1,93 @@
+package org.fdroid.fdroid.work;
+
+import android.content.Context;
+import androidx.test.core.app.ApplicationProvider;
+import org.fdroid.fdroid.BuildConfig;
+import org.fdroid.fdroid.Preferences;
+import org.fdroid.fdroid.nearby.LocalRepoManager;
+import org.fdroid.fdroid.shadows.ShadowLog;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.RobolectricTestRunner;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Test non-time-based cache deletion methods.  Robolectric is lacking full
+ * support for getting time from files, so methods that rely on time must be
+ * tested in {@code CleanCacheWorkerTest} in {@code androidTest}.
+ */
+@RunWith(RobolectricTestRunner.class)
+public class CleanCacheWorkerTest {
+    public static final String TAG = "CleanCacheWorkerTest";
+
+    private static final Context CONTEXT = ApplicationProvider.getApplicationContext();
+    private static final File FILES_DIR = CONTEXT.getFilesDir();
+
+    @Before
+    public void setUp() {
+        ShadowLog.stream = System.out;
+        Preferences.setupForTests(CONTEXT);
+    }
+
+    @Test
+    public void testDeleteOldInstallerFiles() throws IOException {
+        Assume.assumeTrue(BuildConfig.FLAVOR.startsWith("full"));
+        ArrayList<File> webRootAssetFiles = new ArrayList<>();
+        File indexHtml = new File(FILES_DIR, "index.html");
+        assertTrue(indexHtml.createNewFile());
+        webRootAssetFiles.add(indexHtml);
+        for (String name : LocalRepoManager.WEB_ROOT_ASSET_FILES) {
+            File f = new File(FILES_DIR, name);
+            assertTrue(f.createNewFile());
+            webRootAssetFiles.add(f);
+        }
+        File apk = new File(FILES_DIR, "fake.apk");
+        assertTrue(apk.createNewFile());
+        File giantblob = new File(FILES_DIR, "giantblob");
+        assertTrue(giantblob.createNewFile());
+        File obf = new File(FILES_DIR, "fake.obf");
+        assertTrue(obf.createNewFile());
+        File zip = new File(FILES_DIR, "fake.zip");
+        assertTrue(zip.createNewFile());
+        CleanCacheWorker.deleteOldInstallerFiles(CONTEXT);
+        assertFalse(apk.exists());
+        assertFalse(giantblob.exists());
+        assertFalse(obf.exists());
+        assertFalse(zip.exists());
+        for (File f : webRootAssetFiles) {
+            assertTrue(f.exists());
+        }
+    }
+
+    /**
+     * Pure smoke check, Robolectric does not support file times fully.
+     */
+    @Test
+    public void testDeleteExpiredApksFromCache() {
+        CleanCacheWorker.deleteExpiredApksFromCache(CONTEXT);
+    }
+
+    /**
+     * Pure smoke check, Robolectric does not support file times fully.
+     */
+    @Test
+    public void testDeleteStrayIndexFiles() {
+        CleanCacheWorker.deleteStrayIndexFiles(CONTEXT);
+    }
+
+    /**
+     * Pure smoke check, Robolectric does not support file times fully.
+     */
+    @Test
+    public void testDeleteOldIcons() {
+        CleanCacheWorker.deleteOldIcons(CONTEXT);
+    }
+}