Fix DownloadManager assertion failure

Issue: #8419 PiperOrigin-RevId: 350134719

Fix DownloadManager assertion failure
Issue: #8419 PiperOrigin-RevId: 350134719
edc5b839 · olly · Ian Baker · ca287ba5 · edc5b839 · edc5b839
Commit edc5b839 authored Jan 05, 2021 by olly Committed by Ian Baker Jan 11, 2021
Showing with 81 additions and 17 deletions
RELEASENOTES.md
library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java
library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java
--- a/RELEASENOTES.md
+++ b/RELEASENOTES.md
@@ -12,6 +12,10 @@
        would not be dismissed when tapping outside of the menu area or pressing
        the back button, on API level 22 and earlier
        ([#8272](https://github.com/google/ExoPlayer/issues/8272)).
+*   Downloads:
+    *    Fix crash in `DownloadManager` that could occur when adding a stopped
+         download with the same ID as a download currently being removed
+         ([#8419](https://github.com/google/ExoPlayer/issues/8419)).
 *   IMA extension:
    *   Fix a condition where playback can get stuck before an empty ad
        ([#8205](https://github.com/google/ExoPlayer/issues/8205)).
@@ -147,7 +151,7 @@
        ([#7378](https://github.com/google/ExoPlayer/issues/7378)).
 *   Downloads: Fix issue retrying progressive downloads, which could also result
    in a crash in `DownloadManager.InternalHandler.onContentLengthChanged`
-    ([#8078](https://github.com/google/ExoPlayer/issues/8078).
+    ([#8078](https://github.com/google/ExoPlayer/issues/8078)).
 *   HLS: Fix crash affecting chunkful preparation of master playlists that start
    with an I-FRAME only variant
    ([#8025](https://github.com/google/ExoPlayer/issues/8025)).

--- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java
+++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadManager.java
@@ -856,7 +856,7 @@ public final class DownloadManager {
    private void setStopReason(Download download, int stopReason) {
      if (stopReason == STOP_REASON_NONE) {
        if (download.state == STATE_STOPPED) {
-          putDownloadWithState(download, STATE_QUEUED);
+          putDownloadWithState(download, STATE_QUEUED, STOP_REASON_NONE);
        }
      } else if (stopReason != download.stopReason) {
        @Download.State int state = download.state;
@@ -910,7 +910,7 @@ public final class DownloadManager {
        Log.e(TAG, "Failed to remove nonexistent download: " + id);
        return;
      }
-      putDownloadWithState(download, STATE_REMOVING);
+      putDownloadWithState(download, STATE_REMOVING, STOP_REASON_NONE);
      syncTasks();
    }
@@ -924,10 +924,11 @@ public final class DownloadManager {
        Log.e(TAG, "Failed to load downloads.");
      }
      for (int i = 0; i < downloads.size(); i++) {
-        downloads.set(i, copyDownloadWithState(downloads.get(i), STATE_REMOVING));
+        downloads.set(i, copyDownloadWithState(downloads.get(i), STATE_REMOVING, STOP_REASON_NONE));
      }
      for (int i = 0; i < terminalDownloads.size(); i++) {
-        downloads.add(copyDownloadWithState(terminalDownloads.get(i), STATE_REMOVING));
+        downloads.add(
+            copyDownloadWithState(terminalDownloads.get(i), STATE_REMOVING, STOP_REASON_NONE));
      }
      Collections.sort(downloads, InternalHandler::compareStartTimes);
      try {
@@ -1019,7 +1020,7 @@ public final class DownloadManager {
      }
      // We can start a download task.
-      download = putDownloadWithState(download, STATE_DOWNLOADING);
+      download = putDownloadWithState(download, STATE_DOWNLOADING, STOP_REASON_NONE);
      Downloader downloader = downloaderFactory.createDownloader(download.request);
      activeTask =
          new Task(
@@ -1041,7 +1042,7 @@ public final class DownloadManager {
        Task activeTask, Download download, int accumulatingDownloadTaskCount) {
      Assertions.checkState(!activeTask.isRemove);
      if (!canDownloadsRun() || accumulatingDownloadTaskCount >= maxParallelDownloads) {
-        putDownloadWithState(download, STATE_QUEUED);
+        putDownloadWithState(download, STATE_QUEUED, STOP_REASON_NONE);
        activeTask.cancel(/* released= */ false);
      }
    }
@@ -1161,8 +1162,9 @@ public final class DownloadManager {
    private void onRemoveTaskStopped(Download download) {
      if (download.state == STATE_RESTARTING) {
-        putDownloadWithState(
+        @Download.State
-            download, download.stopReason == STOP_REASON_NONE ? STATE_QUEUED : STATE_STOPPED);
+        int state = download.stopReason == STOP_REASON_NONE ? STATE_QUEUED : STATE_STOPPED;
+        putDownloadWithState(download, state, download.stopReason);
        syncTasks();
      } else {
        int removeIndex = getDownloadIndex(download.request.id);
@@ -1204,12 +1206,11 @@ public final class DownloadManager {
      return !downloadsPaused && notMetRequirements == 0;
    }
-    private Download putDownloadWithState(Download download, @Download.State int state) {
+    private Download putDownloadWithState(
-      // Downloads in terminal states shouldn't be in the downloads list. This method cannot be used
+        Download download, @Download.State int state, int stopReason) {
-      // to set STATE_STOPPED either, because it doesn't have a stopReason argument.
+      // Downloads in terminal states shouldn't be in the downloads list.
-      Assertions.checkState(
+      Assertions.checkState(state != STATE_COMPLETED && state != STATE_FAILED);
-          state != STATE_COMPLETED && state != STATE_FAILED && state != STATE_STOPPED);
+      return putDownload(copyDownloadWithState(download, state, stopReason));
-      return putDownload(copyDownloadWithState(download, state));
    }
    private Download putDownload(Download download) {
@@ -1267,14 +1268,15 @@ public final class DownloadManager {
      return C.INDEX_UNSET;
    }
-    private static Download copyDownloadWithState(Download download, @Download.State int state) {
+    private static Download copyDownloadWithState(
+        Download download, @Download.State int state, int stopReason) {
      return new Download(
          download.request,
          state,
          download.startTimeMs,
          /* updateTimeMs= */ System.currentTimeMillis(),
          download.contentLength,
-          /* stopReason= */ 0,
+          stopReason,
          FAILURE_REASON_NONE,
          download.progress);
    }

--- a/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java
+++ b/library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java
@@ -583,6 +583,64 @@ public class DownloadManagerTest {
  }
  @Test
+  public void addDownload_whilstRemovingWithStopReason_addsStartedDownload() throws Throwable {
+    runOnMainThread(
+        () -> downloadManager.addDownload(createDownloadRequest(ID1), /* stopReason= */ 1234));
+    postRemoveRequest(ID1);
+    FakeDownloader downloadRemover = getDownloaderAt(0);
+    downloadRemover.assertRemoveStarted();
+    // Re-add the download without a stop reason.
+    postDownloadRequest(ID1);
+    downloadRemover.finish();
+    FakeDownloader downloader = getDownloaderAt(1);
+    downloader.finish();
+    assertDownloadIndexSize(1);
+    // We expect one downloader for the removal, and one for when the download was re-added.
+    assertDownloaderCount(2);
+    // The download has completed, and so is no longer current.
+    assertCurrentDownloadCount(0);
+    Download download = postGetDownloadIndex().getDownload(ID1);
+    assertThat(download.state).isEqualTo(Download.STATE_COMPLETED);
+    assertThat(download.stopReason).isEqualTo(0);
+  }
+  /** Test for https://github.com/google/ExoPlayer/issues/8419 */
+  @Test
+  public void addDownloadWithStopReason_whilstRemoving_addsStoppedDownload() throws Throwable {
+    postDownloadRequest(ID1);
+    getDownloaderAt(0).finish();
+    postRemoveRequest(ID1);
+    FakeDownloader downloadRemover = getDownloaderAt(1);
+    downloadRemover.assertRemoveStarted();
+    // Re-add the download with a stop reason.
+    runOnMainThread(
+        () -> downloadManager.addDownload(createDownloadRequest(ID1), /* stopReason= */ 1234));
+    downloadRemover.finish();
+    assertDownloadIndexSize(1);
+    // We expect one downloader for the initial download, and one for the removal. A downloader
+    // should not be created when the download is re-added, since a stop reason is specified.
+    assertDownloaderCount(2);
+    // The download isn't completed, and is therefore still current.
+    assertCurrentDownloadCount(1);
+    List<Download> downloads = postGetCurrentDownloads();
+    Download download = downloads.get(0);
+    assertThat(download.request.id).isEqualTo(ID1);
+    assertThat(download.state).isEqualTo(Download.STATE_STOPPED);
+    assertThat(download.stopReason).isEqualTo(1234);
+  }
+  @Test
  public void mergeRequest_removing_becomesRestarting() {
    DownloadRequest downloadRequest = createDownloadRequest(ID1);
    DownloadBuilder downloadBuilder =