Fix DownloadManager assertion failure

Issue: #8419
#minor-release
PiperOrigin-RevId: 350134719

RELEASENOTES.md

@@ -67,12 +67,23 @@
 *   DRM:
     *   Fix playback failure when switching from PlayReady protected content to
         Widevine or Clearkey protected content in a playlist.
+*   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)).
 *   Analytics:
     *   Pass a `DecoderReuseEvaluation` to `AnalyticsListener`'s
         `onVideoInputFormatChanged` and `onAudioInputFormatChanged` methods. The
         `DecoderReuseEvaluation` indicates whether it was possible to re-use an
         existing decoder instance for the new format, and if not then the
         reasons why.
+*   Text:
+    *   Gracefully handle null-terminated subtitle content in Matroska
+        containers.
+    *   Fix CEA-708 anchor positioning
+        ([#1807](https://github.com/google/ExoPlayer/issues/1807)).
+*   Metadata retriever:
+    *   Parse Google Photos HEIC motion photos metadata.
 *   IMA extension:
     *   Add support for playback of ads in playlists
         ([#3750](https://github.com/google/ExoPlayer/issues/3750)).
@@ -83,16 +94,9 @@
         ([#8290](https://github.com/google/ExoPlayer/issues/8290)).
     *   Add `ImaAdsLoader.Builder.setEnableContinuousPlayback` for setting
         whether to request ads for continuous playback.
-*   Metadata retriever:
-    *   Parse Google Photos HEIC motion photos metadata.
 *   FFmpeg extension:
     *   Link the FFmpeg library statically, saving 350KB in binary size on
         average.
-*   Text:
-    *   Gracefully handle null-terminated subtitle content in Matroska
-        containers.
-    *   Fix CEA-708 anchor positioning
-        ([#1807](https://github.com/google/ExoPlayer/issues/1807)).
 *   OkHttp extension:
     *   Add `OkHttpDataSource.Factory` and deprecate `OkHttpDataSourceFactory`.
 *   Cronet extension:
@@ -225,7 +229,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)).

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);
     }

library/core/src/test/java/com/google/android/exoplayer2/offline/DownloadManagerTest.java

@@ -583,6 +583,64 @@ public class DownloadManagerTest {
     assertThat(download2.state).isEqualTo(Download.STATE_REMOVING);
   }
 
+  @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);