Downloading of a file has a small chance of failing randomly.

Due to timing issues a download could be cancelled. This happens when the
downloading state machine gets in the wrong state. There are two variables
in the state machine which gets out of sync with each other: status and
mHasActiveThread.
The DownloadService thread reads several downloads from the database
and starts to process them. For each applicable download a DowloadThread
is started. For that download mHasActiveThread is set to true and the
download is started. When the download is finished the correct status
is written to the database and mHasActiveThread is set to false.
The problem is that mHasActiveThread changes value instantly in
DownloadService but the status for all downloads are read from the
database at certain times. If the DownloadThread changes the value of
mHasActiveThread before that download has been processed in the
DownloadService thread the value of mHasActiveThread will be up to date
but the status value will be out of sync.
The download will then be restarted even if it is already completed. The
DownloadService will interpred the download as a resume but the server will
handle it as a new download. The downloading statemachine will then be
confused and terminate the download.
This is fixed by synching the changing of status and mHasActiveThread between
DownloadService and DownloadThread. The database information in DowloadService
will also be re-read when it is not up to date.

Change-Id: I8bb28b42b434b424187c918dfded4fdf3baa055b

6 files changed, 131 insertions, 71 deletions

diff --git a/src/com/android/providers/downloads/DownloadInfo.java b/src/com/android/providers/downloads/DownloadInfo.java
index 7b291683..c0b1b8a2 100644
--- a/src/com/android/providers/downloads/DownloadInfo.java
+++ b/src/com/android/providers/downloads/DownloadInfo.java
@@ -471,8 +471,12 @@ public class DownloadInfo {
             mContext.getContentResolver().update(getAllDownloadsUri(), values, null, null);
         }
         DownloadThread downloader = new DownloadThread(mContext, mSystemFacade, this);
