From 6d2d2e31ef09de3d9dc68e9f65c1d0eb05949ad1 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 5 Jan 2021 15:18:28 +0000 Subject: [PATCH] Fix DownloadManager assertion failure Issue: #8419 #minor-release PiperOrigin-RevId: 350134719 --- RELEASENOTES.md | 20 ++++--- .../exoplayer2/offline/DownloadManager.java | 34 ++++++----- .../offline/DownloadManagerTest.java | 58 +++++++++++++++++++ 3 files changed, 88 insertions(+), 24 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 58bfec7c717..bae800caf08 100644 --- a/RELEASENOTES.md +++ b/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)). 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 1444b0484c9..fc050a068ab 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 @@ -583,6 +583,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);