From cad04f523875cfec0fa4212f13cd6a977c76502f Mon Sep 17 00:00:00 2001 From: Katherine Blizard <414924+kabliz@users.noreply.github.com> Date: Tue, 17 Sep 2024 13:26:40 -0700 Subject: [PATCH 1/2] [APT-10372] Prevent Crashing If Clients Act Too Early The `ActionBeforeSetup` exception is being thrown from an area designed to handle errors, resulting in crashing. We catch these exceptions and update the error state instead. --- .../main/java/com/scribd/armadillo/Reducer.kt | 407 ++++++++++-------- .../java/com/scribd/armadillo/ReducerTest.kt | 11 +- RELEASE.md | 3 + gradle.properties | 2 +- 4 files changed, 228 insertions(+), 195 deletions(-) diff --git a/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt b/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt index 9234877..81569f4 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt @@ -66,248 +66,279 @@ internal object Reducer { fun reduce(oldState: ArmadilloState, action: Action): ArmadilloState { val newDebug = oldState.addNewAction(action) - return when (action) { - is PlaybackSpeedAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - oldState.copy( + try { + return when (action) { + is PlaybackSpeedAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + oldState.copy( playbackInfo = playbackInfo.copy( - playbackSpeed = action.playbackSpeed + playbackSpeed = action.playbackSpeed )) .apply { debugState = newDebug } - } - is PlayerStateAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - val controlState = if (action.playerState == PlaybackState.NONE) { - MediaControlState(isStopping = true) - } else playbackInfo.controlState.copy(isStartingNewAudioPlayable = false) - oldState.copy( + } + + is PlayerStateAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + val controlState = if (action.playerState == PlaybackState.NONE) { + MediaControlState(isStopping = true) + } else playbackInfo.controlState.copy(isStartingNewAudioPlayable = false) + oldState.copy( playbackInfo = playbackInfo.copy( - controlState = controlState, - playbackState = action.playerState + controlState = controlState, + playbackState = action.playerState )) .apply { debugState = newDebug } - } - is PlaybackProgressAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - oldState.copy(playbackInfo = playbackInfo.copy(progress = playbackInfo.progress.copy( - currentChapterIndex = playbackInfo.audioPlayable.getChapterIndexAtOffset(action.currentPosition), - positionInDuration = action.currentPosition, - totalPlayerDuration = action.playerDuration - ))) - .apply { debugState = newDebug } - } + } - is ErrorAction -> { - oldState.copy(error = action.error) + is PlaybackProgressAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + oldState.copy(playbackInfo = playbackInfo.copy(progress = playbackInfo.progress.copy( + currentChapterIndex = playbackInfo.audioPlayable.getChapterIndexAtOffset(action.currentPosition), + positionInDuration = action.currentPosition, + totalPlayerDuration = action.playerDuration + ))) .apply { debugState = newDebug } - } + } + + is ErrorAction -> { + oldState.copy(error = action.error) + .apply { debugState = newDebug } + } - is LoadingAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - oldState.copy(playbackInfo = playbackInfo.copy( + is LoadingAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + oldState.copy(playbackInfo = playbackInfo.copy( isLoading = action.isLoading)) .apply { debugState = newDebug } - } + } - is UpdateDownloadAction -> { - val newDownloadInfo = action.downloadProgressInfo - val oldDownloadInfo = oldState.downloadInfo.filterOutCompletedItems() - val updatedList = oldDownloadInfo.replaceDownloadProgressItemsByUrl(listOf(newDownloadInfo)) - oldState.copy(downloadInfo = updatedList) + is UpdateDownloadAction -> { + val newDownloadInfo = action.downloadProgressInfo + val oldDownloadInfo = oldState.downloadInfo.filterOutCompletedItems() + val updatedList = oldDownloadInfo.replaceDownloadProgressItemsByUrl(listOf(newDownloadInfo)) + oldState.copy(downloadInfo = updatedList) .apply { debugState = newDebug } - } + } - is StopTrackingDownloadAction -> { - val newDownloadInfo = action.downloadProgressInfo - val updatedList = oldState.downloadInfo.removeItemsByUrl(listOf(newDownloadInfo)) - oldState.copy(downloadInfo = updatedList) + is StopTrackingDownloadAction -> { + val newDownloadInfo = action.downloadProgressInfo + val updatedList = oldState.downloadInfo.removeItemsByUrl(listOf(newDownloadInfo)) + oldState.copy(downloadInfo = updatedList) .apply { debugState = newDebug } - } - is SkipDistanceAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - oldState.copy(playbackInfo = playbackInfo.copy( + } + + is SkipDistanceAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + oldState.copy(playbackInfo = playbackInfo.copy( skipDistance = action.skipDistance - )) + )) .apply { debugState = newDebug } - } - is PlaybackEngineReady -> { - val internalStateInfo = oldState.internalState - oldState.copy( + } + + is PlaybackEngineReady -> { + val internalStateInfo = oldState.internalState + oldState.copy( internalState = internalStateInfo.copy( - isPlaybackEngineReady = action.isReady + isPlaybackEngineReady = action.isReady )) .apply { debugState = newDebug } - } - is NewAudioPlayableAction -> { - maxDurationDiscrepancy = action.maxDurationDiscrepancy - oldState.copy(playbackInfo = PlaybackInfo( + } + + is NewAudioPlayableAction -> { + maxDurationDiscrepancy = action.maxDurationDiscrepancy + oldState.copy(playbackInfo = PlaybackInfo( audioPlayable = action.audioPlayable, playbackState = PlaybackState.NONE, progress = PlaybackProgress( - positionInDuration = action.initialOffset, - totalChaptersDuration = action.audioPlayable.duration), + positionInDuration = action.initialOffset, + totalChaptersDuration = action.audioPlayable.duration), playbackSpeed = Constants.DEFAULT_PLAYBACK_SPEED, controlState = MediaControlState(isStartingNewAudioPlayable = true), skipDistance = oldState.playbackInfo?.skipDistance ?: Constants.AUDIO_SKIP_DURATION, isLoading = true), - drmPlaybackState = DrmState.NoDRM) + drmPlaybackState = DrmState.NoDRM) .apply { debugState = newDebug } - } - is MediaRequestUpdateAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - if (playbackInfo.audioPlayable.request.url != action.mediaRequest.url) { - throw InvalidRequest("MediaRequestUpdate cannot be used to change playback URL") } - oldState.copy( - playbackInfo = playbackInfo.copy( - audioPlayable = playbackInfo.audioPlayable.copy( - request = action.mediaRequest + + is MediaRequestUpdateAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + if (playbackInfo.audioPlayable.request.url != action.mediaRequest.url) { + throw InvalidRequest("MediaRequestUpdate cannot be used to change playback URL") + } + oldState.copy( + playbackInfo = playbackInfo.copy( + audioPlayable = playbackInfo.audioPlayable.copy( + request = action.mediaRequest + ) ) + ).apply { debugState = newDebug } + } + + is MetadataUpdateAction -> { + oldState.copy( + playbackInfo = oldState.playbackInfo?.let { + it.copy( + progress = it.progress.copy(totalChaptersDuration = action.chapters.last().endTime), + audioPlayable = it.audioPlayable.copy(title = action.title, chapters = action.chapters) + ) + } ) - ).apply { debugState = newDebug } - } - is MetadataUpdateAction -> { - oldState.copy( - playbackInfo = oldState.playbackInfo?.let { - it.copy( - progress = it.progress.copy(totalChaptersDuration = action.chapters.last().endTime), - audioPlayable = it.audioPlayable.copy(title = action.title, chapters = action.chapters) - ) - } - ) - .apply { debugState = newDebug } - } - is SeekAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - //clear control inputs once loading ends, otherwise keep it - val controlState = if (action.isSeeking) { - MediaControlState( + .apply { debugState = newDebug } + } + + is SeekAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + //clear control inputs once loading ends, otherwise keep it + val controlState = if (action.isSeeking) { + MediaControlState( isSeeking = action.isSeeking, seekTarget = action.seekPositionTarget) - } else MediaControlState() + } else MediaControlState() - oldState.copy(playbackInfo = playbackInfo.copy( + oldState.copy(playbackInfo = playbackInfo.copy( controlState = controlState)) .apply { debugState = newDebug } - } - is FastForwardAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - oldState.copy(playbackInfo = playbackInfo.copy( + } + + is FastForwardAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + oldState.copy(playbackInfo = playbackInfo.copy( controlState = MediaControlState( - isSeeking = true, - isFastForwarding = true, - seekTarget = action.seekPositionTarget) - )).apply { debugState = newDebug } - } - is RewindAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - oldState.copy(playbackInfo = playbackInfo.copy( + isSeeking = true, + isFastForwarding = true, + seekTarget = action.seekPositionTarget) + )).apply { debugState = newDebug } + } + + is RewindAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + oldState.copy(playbackInfo = playbackInfo.copy( controlState = MediaControlState( - isSeeking = true, - isRewinding = true, - seekTarget = action.seekPositionTarget) - )).apply { debugState = newDebug } - } - is SkipNextAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - oldState.copy(playbackInfo = playbackInfo.copy( + isSeeking = true, + isRewinding = true, + seekTarget = action.seekPositionTarget) + )).apply { debugState = newDebug } + } + + is SkipNextAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + oldState.copy(playbackInfo = playbackInfo.copy( controlState = MediaControlState( - isSeeking = true, - isNextChapter = true, - seekTarget = action.seekPositionTarget) - )).apply { debugState = newDebug } - } - is SkipPrevAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - oldState.copy(playbackInfo = playbackInfo.copy( + isSeeking = true, + isNextChapter = true, + seekTarget = action.seekPositionTarget) + )).apply { debugState = newDebug } + } + + is SkipPrevAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + oldState.copy(playbackInfo = playbackInfo.copy( controlState = MediaControlState( - isSeeking = true, - isPrevChapter = true, - seekTarget = action.seekPositionTarget) - )).apply { debugState = newDebug } - } - is CustomMediaSessionAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - val newControlState = playbackInfo.controlState.copy( + isSeeking = true, + isPrevChapter = true, + seekTarget = action.seekPositionTarget) + )).apply { debugState = newDebug } + } + + is CustomMediaSessionAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + val newControlState = playbackInfo.controlState.copy( customMediaActionName = action.actionName) - oldState.copy(playbackInfo = playbackInfo.copy( + oldState.copy(playbackInfo = playbackInfo.copy( controlState = newControlState)) .apply { debugState = newDebug } - } - is UpdateProgressAction -> { - val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) - val chapterIndex = if (action.currentChapterIndex >= 0) { - action.currentChapterIndex - } else { - playbackInfo.controlState.updatedChapterIndex } - val newControlState = playbackInfo.controlState.copy( + + is UpdateProgressAction -> { + val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action) + val chapterIndex = if (action.currentChapterIndex >= 0) { + action.currentChapterIndex + } else { + playbackInfo.controlState.updatedChapterIndex + } + val newControlState = playbackInfo.controlState.copy( isPlaybackStateUpdating = action.isUpdated, updatedChapterIndex = chapterIndex) - /** - * We dispatch an error when a progress update is received that is beyond the bounds of the audioPlayable. When this happens, - * chapter metadata indicates that the content is shorter then it is. - */ - val error = verifyProgressIsNotBeyondDuration(playbackInfo.progress) - oldState.copy( + + /** + * We dispatch an error when a progress update is received that is beyond the bounds of the audioPlayable. When this happens, + * chapter metadata indicates that the content is shorter then it is. + */ + val error = verifyProgressIsNotBeyondDuration(playbackInfo.progress) + oldState.copy( playbackInfo = playbackInfo.copy(controlState = newControlState), error = error).apply { debugState = newDebug } - } - is ClearErrorAction -> { - oldState.copy(error = null) + } + + is ClearErrorAction -> { + oldState.copy(error = null) .apply { debugState = newDebug } - } - /** - * The playback engine will dispatch this action when it reaches the end of the content. If the metadata indicates that there - * is more content to be played, we should alert the client of an error. - */ - is ContentEndedAction -> { - val playbackProgress = oldState.playbackInfo?.progress ?: throw ActionBeforeSetup(action) - val error = verifyAudiobookIsComplete(playbackProgress) - return oldState + } + /** + * The playback engine will dispatch this action when it reaches the end of the content. If the metadata indicates that there + * is more content to be played, we should alert the client of an error. + */ + is ContentEndedAction -> { + val playbackProgress = oldState.playbackInfo?.progress ?: throw ActionBeforeSetup(action) + val error = verifyAudiobookIsComplete(playbackProgress) + return oldState .copy(error = error, playbackInfo = oldState.playbackInfo.copy( controlState = MediaControlState(hasContentEnded = true) - )) + )) .apply { debugState = newDebug } - } - is OpeningLicenseAction -> { - val oldDrm = oldState.drmPlaybackState - return oldState - .copy(drmPlaybackState = DrmState.LicenseOpening(action.type ?: oldDrm.drmType)) - } - is LicenseAcquiredAction -> { - val oldDrm = oldState.drmPlaybackState - return oldState - .copy(drmPlaybackState = DrmState.LicenseAcquired(action.type, oldDrm.expireMillis)) - } - is LicenseExpiredAction -> { - val oldDrm = oldState.drmPlaybackState - return oldState - .copy(drmPlaybackState = DrmState.LicenseExpired(oldDrm.drmType, oldDrm.expireMillis)) - } - is LicenseExpirationDetermined -> { - val oldDrm = oldState.drmPlaybackState - return oldState - .copy(drmPlaybackState = DrmState.LicenseUsable(oldDrm.drmType, action.expirationMilliseconds)) - } - is LicenseReleasedAction -> { - val oldDrm = oldState.drmPlaybackState - return oldState - .copy(drmPlaybackState = DrmState.LicenseReleased(oldDrm.drmType,oldDrm.expireMillis, oldDrm.isSessionValid)) - } - is LicenseKeyIsUsableAction -> { - val oldDrm = oldState.drmPlaybackState - return oldState - .copy(drmPlaybackState = DrmState.LicenseUsable(oldDrm.drmType, oldDrm.expireMillis)) - } - is LicenseDrmErrorAction -> { - val oldDrm = oldState.drmPlaybackState - return oldState - .copy(drmPlaybackState = DrmState.LicenseError(oldDrm.drmType, oldDrm.expireMillis)) - } + } + + is OpeningLicenseAction -> { + val oldDrm = oldState.drmPlaybackState + return oldState + .copy(drmPlaybackState = DrmState.LicenseOpening(action.type ?: oldDrm.drmType)) + } + + is LicenseAcquiredAction -> { + val oldDrm = oldState.drmPlaybackState + return oldState + .copy(drmPlaybackState = DrmState.LicenseAcquired(action.type, oldDrm.expireMillis)) + } + + is LicenseExpiredAction -> { + val oldDrm = oldState.drmPlaybackState + return oldState + .copy(drmPlaybackState = DrmState.LicenseExpired(oldDrm.drmType, oldDrm.expireMillis)) + } + + is LicenseExpirationDetermined -> { + val oldDrm = oldState.drmPlaybackState + return oldState + .copy(drmPlaybackState = DrmState.LicenseUsable(oldDrm.drmType, action.expirationMilliseconds)) + } + + is LicenseReleasedAction -> { + val oldDrm = oldState.drmPlaybackState + return oldState + .copy(drmPlaybackState = DrmState.LicenseReleased(oldDrm.drmType, oldDrm.expireMillis, oldDrm.isSessionValid)) + } + + is LicenseKeyIsUsableAction -> { + val oldDrm = oldState.drmPlaybackState + return oldState + .copy(drmPlaybackState = DrmState.LicenseUsable(oldDrm.drmType, oldDrm.expireMillis)) + } + + is LicenseDrmErrorAction -> { + val oldDrm = oldState.drmPlaybackState + return oldState + .copy(drmPlaybackState = DrmState.LicenseError(oldDrm.drmType, oldDrm.expireMillis)) + } - else -> throw UnrecognizedAction(action) + else -> throw UnrecognizedAction(action) + } + } catch (ex: ActionBeforeSetup) { + return oldState.copy(error = ex) + .apply { debugState = newDebug } + } catch (ex: InvalidRequest) { + return oldState.copy(error = ex) + .apply { debugState = newDebug } } } diff --git a/Armadillo/src/test/java/com/scribd/armadillo/ReducerTest.kt b/Armadillo/src/test/java/com/scribd/armadillo/ReducerTest.kt index fd8cc57..2330032 100644 --- a/Armadillo/src/test/java/com/scribd/armadillo/ReducerTest.kt +++ b/Armadillo/src/test/java/com/scribd/armadillo/ReducerTest.kt @@ -386,9 +386,9 @@ class ReducerTest { "https://www.github.com/scribd/armadillo" )) - assertThrows(ActionBeforeSetup::class.java) { - Reducer.reduce(oldState, action) - } + val state = Reducer.reduce(oldState, action) + assertThat(state.error).isInstanceOfAny(ActionBeforeSetup::class.java) + } @Test @@ -398,9 +398,8 @@ class ReducerTest { "https://www.github.com/scribd/armadillo" )) - assertThrows("MediaRequestUpdate cannot be used to change playback URL", InvalidRequest::class.java) { - Reducer.reduce(oldState, action) - } + val state = Reducer.reduce(oldState, action) + assertThat(state.error).isInstanceOfAny(InvalidRequest::class.java) } @Test fun reduce_mediaRequestUpdateAction_updatesRequest() { diff --git a/RELEASE.md b/RELEASE.md index 1d5f4ee..0d1f930 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -1,5 +1,8 @@ # Project Armadillo Release Notes +## 1.6.2 +- Prevents fatal crashing for actions are being performed before the player is initialized + ## 1.6.1 - Prevents fatal crashing when content fails to load diff --git a/gradle.properties b/gradle.properties index 7c6b276..cd06b72 100644 --- a/gradle.properties +++ b/gradle.properties @@ -13,7 +13,7 @@ org.gradle.jvmargs=-Xmx1536m # org.gradle.parallel=true PACKAGE_NAME=com.scribd.armadillo GRADLE_PLUGIN_VERSION=7.2.0 -LIBRARY_VERSION=1.6.1 +LIBRARY_VERSION=1.6.2 EXOPLAYER_VERSION=2.19.1 RXJAVA_VERSION=2.2.4 RXANDROID_VERSION=2.0.1 From beb79fb0b324ab6c6f48ef46b8e8059ef6a9e160 Mon Sep 17 00:00:00 2001 From: Katherine Blizard <414924+kabliz@users.noreply.github.com> Date: Tue, 17 Sep 2024 14:09:44 -0700 Subject: [PATCH 2/2] [APT-10372] Prevent Crashing If Clients Act Too Early The `ActionBeforeSetup` exception is being thrown from an area designed to handle errors, resulting in crashing. We catch these exceptions and update the error state instead. --- Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt b/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt index 81569f4..f302bf6 100644 --- a/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt +++ b/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt @@ -339,6 +339,9 @@ internal object Reducer { } catch (ex: InvalidRequest) { return oldState.copy(error = ex) .apply { debugState = newDebug } + } catch (ex: UnrecognizedAction) { + return oldState.copy(error = ex) + .apply { debugState = newDebug } } }