diff --git a/app/src/androidTest/java/org/fdroid/fdroid/net/DownloaderServiceTest.java b/app/src/androidTest/java/org/fdroid/fdroid/net/DownloaderServiceTest.java deleted file mode 100644 index d859d7c0d..000000000 --- a/app/src/androidTest/java/org/fdroid/fdroid/net/DownloaderServiceTest.java +++ /dev/null @@ -1,41 +0,0 @@ - -package org.fdroid.fdroid.net; - -import android.content.BroadcastReceiver; -import android.content.Context; -import android.content.Intent; -import android.content.IntentFilter; -import android.support.v4.content.LocalBroadcastManager; -import android.test.ServiceTestCase; -import android.util.Log; - -@SuppressWarnings("PMD") // TODO port this to JUnit 4 semantics -public class DownloaderServiceTest extends ServiceTestCase { - public static final String TAG = "DownloaderServiceTest"; - - String[] urls = { - "https://en.wikipedia.org/wiki/Index.html", - "https://mirrors.kernel.org/debian/dists/stable/Release", - "https://f-droid.org/archive/de.we.acaldav_5.apk", - // sites that use SNI for HTTPS - "https://guardianproject.info/fdroid/repo/index.jar", - }; - - public DownloaderServiceTest() { - super(DownloaderService.class); - } - - public void testQueueingDownload() throws InterruptedException { - LocalBroadcastManager localBroadcastManager = LocalBroadcastManager.getInstance(getContext()); - localBroadcastManager.registerReceiver(new BroadcastReceiver() { - @Override - public void onReceive(Context context, Intent intent) { - Log.i(TAG, "onReceive " + intent); - } - }, new IntentFilter(Downloader.ACTION_PROGRESS)); - for (String url : urls) { - DownloaderService.queue(getContext(), null, url); - } - Thread.sleep(30000); - } -} diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 87d8cf851..ced498306 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -442,9 +442,6 @@ - diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index 8142f62e4..88d17a270 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -1443,10 +1443,11 @@ public class AppDetails extends AppCompatActivity { if (appDetails.activeDownloadUrlString != null) { // We don't actually know what the current progress is, so this will show an indeterminate // progress bar until the first progress/complete event we receive. - final String message = DownloaderService.isQueued(appDetails.activeDownloadUrlString) - ? getString(R.string.download_pending) - : ""; - showIndeterminateProgress(message); + if (DownloaderService.isQueuedOrActive(appDetails.activeDownloadUrlString)) { + showIndeterminateProgress(getString(R.string.download_pending)); + } else { + showIndeterminateProgress(""); + } } } diff --git a/app/src/main/java/org/fdroid/fdroid/Utils.java b/app/src/main/java/org/fdroid/fdroid/Utils.java index 2e7ceb163..d0dfa9b1a 100644 --- a/app/src/main/java/org/fdroid/fdroid/Utils.java +++ b/app/src/main/java/org/fdroid/fdroid/Utils.java @@ -67,7 +67,6 @@ import java.util.Formatter; import java.util.Iterator; import java.util.List; import java.util.Locale; -import java.util.zip.Adler32; public final class Utils { @@ -439,15 +438,6 @@ public final class Utils { return repoAddress + "/" + apk.apkName.replace(" ", "%20"); } - /** - * This generates a unique, reproducible ID for notifications related to {@code urlString} - */ - public static int getApkUrlNotificationId(String urlString) { - Adler32 checksum = new Adler32(); - checksum.update(urlString.getBytes()); - return (int) checksum.getValue(); - } - public static final class CommaSeparatedList implements Iterable { private final String value; 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 8a2e21ed6..41266f021 100644 --- a/app/src/main/java/org/fdroid/fdroid/installer/Installer.java +++ b/app/src/main/java/org/fdroid/fdroid/installer/Installer.java @@ -227,7 +227,7 @@ public abstract class Installer { NotificationManager nm = (NotificationManager) mContext.getSystemService(Context.NOTIFICATION_SERVICE); - nm.cancel(Utils.getApkUrlNotificationId(urlString)); + nm.cancel(urlString.hashCode()); } catch (NumberFormatException | NoSuchAlgorithmException | IOException e) { throw new InstallFailedException(e); } catch (ClassCastException e) { diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java deleted file mode 100644 index fa5029d94..000000000 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloadCompleteService.java +++ /dev/null @@ -1,76 +0,0 @@ -package org.fdroid.fdroid.net; - -import android.app.IntentService; -import android.app.NotificationManager; -import android.content.Context; -import android.content.Intent; -import android.content.pm.PackageManager; -import android.net.Uri; -import android.os.Process; -import android.support.v4.app.NotificationCompat; -import android.text.TextUtils; - -import org.fdroid.fdroid.R; -import org.fdroid.fdroid.Utils; -import org.fdroid.fdroid.data.App; -import org.fdroid.fdroid.data.AppProvider; - -public class DownloadCompleteService extends IntentService { - private static final String TAG = "DownloadCompleteService"; - - private static final String ACTION_NOTIFY = "org.fdroid.fdroid.net.action.NOTIFY"; - private static final String EXTRA_PACKAGE_NAME = "org.fdroid.fdroid.net.extra.PACKAGE_NAME"; - - public DownloadCompleteService() { - super("DownloadCompleteService"); - } - - public static void notify(Context context, String packageName, String urlString) { - Intent intent = new Intent(context, DownloadCompleteService.class); - intent.setAction(ACTION_NOTIFY); - intent.setData(Uri.parse(urlString)); - intent.putExtra(EXTRA_PACKAGE_NAME, packageName); - context.startService(intent); - } - - @Override - protected void onHandleIntent(Intent intent) { - Process.setThreadPriority(Process.THREAD_PRIORITY_LOWEST); - if (intent != null) { - final String action = intent.getAction(); - if (!ACTION_NOTIFY.equals(action)) { - Utils.debugLog(TAG, "Intent action is not ACTION_NOTIFY"); - return; - } - String packageName = intent.getStringExtra(EXTRA_PACKAGE_NAME); - if (TextUtils.isEmpty(packageName)) { - Utils.debugLog(TAG, "Intent is missing EXTRA_PACKAGE_NAME"); - return; - } - - String title; - try { - PackageManager pm = getPackageManager(); - title = String.format(getString(R.string.tap_to_update_format), - pm.getApplicationLabel(pm.getApplicationInfo(packageName, 0))); - } catch (PackageManager.NameNotFoundException e) { - App app = AppProvider.Helper.findByPackageName(getContentResolver(), packageName, - new String[]{ - AppProvider.DataColumns.NAME, - }); - title = String.format(getString(R.string.tap_to_install_format), app.name); - } - - int requestCode = Utils.getApkUrlNotificationId(intent.getDataString()); - NotificationCompat.Builder builder = - new NotificationCompat.Builder(this) - .setAutoCancel(true) - .setContentTitle(title) - .setSmallIcon(android.R.drawable.stat_sys_download_done) - .setContentIntent(DownloaderService.createAppDetailsIntent(this, requestCode, packageName)) - .setContentText(getString(R.string.tap_to_install)); - NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - nm.notify(Utils.getApkUrlNotificationId(intent.getDataString()), builder.build()); - } - } -} diff --git a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java index 86e7f2685..7981c2571 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java @@ -30,7 +30,6 @@ public abstract class Downloader { private volatile boolean cancelled = false; private volatile int bytesRead; private volatile int totalBytes; - private Timer timer; public final File outputFile; @@ -49,7 +48,7 @@ public abstract class Downloader { /** * For sending download progress, should only be called in {@link #progressTask} */ - private DownloaderProgressListener downloaderProgressListener; + private volatile DownloaderProgressListener downloaderProgressListener; protected abstract InputStream getDownloadersInputStream() throws IOException; @@ -131,9 +130,6 @@ public abstract class Downloader { private void throwExceptionIfInterrupted() throws InterruptedException { if (cancelled) { Utils.debugLog(TAG, "Received interrupt, cancelling download"); - if (timer != null) { - timer.cancel(); - } throw new InterruptedException(); } } @@ -151,40 +147,44 @@ public abstract class Downloader { * progress counter. */ private void copyInputToOutputStream(InputStream input, int bufferSize, OutputStream output) throws IOException, InterruptedException { - bytesRead = 0; - totalBytes = totalDownloadSize(); - byte[] buffer = new byte[bufferSize]; + Timer timer = new Timer(); + try { + bytesRead = 0; + totalBytes = totalDownloadSize(); + byte[] buffer = new byte[bufferSize]; - timer = new Timer(); - timer.scheduleAtFixedRate(progressTask, 0, 100); - - // Getting the total download size could potentially take time, depending on how - // it is implemented, so we may as well check this before we proceed. - throwExceptionIfInterrupted(); - - while (true) { - - int count; - if (input.available() > 0) { - int readLength = Math.min(input.available(), buffer.length); - count = input.read(buffer, 0, readLength); - } else { - count = input.read(buffer); - } + timer.scheduleAtFixedRate(progressTask, 0, 100); + // Getting the total download size could potentially take time, depending on how + // it is implemented, so we may as well check this before we proceed. throwExceptionIfInterrupted(); - if (count == -1) { - Utils.debugLog(TAG, "Finished downloading from stream"); - break; + while (true) { + + int count; + if (input.available() > 0) { + int readLength = Math.min(input.available(), buffer.length); + count = input.read(buffer, 0, readLength); + } else { + count = input.read(buffer); + } + + throwExceptionIfInterrupted(); + + if (count == -1) { + Utils.debugLog(TAG, "Finished downloading from stream"); + break; + } + bytesRead += count; + output.write(buffer, 0, count); } - bytesRead += count; - output.write(buffer, 0, count); + } finally { + downloaderProgressListener = null; + timer.cancel(); + timer.purge(); + output.flush(); + output.close(); } - timer.cancel(); - timer.purge(); - output.flush(); - output.close(); } /** diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java index b8d93c0d0..0f53a4693 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderService.java @@ -24,6 +24,7 @@ import android.app.Service; import android.content.Context; import android.content.Intent; import android.content.IntentFilter; +import android.content.pm.PackageManager; import android.net.Uri; import android.os.Handler; import android.os.HandlerThread; @@ -52,7 +53,6 @@ import org.fdroid.fdroid.data.SanitizedFile; import java.io.File; import java.io.IOException; import java.net.URL; -import java.util.HashMap; /** * DownloaderService is a service that handles asynchronous download requests @@ -60,24 +60,28 @@ import java.util.HashMap; * through {@link android.content.Context#startService(Intent)} calls; the * service is started as needed, handles each Intent in turn using a worker * thread, and stops itself when it runs out of work. - *

