diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 77d8ffc8c76..f7177f9fe76 100644 --- 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)). diff --git 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 index b6228025cf0..a989e2575f7 100644 --- 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 @@ private void setStopReason(@Nullable String id, int stopReason) { 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 @@ private void removeDownload(String id) { 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 @@ private void removeAllDownloads() { 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 @@ private Task syncQueuedDownload(@Nullable Task activeTask, Download download) { } // 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 @@ private void syncDownloadingDownload( 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 @@ private void onDownloadTaskStopped(Download download, @Nullable Exception finalE private void onRemoveTaskStopped(Download download) { if (download.state == STATE_RESTARTING) { - putDownloadWithState( - download, download.stopReason == STOP_REASON_NONE ? STATE_QUEUED : STATE_STOPPED); + @Download.State + 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 @@ private boolean canDownloadsRun() { return !downloadsPaused && notMetRequirements == 0; } - private Download putDownloadWithState(Download download, @Download.State int state) { - // Downloads in terminal states shouldn't be in the downloads list. This method cannot be used - // to set STATE_STOPPED either, because it doesn't have a stopReason argument. - Assertions.checkState( - state != STATE_COMPLETED && state != STATE_FAILED && state != STATE_STOPPED); - return putDownload(copyDownloadWithState(download, state)); + private Download putDownloadWithState( + Download download, @Download.State int state, int stopReason) { + // Downloads in terminal states shouldn't be in the downloads list. + Assertions.checkState(state != STATE_COMPLETED && state != STATE_FAILED); + return putDownload(copyDownloadWithState(download, state, stopReason)); } private Download putDownload(Download download) { @@ -1267,14 +1268,15 @@ private int getDownloadIndex(String id) { 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); } diff --git 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 index d5959584ad9..4dd80b6c7c3 100644 --- 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 @@ -582,6 +582,64 @@ public void getCurrentDownloads_returnsCurrentDownloads() throws Throwable { 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 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);