From c35d327fa4c503425c629d49778c001b08ecb833 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 19 May 2016 20:30:17 +0200 Subject: [PATCH 1/3] include all needed data in install Intents Including the App and Apk instances in the Intent that starts InstallManagerService ensures that the needed data is present in the Service no matter what happens outside of the Service. For example, if the index is updated or cleared while an install is in progress, the install process still needs to know the name and packageName of the app to update the Notification. A cleaner but more labor-intensive way to implement this would be to make App and Apk properly implement the full Parcelable interface. That would require tests to check that the Parcelable methods have all the same fields as toContentValues() and the database. closes #660 https://gitlab.com/fdroid/fdroidclient/issues/660 --- .../main/java/org/fdroid/fdroid/data/Apk.java | 8 +- .../main/java/org/fdroid/fdroid/data/App.java | 5 ++ .../fdroid/data/ContentValuesCursor.java | 90 +++++++++++++++++++ .../installer/InstallManagerService.java | 36 +++++--- 4 files changed, 126 insertions(+), 13 deletions(-) create mode 100644 app/src/main/java/org/fdroid/fdroid/data/ContentValuesCursor.java diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index ee68ede09..377bf37bf 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -4,6 +4,7 @@ import android.annotation.TargetApi; import android.content.ContentValues; import android.database.Cursor; import android.os.Build; +import android.os.Parcelable; import org.fdroid.fdroid.Utils; @@ -49,7 +50,12 @@ public class Apk extends ValueObject implements Comparable { public String repoAddress; public Utils.CommaSeparatedList incompatibleReasons; - public Apk() { } + public Apk() { + } + + public Apk(Parcelable parcelable) { + this(new ContentValuesCursor((ContentValues) parcelable)); + } public Apk(Cursor cursor) { diff --git a/app/src/main/java/org/fdroid/fdroid/data/App.java b/app/src/main/java/org/fdroid/fdroid/data/App.java index e53460ab3..94e1af011 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/App.java +++ b/app/src/main/java/org/fdroid/fdroid/data/App.java @@ -8,6 +8,7 @@ import android.content.pm.FeatureInfo; import android.content.pm.PackageInfo; import android.content.pm.PackageManager; import android.database.Cursor; +import android.os.Parcelable; import android.text.TextUtils; import android.util.Log; @@ -124,6 +125,10 @@ public class App extends ValueObject implements Comparable { public App() { } + public App(Parcelable parcelable) { + this(new ContentValuesCursor((ContentValues) parcelable)); + } + public App(Cursor cursor) { checkCursorPosition(cursor); diff --git a/app/src/main/java/org/fdroid/fdroid/data/ContentValuesCursor.java b/app/src/main/java/org/fdroid/fdroid/data/ContentValuesCursor.java new file mode 100644 index 000000000..29ff7dfa5 --- /dev/null +++ b/app/src/main/java/org/fdroid/fdroid/data/ContentValuesCursor.java @@ -0,0 +1,90 @@ +package org.fdroid.fdroid.data; + +import android.content.ContentValues; +import android.database.AbstractCursor; +import android.database.Cursor; +import android.os.Bundle; + +import java.util.Map; + +/** + * In order to keep {@link App#App(Cursor)} and {@link Apk#Apk(Cursor)} as + * efficient as possible, this wrapper class is used to instantiate {@code App} + * and {@code Apk} from {@link App#toContentValues()} and + * {@link Apk#toContentValues()} included as extras {@link Bundle}s in the + * {@link android.content.Intent} that starts + * {@link org.fdroid.fdroid.installer.InstallManagerService} + *

