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..69b96ad21a6 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() @@ -613,13 +613,15 @@ abstract class AbstractFetchDownloadService : Service() { Request.CookiePolicy.INCLUDE } + var isUsingHttpClient = false 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 || download.response == null) { + isUsingHttpClient = true httpClient.fetch(request) } else { - download.response ?: httpClient.fetch(request) + requireNotNull(download.response) } logger.debug("Fetching download for ${currentDownloadJobState.state.id} ") @@ -636,14 +638,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, isUsingHttpClient) } - verifyDownload(currentDownloadJobState) + if (copyInChuckStatus != CopyInChuckStatus.ERROR_IN_STREAM_CLOSED) { + verifyDownload(currentDownloadJobState) + } } } @@ -672,7 +677,18 @@ abstract class AbstractFetchDownloadService : Service() { } @VisibleForTesting - internal fun copyInChunks(downloadJobState: DownloadJobState, inStream: InputStream, outStream: OutputStream) { + internal enum class CopyInChuckStatus { + COMPLETED, ERROR_IN_STREAM_CLOSED + } + + @VisibleForTesting + @Suppress("MaxLineLength") + internal fun copyInChunks( + downloadJobState: DownloadJobState, + inStream: InputStream, + outStream: OutputStream, + downloadWithHttpClient: Boolean = false + ): CopyInChuckStatus { val data = ByteArray(CHUNK_SIZE) logger.debug("starting copyInChunks ${downloadJobState.state.id}" + " currentBytesCopied ${downloadJobState.state.currentBytesCopied}") @@ -685,10 +701,19 @@ 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) { + if (downloadWithHttpClient) { + throw e + } + 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 +722,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..2df6dcf0b42 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,61 @@ 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 throw when inStream is closed and download was performed using http client`() = runBlocking { + val downloadJobState = DownloadJobState(state = mock(), status = DOWNLOADING) + val inputStream = mock() + var exceptionWasThrown = false + + assertEquals(0, downloadJobState.currentBytesCopied) + + doAnswer { throw IOException() }.`when`(inputStream).read(any()) + doNothing().`when`(service).updateDownloadState(any()) + doNothing().`when`(service).performDownload(any(), anyBoolean()) + + try { + service.copyInChunks(downloadJobState, inputStream, mock(), true) + } catch (e: IOException) { + exceptionWasThrown = true + } + + verify(service, times(0)).performDownload(downloadJobState, true) + assertTrue(exceptionWasThrown) + } + + @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