diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt index e8d7fc02038..a883886a779 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt @@ -597,8 +597,8 @@ abstract class AbstractFetchDownloadService : Service() { } } - @Suppress("ComplexCondition") - internal fun performDownload(currentDownloadJobState: DownloadJobState) { + @Suppress("ComplexCondition", "ComplexMethod") + internal fun performDownload(currentDownloadJobState: DownloadJobState, useHttpClient: Boolean = false) { val download = currentDownloadJobState.state val isResumingDownload = currentDownloadJobState.currentBytesCopied > 0L val headers = MutableHeaders() @@ -616,7 +616,7 @@ abstract class AbstractFetchDownloadService : Service() { val request = Request(download.url.sanitizeURL(), headers = headers, cookiePolicy = cookiePolicy) // When resuming a download we need to use the httpClient as // download.response doesn't support adding headers. - val response = if (isResumingDownload) { + val response = if (isResumingDownload || useHttpClient) { httpClient.fetch(request) } else { download.response ?: httpClient.fetch(request) @@ -636,14 +636,17 @@ abstract class AbstractFetchDownloadService : Service() { } response.body.useStream { inStream -> + var copyInChuckStatus: CopyInChuckStatus? = null val newDownloadState = download.withResponse(response.headers, inStream) currentDownloadJobState.state = newDownloadState useFileStream(newDownloadState, isResumingDownload) { outStream -> - copyInChunks(currentDownloadJobState, inStream, outStream) + copyInChuckStatus = copyInChunks(currentDownloadJobState, inStream, outStream) } - verifyDownload(currentDownloadJobState) + if (copyInChuckStatus != CopyInChuckStatus.ERROR_IN_STREAM_CLOSED) { + verifyDownload(currentDownloadJobState) + } } } @@ -671,8 +674,14 @@ abstract class AbstractFetchDownloadService : Service() { } } + VisibleForTesting + internal enum class CopyInChuckStatus { + COMPLETED, ERROR_IN_STREAM_CLOSED + } + @VisibleForTesting - internal fun copyInChunks(downloadJobState: DownloadJobState, inStream: InputStream, outStream: OutputStream) { + @Suppress("MaxLineLength") + internal fun copyInChunks(downloadJobState: DownloadJobState, inStream: InputStream, outStream: OutputStream): CopyInChuckStatus { val data = ByteArray(CHUNK_SIZE) logger.debug("starting copyInChunks ${downloadJobState.state.id}" + " currentBytesCopied ${downloadJobState.state.currentBytesCopied}") @@ -685,10 +694,16 @@ abstract class AbstractFetchDownloadService : Service() { updateDownloadState(newState) } + var isInStreamClosed = false // To ensure that we copy all files (even ones that don't have fileSize, we must NOT check < fileSize while (getDownloadJobStatus(downloadJobState) == DOWNLOADING) { - val bytesRead = inStream.read(data) - + var bytesRead: Int + try { + bytesRead = inStream.read(data) + } catch (e: IOException) { + isInStreamClosed = true + break + } // If bytesRead is -1, there's no data left to read from the stream if (bytesRead == -1) { break } downloadJobState.currentBytesCopied += bytesRead @@ -697,10 +712,20 @@ abstract class AbstractFetchDownloadService : Service() { outStream.write(data, 0, bytesRead) } + if (isInStreamClosed) { + // In cases where [download.response] is available and users with slow + // networks start a download but quickly press pause and then resume + // [isResumingDownload] will be false as there will be not enough time + // for bytes to be copied, but the stream in [download.response] will be closed, + // we have to fallback to [httpClient] + performDownload(downloadJobState, useHttpClient = true) + return CopyInChuckStatus.ERROR_IN_STREAM_CLOSED + } logger.debug( "Finishing copyInChunks ${downloadJobState.state.id} " + "currentBytesCopied ${downloadJobState.currentBytesCopied}" ) + return CopyInChuckStatus.COMPLETED } /** diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt index f0d0e519aa2..7ff456d65ff 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt @@ -42,6 +42,7 @@ import mozilla.components.feature.downloads.AbstractFetchDownloadService.Compani import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_RESUME import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_TRY_AGAIN import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.PROGRESS_UPDATE_INTERVAL +import mozilla.components.feature.downloads.AbstractFetchDownloadService.CopyInChuckStatus.ERROR_IN_STREAM_CLOSED import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobState import mozilla.components.feature.downloads.DownloadNotification.NOTIFICATION_DOWNLOAD_GROUP_ID import mozilla.components.feature.downloads.facts.DownloadsFacts.Items.NOTIFICATION @@ -72,6 +73,7 @@ import org.mockito.ArgumentMatchers.anyLong import org.mockito.ArgumentMatchers.isNull import org.mockito.Mock import org.mockito.Mockito.atLeastOnce +import org.mockito.Mockito.doAnswer import org.mockito.Mockito.doCallRealMethod import org.mockito.Mockito.doNothing import org.mockito.Mockito.doReturn @@ -146,7 +148,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) assertEquals(download.url, providedDownload.value.state.url) assertEquals(download.fileName, providedDownload.value.state.fileName) @@ -333,7 +335,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) val pauseIntent = Intent(ACTION_PAUSE).apply { setPackage(testContext.applicationContext.packageName) @@ -372,7 +374,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) val cancelIntent = Intent(ACTION_CANCEL).apply { setPackage(testContext.applicationContext.packageName) @@ -419,7 +421,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) // Simulate a pause var downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -472,7 +474,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() // Simulate a failure @@ -523,7 +525,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -577,7 +579,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -597,7 +599,7 @@ class AbstractFetchDownloadServiceTest { val downloadIntent = Intent("ACTION_DOWNLOAD") downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) - doNothing().`when`(service).performDownload(any()) + doNothing().`when`(service).performDownload(any(), anyBoolean()) browserStore.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) @@ -628,7 +630,7 @@ class AbstractFetchDownloadServiceTest { val downloadIntent = Intent("ACTION_DOWNLOAD") downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) - doNothing().`when`(service).performDownload(any()) + doNothing().`when`(service).performDownload(any(), anyBoolean()) browserStore.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) @@ -708,7 +710,7 @@ class AbstractFetchDownloadServiceTest { val downloadIntent = Intent("ACTION_DOWNLOAD") downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) - doNothing().`when`(service).performDownload(any()) + doNothing().`when`(service).performDownload(any(), anyBoolean()) service.onStartCommand(downloadIntent, 0, 0) @@ -723,7 +725,7 @@ class AbstractFetchDownloadServiceTest { val downloadIntent = Intent("ACTION_DOWNLOAD") downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) - doNothing().`when`(service).performDownload(any()) + doNothing().`when`(service).performDownload(any(), anyBoolean()) browserStore.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) @@ -932,7 +934,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -964,7 +966,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -995,7 +997,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -1027,7 +1029,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -1053,7 +1055,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! assertEquals(FAILED, service.getDownloadJobStatus(downloadJobState)) @@ -1160,7 +1162,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { assertTrue(it.job!!.isActive) } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) // Advance the clock so that the puller posts a notification. testDispatcher.advanceTimeBy(PROGRESS_UPDATE_INTERVAL) @@ -1219,7 +1221,7 @@ class AbstractFetchDownloadServiceTest { service.downloadJobs.values.forEach { it.job?.join() } val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.setDownloadJobStatus(service.downloadJobs[download.id]!!, DownloadState.Status.PAUSED) @@ -1502,7 +1504,7 @@ class AbstractFetchDownloadServiceTest { val providedDownload = argumentCaptor() - verify(service).performDownload(providedDownload.capture()) + verify(service).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() val cancelledDownloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -1521,7 +1523,7 @@ class AbstractFetchDownloadServiceTest { browserStore.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } - verify(service, times(2)).performDownload(providedDownload.capture()) + verify(service, times(2)).performDownload(providedDownload.capture(), anyBoolean()) service.downloadJobs[providedDownload.value.state.id]?.job?.join() val downloadJobState = service.downloadJobs[providedDownload.value.state.id]!! @@ -1619,4 +1621,39 @@ class AbstractFetchDownloadServiceTest { assertEquals(15, downloadJobState.currentBytesCopied) } + + @Test + fun `copyInChunks - must return ERROR_IN_STREAM_CLOSED when inStream is closed`() = runBlocking { + val downloadJobState = DownloadJobState(state = mock(), status = DOWNLOADING) + val inputStream = mock() + + assertEquals(0, downloadJobState.currentBytesCopied) + + doAnswer { throw IOException() }.`when`(inputStream).read(any()) + doNothing().`when`(service).updateDownloadState(any()) + doNothing().`when`(service).performDownload(any(), anyBoolean()) + + val status = service.copyInChunks(downloadJobState, inputStream, mock()) + + verify(service).performDownload(downloadJobState, true) + assertEquals(ERROR_IN_STREAM_CLOSED, status) + } + + @Test + fun `copyInChunks - must return COMPLETED when finish copying bytes`() = runBlocking { + val downloadJobState = DownloadJobState(state = mock(), status = DOWNLOADING) + val inputStream = mock() + + assertEquals(0, downloadJobState.currentBytesCopied) + + doReturn(15, -1).`when`(inputStream).read(any()) + doNothing().`when`(service).updateDownloadState(any()) + + val status = service.copyInChunks(downloadJobState, inputStream, mock()) + + verify(service, never()).performDownload(any(), anyBoolean()) + + assertEquals(15, downloadJobState.currentBytesCopied) + assertEquals(AbstractFetchDownloadService.CopyInChuckStatus.COMPLETED, status) + } } diff --git a/docs/changelog.md b/docs/changelog.md index 5c6eaca55a0..f7fe4c59cec 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -15,6 +15,9 @@ permalink: /changelog/ * **browser-engine-gecko**, **browser-engine-gecko-beta**, **browser-engine-gecko-nightly** * 🚒 Bug fixed [issue #8464](https://github.com/mozilla-mobile/android-components/issues/8464) - Crash when confirming a prompt that was already confirmed +* **feature-downloads** + * 🚒 Bug fixed [issue #9033](https://github.com/mozilla-mobile/android-components/issues/9033) - Fix resuming downloads in slow networks more details see the [Fenix issue](https://github.com/mozilla-mobile/fenix/issues/9354#issuecomment-731267368). + * **feature-app-links** * Added handling of PackageItemInfo.packageName NullPointerException on some Xiaomi and TCL devices