-        mHasActiveThread = true;
-        mSystemFacade.startThread(downloader);
+        // This needs to be synchronized with the Update thread to keep the system from
+        // gettings out of sync.
+        synchronized (mSystemFacade) {
+            mHasActiveThread = true;
+            mSystemFacade.startThread(downloader);
+        }
     }
 
     public boolean isOnCache() {
diff --git a/src/com/android/providers/downloads/DownloadService.java b/src/com/android/providers/downloads/DownloadService.java
index 76a317d1..3707656e 100644
--- a/src/com/android/providers/downloads/DownloadService.java
+++ b/src/com/android/providers/downloads/DownloadService.java
@@ -287,16 +287,24 @@ public class DownloadService extends Service {
                     mPendingUpdate = false;
                 }
 
+                boolean downloadUpdateFromDatabaseAborted = false;
                 long now = mSystemFacade.currentTimeMillis();
                 boolean mustScan = false;
                 keepService = false;
                 wakeUp = Long.MAX_VALUE;
                 Set<Long> idsNoLongerInDatabase = new HashSet<Long>(mDownloads.keySet());
 
-                Cursor cursor = getContentResolver().query(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI,
-                        null, null, null, null);
-                if (cursor == null) {
-                    continue;
+                Cursor cursor = null;
+                // This section is synchronized to prevent the DownloadThread
+                // to change the SystemFacade variable while we are
+                // updating the data.
+                synchronized (mSystemFacade) {
+                    cursor = getContentResolver().query(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI,
+                            null, null, null, null);
+                    if (cursor == null) {
+                        continue;
+                    }
+                    mSystemFacade.setUpdateThreadDataIsOutdated(false);
                 }
                 try {
                     DownloadInfo.Reader reader =
@@ -304,82 +312,99 @@ public class DownloadService extends Service {
                     int idColumn = cursor.getColumnIndexOrThrow(Downloads.Impl._ID);
 
                     for (cursor.moveToFirst(); !cursor.isAfterLast(); cursor.moveToNext()) {
-                        long id = cursor.getLong(idColumn);
-                        idsNoLongerInDatabase.remove(id);
-                        DownloadInfo info = mDownloads.get(id);
-                        if (info != null) {
-                            updateDownload(reader, info, now);
-                        } else {
-                            info = insertDownload(reader, now);
-                        }
+                        // This section needs to be synchorized to prevent the DownloadThread
+                        // to change the data while we are using it.
+                        // If the DownloadThread has a pending change and the data is old
+                        // we will do the same thing we did last time.
+                        synchronized (mSystemFacade) {
+                            // The data we have read is no longer valid we need to abort
+                            // and start over to prevent using data with the wrong values.
+                            if (mSystemFacade.isUpdateThreadDataOutdated()) {
+                                mPendingUpdate = true;
+                                downloadUpdateFromDatabaseAborted = true;
+                                break;
+                            }
+                            long id = cursor.getLong(idColumn);
+                            idsNoLongerInDatabase.remove(id);
+                            DownloadInfo info = mDownloads.get(id);
+                            if (info != null) {
+                                updateDownload(reader, info, now);
+                            } else {
+                                info = insertDownload(reader, now);
+                            }
 
-                        if (info.shouldScanFile() && !scanFile(info, true, false)) {
-                            mustScan = true;
-                            keepService = true;
-                        }
-                        if (info.hasCompletionNotification()) {
-                            keepService = true;
-                        }
-                        long next = info.nextAction(now);
-                        if (next == 0) {
-                            keepService = true;
-                        } else if (next > 0 && next < wakeUp) {
-                            wakeUp = next;
+                            if (info.shouldScanFile() && !scanFile(info, true, false)) {
+                                mustScan = true;
+                                keepService = true;
+                            }
+                            if (info.hasCompletionNotification()) {
+                                keepService = true;
+                            }
+                            long next = info.nextAction(now);
+                            if (next == 0) {
+                                keepService = true;
+                            } else if (next > 0 && next < wakeUp) {
+                                wakeUp = next;
+                            }
                         }
                     }
                 } finally {
                     cursor.close();
                 }
 
-                for (Long id : idsNoLongerInDatabase) {
-                    deleteDownload(id);
-                }
+                if (!downloadUpdateFromDatabaseAborted) {
+                    for (Long id : idsNoLongerInDatabase) {
+                        deleteDownload(id);
+                    }
 
-                // is there a need to start the DownloadService? yes, if there are rows to be
-                // deleted.
-                if (!mustScan) {
-                    for (DownloadInfo info : mDownloads.values()) {
-                        if (info.mDeleted && TextUtils.isEmpty(info.mMediaProviderUri)) {
-                            mustScan = true;
-                            keepService = true;
-                            break;
+                    // is there a need to start the DownloadService? yes, if there are rows to be
+                    // deleted.
+                    if (!mustScan) {
+                        for (DownloadInfo info : mDownloads.values()) {
+                            if (info.mDeleted && TextUtils.isEmpty(info.mMediaProviderUri)) {
+                                mustScan = true;
+                                keepService = true;
+                                break;
+                            }
                         }
                     }
-                }
-                mNotifier.updateNotification(mDownloads.values());
-                if (mustScan) {
-                    bindMediaScanner();
-                } else {
-                    mMediaScannerConnection.disconnectMediaScanner();
-                }
+                    mNotifier.updateNotification(mDownloads.values());
+                    if (mustScan) {
+                        bindMediaScanner();
+                    } else {
+                        mMediaScannerConnection.disconnectMediaScanner();
+                    }
 
-                // look for all rows with deleted flag set and delete the rows from the database
-                // permanently
-                for (DownloadInfo info : mDownloads.values()) {
-                    if (info.mDeleted) {
-                        // this row is to be deleted from the database. but does it have
-                        // mediaProviderUri?
-                        if (TextUtils.isEmpty(info.mMediaProviderUri)) {
-                            if (info.shouldScanFile()) {
-                                // initiate rescan of the file to - which will populate
-                                // mediaProviderUri column in this row
-                                if (!scanFile(info, false, true)) {
-                                    throw new IllegalStateException("scanFile failed!");
+                    // look for all rows with deleted flag set and delete the rows from the database
+                    // permanently
+                    for (DownloadInfo info : mDownloads.values()) {
+                        if (info.mDeleted) {
+                            // this row is to be deleted from the database. but does it have
+                            // mediaProviderUri?
+                            if (TextUtils.isEmpty(info.mMediaProviderUri)) {
+                                if (info.shouldScanFile()) {
+                                    // initiate rescan of the file to - which will populate
+                                    // mediaProviderUri column in this row
+                                    if (!scanFile(info, false, true)) {
+                                        throw new IllegalStateException("scanFile failed!");
+                                    }
+                                } else {
+                                    // this file should NOT be scanned. delete the file.
+                                    Helpers.deleteFile(getContentResolver(), info.mId,
+                                            info.mFileName, info.mMimeType);
                                 }
                             } else {
-                                // this file should NOT be scanned. delete the file.
+                                // yes it has mediaProviderUri column already filled in.
+                                // delete it from MediaProvider database and then from downloads
+                                // table in DownProvider database (the order of deletion is
+                                // important).
+                                getContentResolver().delete(Uri.parse(info.mMediaProviderUri), null,
+                                        null);
+                                // the following deletes the file and then deletes it from
+                                // downloads db.
                                 Helpers.deleteFile(getContentResolver(), info.mId, info.mFileName,
                                         info.mMimeType);
                             }
-                        } else {
-                            // yes it has mediaProviderUri column already filled in.
-                            // delete it from MediaProvider database and then from downloads table
-                            // in DownProvider database (the order of deletion is important).
-                            getContentResolver().delete(Uri.parse(info.mMediaProviderUri), null,
-                                    null);
-                            // the following deletes the file and then deletes it from downloads db
-                            Helpers.deleteFile(getContentResolver(), info.mId, info.mFileName,
-                                    info.mMimeType);
                         }
                     }
                 }
diff --git a/src/com/android/providers/downloads/DownloadThread.java b/src/com/android/providers/downloads/DownloadThread.java
index 461d8cea..9176e5df 100644
--- a/src/com/android/providers/downloads/DownloadThread.java
+++ b/src/com/android/providers/downloads/DownloadThread.java
@@ -198,10 +198,16 @@ public class DownloadThread extends Thread {
                 client = null;
             }
             cleanupDestination(state, finalStatus);
-            notifyDownloadCompleted(finalStatus, state.mCountRetry, state.mRetryAfter,
-                                    state.mGotData, state.mFilename,
-                                    state.mNewUri, state.mMimeType);
-            mInfo.mHasActiveThread = false;
+            // We have written new information to the database, the data in the update thread
+            // is no longer valid. This needs to be synchronized to prevent the information
+            // to change while the Update Thread in Download Service is using it.
+            synchronized (mSystemFacade) {
+                notifyDownloadCompleted(finalStatus, state.mCountRetry, state.mRetryAfter,
+                        state.mGotData, state.mFilename,
+                        state.mNewUri, state.mMimeType);
+                mInfo.mHasActiveThread = false;
+                mSystemFacade.setUpdateThreadDataIsOutdated(true);
+            }
         }
     }
 
diff --git a/src/com/android/providers/downloads/RealSystemFacade.java b/src/com/android/providers/downloads/RealSystemFacade.java
index ce86f739..4d771060 100644
--- a/src/com/android/providers/downloads/RealSystemFacade.java
+++ b/src/com/android/providers/downloads/RealSystemFacade.java
@@ -15,6 +15,7 @@ import android.util.Log;
 class RealSystemFacade implements SystemFacade {
     private Context mContext;
     private NotificationManager mNotificationManager;
+    private boolean mUpdateThreadDataIsOutdated;
 
     public RealSystemFacade(Context context) {
         mContext = context;
@@ -114,4 +115,12 @@ class RealSystemFacade implements SystemFacade {
     public void startThread(Thread thread) {
         thread.start();
     }
+
+    public boolean isUpdateThreadDataOutdated() {
+        return mUpdateThreadDataIsOutdated;
+    }
+
+    public void setUpdateThreadDataIsOutdated(boolean dataIsOutdated) {
+        mUpdateThreadDataIsOutdated = dataIsOutdated;
+    }
 }
diff --git a/src/com/android/providers/downloads/SystemFacade.java b/src/com/android/providers/downloads/SystemFacade.java
index ed0d3306..1c790cfc 100644
--- a/src/com/android/providers/downloads/SystemFacade.java
+++ b/src/com/android/providers/downloads/SystemFacade.java
@@ -1,4 +1,3 @@
-
 package com.android.providers.downloads;
 
 import android.app.Notification;
@@ -65,4 +64,14 @@ interface SystemFacade {
      * Start a thread.
      */
     public void startThread(Thread thread);
+
+    /**
+     * @return true if the data read from the database in update thread is no longer up to date.
+     */
+    public boolean isUpdateThreadDataOutdated();
+
+    /**
+     * @param dataIsOutdated If the data read from the database in the update thread is outdated.
+     */
+    public void setUpdateThreadDataIsOutdated(boolean dataIsOutdated);
 }
diff --git a/tests/src/com/android/providers/downloads/FakeSystemFacade.java b/tests/src/com/android/providers/downloads/FakeSystemFacade.java
index 5263015c..47555260 100644
--- a/tests/src/com/android/providers/downloads/FakeSystemFacade.java
+++ b/tests/src/com/android/providers/downloads/FakeSystemFacade.java
@@ -91,4 +91,11 @@ public class FakeSystemFacade implements SystemFacade {
             mStartedThreads.poll().run();
         }
     }
+
+    public boolean isUpdateThreadDataOutdated() {
+        return false;
+    }
+
+    public void setUpdateThreadDataIsOutdated(boolean dataIsOutdated) {
+    }
 }