+ * This implemented to throw an {@link IllegalArgumentException} if the types + * do not match what they are expected to be so that things fail fast. So that + * means only types used in {@link App#toContentValues()} and + * {@link Apk#toContentValues()} are implemented. + */ +public class ContentValuesCursor extends AbstractCursor { + + private final String[] keys; + private final Object[] values; + + public ContentValuesCursor(ContentValues contentValues) { + super(); + keys = new String[contentValues.size()]; + values = new Object[contentValues.size()]; + int i = 0; + for (Map.Entry entry : contentValues.valueSet()) { + keys[i] = entry.getKey(); + values[i] = entry.getValue(); + i++; + } + moveToFirst(); + } + + @Override + public int getCount() { + return 1; + } + + @Override + public String[] getColumnNames() { + return keys; + } + + @Override + public String getString(int i) { + return (String) values[i]; + } + + @Override + public int getInt(int i) { + if (values[i] instanceof Long) { + return ((Long) values[i]).intValue(); + } else if (values[i] instanceof Integer) { + return (int) values[i]; + } + throw new IllegalArgumentException("unimplemented"); + } + + @Override + public long getLong(int i) { + throw new IllegalArgumentException("unimplemented"); + } + + @Override + public short getShort(int i) { + throw new IllegalArgumentException("unimplemented"); + } + + @Override + public float getFloat(int i) { + throw new IllegalArgumentException("unimplemented"); + } + + @Override + public double getDouble(int i) { + throw new IllegalArgumentException("unimplemented"); + } + + @Override + public boolean isNull(int i) { + return values[i] == null; + } +} diff --git a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java index 322f8ab0e..7a2091a9c 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/InstallManagerService.java @@ -15,7 +15,6 @@ import android.support.v4.app.NotificationCompat; import android.support.v4.app.TaskStackBuilder; import android.support.v4.content.LocalBroadcastManager; import android.text.TextUtils; -import android.widget.Toast; import org.fdroid.fdroid.AppDetails; import org.fdroid.fdroid.R; @@ -36,10 +35,12 @@ import java.util.Set; * requests an APK to be installed. It handles checking whether the APK is cached, * downloading it, putting up and maintaining a {@link Notification}, and more. *

- * Data is sent via {@link Intent}s so that Android handles the message queuing - * and {@link Service} lifecycle for us, although it adds one layer of redirection - * between the static method to send the {@code Intent} and the method to - * actually process it. + * The {@link App} and {@link Apk} instances are sent via + * {@link Intent#putExtra(String, android.os.Bundle)} + * so that Android handles the message queuing and {@link Service} lifecycle for us. + * For example, if this {@code InstallManagerService} gets killed, Android will cache + * and then redeliver the {@link Intent} for us, which includes all of the data needed + * for {@code InstallManagerService} to do its job for the whole lifecycle of an install. *

* The full URL for the APK file to download is also used as the unique ID to * represent the download itself throughout F-Droid. This follows the model @@ -59,7 +60,10 @@ import java.util.Set; public class InstallManagerService extends Service { public static final String TAG = "InstallManagerService"; - private static final String ACTION_INSTALL = "org.fdroid.fdroid.InstallManagerService.action.INSTALL"; + private static final String ACTION_INSTALL = "org.fdroid.fdroid.installer.action.INSTALL"; + + private static final String EXTRA_APP = "org.fdroid.fdroid.installer.extra.APP"; + private static final String EXTRA_APK = "org.fdroid.fdroid.installer.extra.APK"; /** * The collection of {@link Apk}s that are actively going through this whole process, @@ -145,14 +149,21 @@ public class InstallManagerService extends Service { return START_NOT_STICKY; } - Apk apk = ACTIVE_APKS.get(urlString); - if (apk == null) { - Utils.debugLog(TAG, urlString + " is not in ACTIVE_APKS, why are we trying to download it?"); - Toast.makeText(this, urlString + " failed with an imcomplete download request!", - Toast.LENGTH_LONG).show(); + if (!intent.hasExtra(EXTRA_APP) || !intent.hasExtra(EXTRA_APK)) { + Utils.debugLog(TAG, urlString + " did not include both an App and Apk instance, ignoring"); return START_NOT_STICKY; } + if (!DownloaderService.isQueuedOrActive(urlString)) { + Utils.debugLog(TAG, urlString + " finished downloading while InstallManagerService was killed."); + cancelNotification(urlString); + return START_NOT_STICKY; + } + + App app = new App(intent.getParcelableExtra(EXTRA_APP)); + Apk apk = new Apk(intent.getParcelableExtra(EXTRA_APK)); + addToActive(urlString, app, apk); + Notification notification = createNotification(intent.getDataString(), apk).build(); notificationManager.notify(urlString.hashCode(), notification); @@ -357,10 +368,11 @@ public class InstallManagerService extends Service { public static void queue(Context context, App app, Apk apk) { String urlString = apk.getUrl(); Utils.debugLog(TAG, "queue " + app.packageName + " " + apk.versionCode + " from " + urlString); - addToActive(urlString, app, apk); Intent intent = new Intent(context, InstallManagerService.class); intent.setAction(ACTION_INSTALL); intent.setData(Uri.parse(urlString)); + intent.putExtra(EXTRA_APP, app.toContentValues()); + intent.putExtra(EXTRA_APK, apk.toContentValues()); context.startService(intent); } From 0ab80e4c6ac67508b71a8152aeb93da356c8376e Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 19 May 2016 20:42:01 +0200 Subject: [PATCH 2/3] delete the APK copy that Installer instances make Installer instances always copy the APK to a safe place to run the install from. That copy needs to be deleted. Until we have the whole lifecycle in InstallManagerService, we need this hack. It should be handled on the broadcast from InstallerService to say that its complete. #611 !300 --- .../org/fdroid/fdroid/CleanCacheService.java | 26 ++++++++++++++++--- .../fdroid/fdroid/installer/Installer.java | 18 ++++++++++++- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/CleanCacheService.java b/app/src/main/java/org/fdroid/fdroid/CleanCacheService.java index 2feb9d73d..1f817f03e 100644 --- a/app/src/main/java/org/fdroid/fdroid/CleanCacheService.java +++ b/app/src/main/java/org/fdroid/fdroid/CleanCacheService.java @@ -7,7 +7,6 @@ import android.content.Context; import android.content.Intent; import android.os.Process; import android.os.SystemClock; -import android.util.Log; import org.apache.commons.io.FileUtils; @@ -20,7 +19,6 @@ import java.io.File; * {@link FDroidApp#onCreate()} */ public class CleanCacheService extends IntentService { - private static final String TAG = "CleanCacheService"; /** * Schedule or cancel this service to update the app index, according to the @@ -33,7 +31,6 @@ public class CleanCacheService extends IntentService { if (keepTime < interval) { interval = keepTime * 1000; } - Log.i(TAG, "schedule " + keepTime + " " + interval); Intent intent = new Intent(context, CleanCacheService.class); PendingIntent pending = PendingIntent.getService(context, 0, intent, 0); @@ -53,6 +50,29 @@ public class CleanCacheService extends IntentService { Process.setThreadPriority(Process.THREAD_PRIORITY_LOWEST); Utils.clearOldFiles(Utils.getApkCacheDir(this), Preferences.get().getKeepCacheTime()); deleteStrayIndexFiles(); + deleteOldInstallerFiles(); + } + + /** + * {@link org.fdroid.fdroid.installer.Installer} instances copy the APK into + * a safe place before installing. It doesn't clean up them reliably yet. + */ + private void deleteOldInstallerFiles() { + File filesDir = getFilesDir(); + if (filesDir == null) { + return; + } + + final File[] files = filesDir.listFiles(); + if (files == null) { + return; + } + + for (File f : files) { + if (f.getName().startsWith("install-")) { + FileUtils.deleteQuietly(f); + } + } } /** diff --git a/app/src/main/java/org/fdroid/fdroid/installer/Installer.java b/app/src/main/java/org/fdroid/fdroid/installer/Installer.java index daf27221d..8f4011190 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/Installer.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/Installer.java @@ -170,7 +170,7 @@ public abstract class Installer { */ public void installPackage(File apkFile, String packageName, String urlString) throws InstallFailedException { - SanitizedFile apkToInstall; + SanitizedFile apkToInstall = null; try { Map attributes = AndroidXMLDecompress.getManifestHeaderAttributes(apkFile.getAbsolutePath()); @@ -232,6 +232,22 @@ public abstract class Installer { throw new InstallFailedException(e); } catch (ClassCastException e) { throw new InstallFailedException("F-Droid Privileged can only be updated using an activity!"); + } finally { + // 20 minutes the start of the install process, delete the file + final File apkToDelete = apkToInstall; + new Thread() { + @Override + public void run() { + android.os.Process.setThreadPriority(android.os.Process.THREAD_PRIORITY_LOWEST); + try { + Thread.sleep(1200000); + } catch (InterruptedException e) { + e.printStackTrace(); + } finally { + FileUtils.deleteQuietly(apkToDelete); + } + } + }.start(); } } From e1f65cab620d9e6eedcf999aefccaa603b294863 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 19 May 2016 23:17:03 +0200 Subject: [PATCH 3/3] prevent divide-by-zero errors when showing update download progress --- app/src/main/java/org/fdroid/fdroid/UpdateService.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/fdroid/fdroid/UpdateService.java b/app/src/main/java/org/fdroid/fdroid/UpdateService.java index 3f163f438..0d189b4fc 100644 --- a/app/src/main/java/org/fdroid/fdroid/UpdateService.java +++ b/app/src/main/java/org/fdroid/fdroid/UpdateService.java @@ -512,7 +512,10 @@ public class UpdateService extends IntentService { public void onProgress(URL sourceUrl, int bytesRead, int totalBytes) { Log.i(TAG, "downloadProgressReceiver " + sourceUrl); String downloadedSizeFriendly = Utils.getFriendlySize(bytesRead); - int percent = (int) ((double) bytesRead / totalBytes * 100); + int percent = -1; + if (totalBytes > 0) { + percent = (int) ((double) bytesRead / totalBytes * 100); + } String message; if (totalBytes == -1) { message = getString(R.string.status_download_unknown_size, sourceUrl, downloadedSizeFriendly);