Closes mozilla-mobile#9821 - DownloadService will use a default mime …

…type if otherwise empty Speculative fix (cannot reproduce the issue) for crashes where based on the stacktrace the download's mime type was empty.
Mugurell · Mar 4, 2021 · 022af9c · 022af9c
1 parent 56e37dd
commit 022af9c
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 20 deletions.
diff --git a/...nloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt b/...nloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt
@@ -70,6 +70,7 @@ import mozilla.components.feature.downloads.facts.emitNotificationPauseFact
 import mozilla.components.feature.downloads.facts.emitNotificationResumeFact
 import mozilla.components.feature.downloads.facts.emitNotificationTryAgainFact
 import mozilla.components.support.base.log.logger.Logger
+import mozilla.components.support.ktx.kotlin.ifNullOrEmpty
 import mozilla.components.support.ktx.kotlin.sanitizeURL
 import mozilla.components.support.ktx.kotlinx.coroutines.throttleLatest
 import mozilla.components.support.utils.DownloadUtils
@@ -461,7 +462,7 @@ abstract class AbstractFetchDownloadService : Service() {
                     title = fileName,
                     description = fileName,
                     isMediaScannerScannable = true,
-                    mimeType = download.contentType ?: "*/*",
+                    mimeType = getSafeContentType(context, download.filePath, download.contentType),
                     path = file.absolutePath,
                     length = download.contentLength ?: file.length(),
                     // Only show notifications if our channel is blocked
@@ -885,7 +886,9 @@ abstract class AbstractFetchDownloadService : Service() {
     internal fun useFileStreamScopedStorage(download: DownloadState, block: (OutputStream) -> Unit) {
         val values = ContentValues().apply {
             put(MediaStore.Downloads.DISPLAY_NAME, download.fileName)
-            put(MediaStore.Downloads.MIME_TYPE, download.contentType ?: "*/*")
+            put(MediaStore.Downloads.MIME_TYPE,
+                getSafeContentType(context, download.filePath, download.contentType)
+            )
             put(MediaStore.Downloads.SIZE, download.contentLength)
             put(MediaStore.Downloads.IS_PENDING, 1)
         }
@@ -968,12 +971,7 @@ abstract class AbstractFetchDownloadService : Service() {
         fun openFile(context: Context, filePath: String, contentType: String?): Boolean {
             // Create a new file with the location of the saved file to extract the correct path
             // `file` has the wrong path, so we must construct it based on the `fileName` and `dir.path`s
-            val fileLocation = File(filePath)
-            val constructedFilePath = FileProvider.getUriForFile(
-                context,
-                context.packageName + FILE_PROVIDER_EXTENSION,
-                fileLocation
-            )
+            val constructedFilePath = getFilePathUri(context, filePath)
 
             val newIntent = Intent(ACTION_VIEW).apply {
                 setDataAndType(constructedFilePath, getSafeContentType(context, constructedFilePath, contentType))
@@ -994,13 +992,23 @@ abstract class AbstractFetchDownloadService : Service() {
             val resultContentType = if (!contentTypeFromFile.isNullOrEmpty()) {
                 contentTypeFromFile
             } else {
-                if (!contentType.isNullOrEmpty()) {
-                    contentType
-                } else {
-                    "*/*"
-                }
+                contentType.ifNullOrEmpty { "*/*" }
             }
-            return (DownloadUtils.sanitizeMimeType(resultContentType) ?: "*/*")
+            return DownloadUtils.sanitizeMimeType(resultContentType).ifNullOrEmpty { "*/*" }
+        }
+
+        @VisibleForTesting
+        internal fun getSafeContentType(context: Context, filePath: String, contentType: String?): String {
+            return getSafeContentType(context, getFilePathUri(context, filePath), contentType)
+        }
+
+        @VisibleForTesting
+        internal fun getFilePathUri(context: Context, filePath: String): Uri {
+            return FileProvider.getUriForFile(
+                context,
+                context.packageName + FILE_PROVIDER_EXTENSION,
+                File(filePath)
+            )
         }
 
         private const val FILE_PROVIDER_EXTENSION = ".feature.downloads.fileprovider"

diff --git a/...ds/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt b/...ds/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt
@@ -18,22 +18,22 @@ import androidx.core.app.NotificationManagerCompat
 import androidx.core.content.getSystemService
 import androidx.localbroadcastmanager.content.LocalBroadcastManager
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.delay
-import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.Dispatchers.IO
-import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.test.TestCoroutineDispatcher
 import kotlinx.coroutines.test.resetMain
 import kotlinx.coroutines.test.runBlockingTest
 import kotlinx.coroutines.test.setMain
 import mozilla.components.browser.state.action.DownloadAction
 import mozilla.components.browser.state.state.content.DownloadState
+import mozilla.components.browser.state.state.content.DownloadState.Status.COMPLETED
 import mozilla.components.browser.state.state.content.DownloadState.Status.DOWNLOADING
 import mozilla.components.browser.state.state.content.DownloadState.Status.FAILED
 import mozilla.components.browser.state.state.content.DownloadState.Status.INITIATED
-import mozilla.components.browser.state.state.content.DownloadState.Status.COMPLETED
 import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.concept.fetch.Client
 import mozilla.components.concept.fetch.MutableHeaders
@@ -63,16 +63,17 @@ import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertNotEquals
 import org.junit.Assert.assertNotNull
-import org.junit.Assert.assertTrue
 import org.junit.Assert.assertNull
+import org.junit.Assert.assertSame
+import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
 import org.junit.rules.TemporaryFolder
 import org.junit.runner.RunWith
 import org.mockito.ArgumentMatchers.anyBoolean
-import org.mockito.ArgumentMatchers.anyString
 import org.mockito.ArgumentMatchers.anyLong
+import org.mockito.ArgumentMatchers.anyString
 import org.mockito.ArgumentMatchers.isNull
 import org.mockito.Mock
 import org.mockito.Mockito.atLeastOnce
@@ -1565,6 +1566,42 @@ class AbstractFetchDownloadServiceTest {
         verify(downloadManager).addCompletedDownload(anyString(), anyString(), anyBoolean(), anyString(), anyString(), anyLong(), anyBoolean(), any(), any())
     }
 
+    @Test
+    @Config(sdk = [Build.VERSION_CODES.P])
+    @Suppress("Deprecation")
+    fun `always call addCompletedDownload with a not empty or null mimeType`() = runBlockingTest {
+        val service = spy(object : AbstractFetchDownloadService() {
+            override val httpClient = client
+            override val store = browserStore
+        })
+        val spyContext = spy(testContext)
+        var downloadManager: DownloadManager = mock()
+        doReturn(spyContext).`when`(service).context
+        doReturn(downloadManager).`when`(spyContext).getSystemService<DownloadManager>()
+        val downloadWithNullMimeType = DownloadState(
+            url = "blob:moz-extension://d5ea9baa-64c9-4c3d-bb38-49308c47997c/",
+            fileName = "example.apk",
+            destinationDirectory = folder.root.path,
+            contentType = null
+        )
+        val downloadWithEmptyMimeType = downloadWithNullMimeType.copy(contentType = "")
+        val defaultMimeType = "*/*"
+
+        service.addToDownloadSystemDatabaseCompat(downloadWithNullMimeType, this)
+        verify(downloadManager).addCompletedDownload(
+            anyString(), anyString(), anyBoolean(), eq(defaultMimeType),
+            anyString(), anyLong(), anyBoolean(), isNull(), any()
+        )
+
+        downloadManager = mock()
+        doReturn(downloadManager).`when`(spyContext).getSystemService<DownloadManager>()
+        service.addToDownloadSystemDatabaseCompat(downloadWithEmptyMimeType, this)
+        verify(downloadManager).addCompletedDownload(
+            anyString(), anyString(), anyBoolean(), eq(defaultMimeType),
+            anyString(), anyLong(), anyBoolean(), isNull(), any()
+        )
+    }
+
     @Test
     fun `cancelled download does not prevent other notifications`() = runBlocking {
         val cancelledDownload = DownloadState("https://example.com/file.txt", "file.txt")
@@ -1800,4 +1837,21 @@ class AbstractFetchDownloadServiceTest {
 
         assertEquals("*/*", result)
     }
+
+    @Test
+    fun `getNotEmptyMimeType - WHEN the current mimeType is not empty THEN return without any modification`() {
+        val validMimeType = "application/pdf"
+
+        assertSame(validMimeType, AbstractFetchDownloadService.getNotEmptyMimeType(validMimeType))
+    }
+
+    @Test
+    fun `getNotEmptyMimeType - WHEN the current mimeType is empty THEN return a default mimeType`() {
+        assertEquals("*/*", AbstractFetchDownloadService.getNotEmptyMimeType(""))
+    }
+
+    @Test
+    fun `getNotEmptyMimeType - WHEN the current mimeType is null THEN return a default mimeType`() {
+        assertEquals("*/*", AbstractFetchDownloadService.getNotEmptyMimeType(null))
+    }
 }
diff --git a/components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt b/components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt
@@ -191,3 +191,10 @@ fun String.getDataUrlImageExtension(defaultExtension: String = "jpg"): String {
     return ("data:image\\/([a-zA-Z0-9-.+]+).*").toRegex()
         .find(this)?.groups?.get(1)?.value ?: defaultExtension
 }
+
+/**
+ * Returns this char sequence if it's not null or empty
+ * or the result of calling [defaultValue] function if the char sequence is null or empty.
+ */
+inline fun <C, R> C?.ifNullOrEmpty(defaultValue: () -> R): C where C : CharSequence, R : C =
+    if (isNullOrEmpty()) defaultValue() else this
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -12,6 +12,9 @@ permalink: /changelog/
 * [Gecko](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Gecko.kt)
 * [Configuration](https://github.com/mozilla-mobile/android-components/blob/master/.config.yml)
 
+* **feature-downloads**:
+  * 🚒 Bug fixed [issue #9821](https://github.com/mozilla-mobile/android-components/issues/9821) - Crash for downloads inferred empty mime types.
+
 * **browser-toolbar**
   * 🌟️ Added `ToolbarBehaviorController` to automatically block the `BrowserToolbar` being animated by the `BrowserToolbarBehavior` while the tab is loading. This new class just has to be initialized by AC clients, similar to `ToolbarPresenter`.