From 87865a5cc442f353fce1979a0047f726a0b8ffd6 Mon Sep 17 00:00:00 2001 From: olly Date: Fri, 20 Dec 2019 15:41:01 +0000 Subject: [PATCH] DefaultDownloadIndex: Clear failure reason when removing download The Download constructor considers it invalid to have a failure reason if the download isn't in the failed state. Unfortunately, calling DefaultDownloadIndex.removeAllDownloads when there's a failed download will change the state without clearing the reason. If the downloads are then read back from the DefaultDownloadIndex we end up violating the Download constructor assertion. This change clears the failed reason for any existing rows in the invalid state, and also fixes the root cause that allows invalid rows to enter the table in the first place. Issue: #6785 PiperOrigin-RevId: 286576242 --- RELEASENOTES.md | 4 ++++ .../offline/DefaultDownloadIndex.java | 17 +++++++++++++++-- .../android/exoplayer2/offline/Download.java | 4 ++-- .../offline/DefaultDownloadIndexTest.java | 17 +++++++++++++++++ 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 074294ae0a..3f97174278 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -16,6 +16,10 @@ `SsaStyle$SsaAlignment` ([#6771](https://github.com/google/ExoPlayer/issues/6771)). * Fix `CacheDataSource` to correctly propagate `DataSpec.httpRequestHeaders`. +* Fix issue with `DefaultDownloadIndex` that could result in an + `IllegalStateException` being thrown from + `DefaultDownloadIndex.getDownloadForCurrentRow` + ([#6785](https://github.com/google/ExoPlayer/issues/6785)). * Add missing @Nullable to `MediaCodecAudioRenderer.getMediaClock` and `SimpleDecoderAudioRenderer.getMediaClock` ([#6792](https://github.com/google/ExoPlayer/issues/6792)). diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java index 7ed1eb095f..f1c897813f 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DefaultDownloadIndex.java @@ -26,6 +26,8 @@ import androidx.annotation.VisibleForTesting; import com.google.android.exoplayer2.database.DatabaseIOException; import com.google.android.exoplayer2.database.DatabaseProvider; import com.google.android.exoplayer2.database.VersionTable; +import com.google.android.exoplayer2.offline.Download.FailureReason; +import com.google.android.exoplayer2.offline.Download.State; import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.Util; import java.util.ArrayList; @@ -239,6 +241,9 @@ public final class DefaultDownloadIndex implements WritableDownloadIndex { try { ContentValues values = new ContentValues(); values.put(COLUMN_STATE, Download.STATE_REMOVING); + // Only downloads in STATE_FAILED are allowed a failure reason, so we need to clear it here in + // case we're moving downloads from STATE_FAILED to STATE_REMOVING. + values.put(COLUMN_FAILURE_REASON, Download.FAILURE_REASON_NONE); SQLiteDatabase writableDatabase = databaseProvider.getWritableDatabase(); writableDatabase.update(tableName, values, /* whereClause= */ null, /* whereArgs= */ null); } catch (SQLException e) { @@ -351,14 +356,22 @@ public final class DefaultDownloadIndex implements WritableDownloadIndex { DownloadProgress downloadProgress = new DownloadProgress(); downloadProgress.bytesDownloaded = cursor.getLong(COLUMN_INDEX_BYTES_DOWNLOADED); downloadProgress.percentDownloaded = cursor.getFloat(COLUMN_INDEX_PERCENT_DOWNLOADED); + @State int state = cursor.getInt(COLUMN_INDEX_STATE); + // It's possible the database contains failure reasons for non-failed downloads, which is + // invalid. Clear them here. See https://github.com/google/ExoPlayer/issues/6785. + @FailureReason + int failureReason = + state == Download.STATE_FAILED + ? cursor.getInt(COLUMN_INDEX_FAILURE_REASON) + : Download.FAILURE_REASON_NONE; return new Download( request, - /* state= */ cursor.getInt(COLUMN_INDEX_STATE), + state, /* startTimeMs= */ cursor.getLong(COLUMN_INDEX_START_TIME_MS), /* updateTimeMs= */ cursor.getLong(COLUMN_INDEX_UPDATE_TIME_MS), /* contentLength= */ cursor.getLong(COLUMN_INDEX_CONTENT_LENGTH), /* stopReason= */ cursor.getInt(COLUMN_INDEX_STOP_REASON), - /* failureReason= */ cursor.getInt(COLUMN_INDEX_FAILURE_REASON), + failureReason, downloadProgress); } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java index 97dff8394e..da46120b29 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java @@ -130,9 +130,9 @@ public final class Download { @FailureReason int failureReason, DownloadProgress progress) { Assertions.checkNotNull(progress); - Assertions.checkState((failureReason == FAILURE_REASON_NONE) == (state != STATE_FAILED)); + Assertions.checkArgument((failureReason == FAILURE_REASON_NONE) == (state != STATE_FAILED)); if (stopReason != 0) { - Assertions.checkState(state != STATE_DOWNLOADING && state != STATE_QUEUED); + Assertions.checkArgument(state != STATE_DOWNLOADING && state != STATE_QUEUED); } this.request = request; this.state = state; diff --git a/library/core/src/test/java/com/google/android/exoplayer2/offline/DefaultDownloadIndexTest.java b/library/core/src/test/java/com/google/android/exoplayer2/offline/DefaultDownloadIndexTest.java index f42a1c6086..d7664e21ca 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/offline/DefaultDownloadIndexTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/offline/DefaultDownloadIndexTest.java @@ -248,6 +248,23 @@ public class DefaultDownloadIndexTest { assertEqual(readDownload, download); } + @Test + public void setStatesToRemoving_setsStateAndClearsFailureReason() throws Exception { + String id = "id"; + DownloadBuilder downloadBuilder = + new DownloadBuilder(id) + .setState(Download.STATE_FAILED) + .setFailureReason(Download.FAILURE_REASON_UNKNOWN); + Download download = downloadBuilder.build(); + downloadIndex.putDownload(download); + + downloadIndex.setStatesToRemoving(); + + download = downloadIndex.getDownload(id); + assertThat(download.state).isEqualTo(Download.STATE_REMOVING); + assertThat(download.failureReason).isEqualTo(Download.FAILURE_REASON_NONE); + } + @Test public void setSingleDownloadStopReason_setReasonToNone() throws Exception { String id = "id";