- *

This "work queue processor" pattern is commonly used to offload tasks + *

+ * This "work queue processor" pattern is commonly used to offload tasks * from an application's main thread. The DownloaderService class exists to * simplify this pattern and take care of the mechanics. DownloaderService * will receive the Intents, launch a worker thread, and stop the service as * appropriate. - *

- *

All requests are handled on a single worker thread -- they may take as + *

+ * All requests are handled on a single worker thread -- they may take as * long as necessary (and will not block the application's main loop), but * only one request will be processed at a time. - *

- *

- *

Developer Guides

- *

For a detailed discussion about how to create services, read the - * Services developer guide.

- *
+ *

+ * The full URL for the file to download is also used as the unique ID to + * represent the download itself throughout F-Droid. This follows the model + * of {@link Intent#setData(Uri)}, where the core data of an {@code Intent} is + * a {@code Uri}. For places that need an {@code int} ID, + * {@link String#hashCode()} should be used to get a reproducible, unique {@code int} + * from any {@code urlString}. The full URL is guaranteed to be unique since + * it points to a file on a filesystem. This is more important with media files + * than with APKs since there is not reliable standard for a unique ID for + * media files, unlike APKs with {@code packageName} and {@code versionCode}. * - * @see android.os.AsyncTask + * @see android.app.IntentService */ public class DownloaderService extends Service { private static final String TAG = "DownloaderService"; @@ -94,9 +98,6 @@ public class DownloaderService extends Service { private static volatile Downloader downloader; private LocalBroadcastManager localBroadcastManager; - private static final HashMap QUEUE_WHATS = new HashMap<>(); - private int what; - private final class ServiceHandler extends Handler { ServiceHandler(Looper looper) { super(looper); @@ -133,40 +134,30 @@ public class DownloaderService extends Service { } if (ACTION_CANCEL.equals(intent.getAction())) { Utils.debugLog(TAG, "Cancelling download of " + uriString); - if (isQueued(uriString)) { - serviceHandler.removeMessages(what); + Integer whatToRemove = uriString.hashCode(); + if (serviceHandler.hasMessages(whatToRemove)) { + serviceHandler.removeMessages(whatToRemove); } else if (isActive(uriString)) { downloader.cancelDownload(); } else { Log.e(TAG, "ACTION_CANCEL called on something not queued or running"); } - - QUEUE_WHATS.remove(uriString); - if (isQueueEmpty()) { - stopForeground(true); - } } else if (ACTION_QUEUE.equals(intent.getAction())) { Message msg = serviceHandler.obtainMessage(); msg.arg1 = startId; msg.obj = intent; - msg.what = what++; + msg.what = uriString.hashCode(); serviceHandler.sendMessage(msg); - QUEUE_WHATS.put(uriString, msg.what); - Utils.debugLog(TAG, "Queued download of " + uriString + ". Now " + QUEUE_WHATS.size() + " downloads in the queue"); + Utils.debugLog(TAG, "Queued download of " + uriString); } else { Log.e(TAG, "Received Intent with unknown action: " + intent); } } - @Nullable - private static String getPackageNameFromIntent(@NonNull Intent intent) { - return intent.hasExtra(EXTRA_PACKAGE_NAME) ? intent.getStringExtra(EXTRA_PACKAGE_NAME) : null; - } - private NotificationCompat.Builder createNotification(String urlString, @Nullable String packageName) { return new NotificationCompat.Builder(this) .setAutoCancel(true) - .setContentIntent(createAppDetailsIntent(this, 0, packageName)) + .setContentIntent(createAppDetailsIntent(0, packageName)) .setContentTitle(getNotificationTitle(packageName)) .addAction(R.drawable.ic_cancel_black_24dp, getString(R.string.cancel), createCancelDownloadIntent(this, 0, urlString)) @@ -191,20 +182,20 @@ public class DownloaderService extends Service { return getString(R.string.downloading); } - public static PendingIntent createAppDetailsIntent(@NonNull Context context, int requestCode, @Nullable String packageName) { + private PendingIntent createAppDetailsIntent(int requestCode, String packageName) { TaskStackBuilder stackBuilder; if (packageName != null) { - Intent notifyIntent = new Intent(context.getApplicationContext(), AppDetails.class) + Intent notifyIntent = new Intent(getApplicationContext(), AppDetails.class) .putExtra(AppDetails.EXTRA_APPID, packageName); stackBuilder = TaskStackBuilder - .create(context.getApplicationContext()) + .create(getApplicationContext()) .addParentStack(AppDetails.class) .addNextIntent(notifyIntent); } else { - Intent notifyIntent = new Intent(context.getApplicationContext(), FDroid.class); + Intent notifyIntent = new Intent(getApplicationContext(), FDroid.class); stackBuilder = TaskStackBuilder - .create(context.getApplicationContext()) + .create(getApplicationContext()) .addParentStack(FDroid.class) .addNextIntent(notifyIntent); } @@ -266,11 +257,11 @@ public class DownloaderService extends Service { File downloadDir = new File(Utils.getApkCacheDir(this), uri.getHost() + "-" + uri.getPort()); downloadDir.mkdirs(); final SanitizedFile localFile = new SanitizedFile(downloadDir, uri.getLastPathSegment()); - final String packageName = getPackageNameFromIntent(intent); + final String packageName = intent.getStringExtra(EXTRA_PACKAGE_NAME); sendBroadcast(uri, Downloader.ACTION_STARTED, localFile); if (Preferences.get().isUpdateNotificationEnabled()) { - Notification notification = createNotification(intent.getDataString(), getPackageNameFromIntent(intent)).build(); + Notification notification = createNotification(intent.getDataString(), intent.getStringExtra(EXTRA_PACKAGE_NAME)).build(); startForeground(NOTIFY_DOWNLOADING, notification); } @@ -279,26 +270,24 @@ public class DownloaderService extends Service { downloader.setListener(new Downloader.DownloaderProgressListener() { @Override public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { - if (isActive(uri.toString())) { - Intent intent = new Intent(Downloader.ACTION_PROGRESS); - intent.setData(uri); - intent.putExtra(Downloader.EXTRA_BYTES_READ, bytesRead); - intent.putExtra(Downloader.EXTRA_TOTAL_BYTES, totalBytes); - localBroadcastManager.sendBroadcast(intent); + Intent intent = new Intent(Downloader.ACTION_PROGRESS); + intent.setData(uri); + intent.putExtra(Downloader.EXTRA_BYTES_READ, bytesRead); + intent.putExtra(Downloader.EXTRA_TOTAL_BYTES, totalBytes); + localBroadcastManager.sendBroadcast(intent); - if (Preferences.get().isUpdateNotificationEnabled()) { - NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); - Notification notification = createNotification(uri.toString(), packageName) - .setProgress(totalBytes, bytesRead, false) - .build(); - nm.notify(NOTIFY_DOWNLOADING, notification); - } + if (Preferences.get().isUpdateNotificationEnabled()) { + NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + Notification notification = createNotification(uri.toString(), packageName) + .setProgress(totalBytes, bytesRead, false) + .build(); + nm.notify(NOTIFY_DOWNLOADING, notification); } } }); downloader.download(); sendBroadcast(uri, Downloader.ACTION_COMPLETE, localFile); - DownloadCompleteService.notify(this, packageName, intent.getDataString()); + notifyDownloadComplete(packageName, intent.getDataString()); } catch (InterruptedException e) { sendBroadcast(uri, Downloader.ACTION_INTERRUPTED, localFile); } catch (IOException e) { @@ -309,14 +298,40 @@ public class DownloaderService extends Service { if (downloader != null) { downloader.close(); } - // May have already been removed in response to a cancel intent, but that wont cause - // problems if we ask to remove it again. - QUEUE_WHATS.remove(uri.toString()); - stopForeground(true); } downloader = null; } + /** + * Post a notification about a completed download. {@code packageName} must be a valid + * and currently in the app index database. + */ + private void notifyDownloadComplete(String packageName, String urlString) { + String title; + try { + PackageManager pm = getPackageManager(); + title = String.format(getString(R.string.tap_to_update_format), + pm.getApplicationLabel(pm.getApplicationInfo(packageName, 0))); + } catch (PackageManager.NameNotFoundException e) { + App app = AppProvider.Helper.findByPackageName(getContentResolver(), packageName, + new String[]{ + AppProvider.DataColumns.NAME, + }); + title = String.format(getString(R.string.tap_to_install_format), app.name); + } + + int downloadUrlId = urlString.hashCode(); + NotificationCompat.Builder builder = + new NotificationCompat.Builder(this) + .setAutoCancel(true) + .setContentTitle(title) + .setSmallIcon(android.R.drawable.stat_sys_download_done) + .setContentIntent(createAppDetailsIntent(downloadUrlId, packageName)) + .setContentText(getString(R.string.tap_to_install)); + NotificationManager nm = (NotificationManager) getSystemService(NOTIFICATION_SERVICE); + nm.notify(downloadUrlId, builder.build()); + } + private void sendBroadcast(Uri uri, String action, File file) { sendBroadcast(uri, action, file, null); } @@ -336,7 +351,7 @@ public class DownloaderService extends Service { *

* All notifications are sent as an {@link Intent} via local broadcasts to be received by * - * @param context + * @param context this app's {@link Context} * @param packageName The packageName of the app being downloaded * @param urlString The URL to add to the download queue * @see #cancel(Context, String) @@ -357,7 +372,7 @@ public class DownloaderService extends Service { *

* All notifications are sent as an {@link Intent} via local broadcasts to be received by * - * @param context + * @param context this app's {@link Context} * @param urlString The URL to remove from the download queue * @see #queue(Context, String, String) */ @@ -373,34 +388,19 @@ public class DownloaderService extends Service { * Check if a URL is waiting in the queue for downloading or if actively being downloaded. * This is useful for checking whether to re-register {@link android.content.BroadcastReceiver}s * in {@link android.app.Activity#onResume()}. - * - * @see DownloaderService#isQueued(String) - * @see DownloaderService#isActive(String) */ public static boolean isQueuedOrActive(String urlString) { - return isQueued(urlString) || isActive(urlString); - } - - public static boolean isQueueEmpty() { - return QUEUE_WHATS.isEmpty(); - } - - /** - * Check if a URL is waiting in the queue for downloading. - */ - public static boolean isQueued(String urlString) { - if (TextUtils.isEmpty(urlString)) { + if (TextUtils.isEmpty(urlString)) { //NOPMD - suggests unreadable format return false; } - Integer what = QUEUE_WHATS.get(urlString); - return what != null && serviceHandler.hasMessages(what); + return serviceHandler.hasMessages(urlString.hashCode()) || isActive(urlString); } /** * Check if a URL is actively being downloaded. */ public static boolean isActive(String urlString) { - return downloader != null && QUEUE_WHATS.containsKey(urlString) && TextUtils.equals(urlString, downloader.sourceUrl.toString()); + return downloader != null && TextUtils.equals(urlString, downloader.sourceUrl.toString()); } /** @@ -409,7 +409,6 @@ public class DownloaderService extends Service { * @param urlString The full file URL to match. * @param action {@link Downloader#ACTION_STARTED}, {@link Downloader#ACTION_PROGRESS}, * {@link Downloader#ACTION_INTERRUPTED}, or {@link Downloader#ACTION_COMPLETE}, - * @return */ public static IntentFilter getIntentFilter(String urlString, String action) { Uri uri = Uri.parse(urlString); diff --git a/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java index 11d87a8b3..5cc5f75ea 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java @@ -120,7 +120,7 @@ public class HttpDownloader extends Downloader { // workaround until NetCipher supports HTTPS SNI // https://gitlab.com/fdroid/fdroidclient/issues/431 if (connection instanceof HttpsURLConnection - && "f-droid.org".equals(sourceUrl.getHost())) { + && !"f-droid.org".equals(sourceUrl.getHost())) { ((HttpsURLConnection) connection).setSSLSocketFactory(HttpsURLConnection.getDefaultSSLSocketFactory()); }