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 3, 2021 · 3261fc5 · 3261fc5
1 parent 56e37dd
commit 3261fc5
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 14 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
@@ -461,7 +461,7 @@ abstract class AbstractFetchDownloadService : Service() {
                     title = fileName,
                     description = fileName,
                     isMediaScannerScannable = true,
-                    mimeType = download.contentType ?: "*/*",
+                    mimeType = getNotEmptyMimeType(download.contentType),
                     path = file.absolutePath,
                     length = download.contentLength ?: file.length(),
                     // Only show notifications if our channel is blocked
@@ -885,7 +885,7 @@ 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, getNotEmptyMimeType(download.contentType))
             put(MediaStore.Downloads.SIZE, download.contentLength)
             put(MediaStore.Downloads.IS_PENDING, 1)
         }
@@ -994,13 +994,16 @@ abstract class AbstractFetchDownloadService : Service() {
             val resultContentType = if (!contentTypeFromFile.isNullOrEmpty()) {
                 contentTypeFromFile
             } else {
-                if (!contentType.isNullOrEmpty()) {
-                    contentType
-                } else {
-                    "*/*"
-                }
+                getNotEmptyMimeType(contentType)
             }
-            return (DownloadUtils.sanitizeMimeType(resultContentType) ?: "*/*")
+            return (getNotEmptyMimeType(DownloadUtils.sanitizeMimeType(resultContentType)))
+        }
+
+        @VisibleForTesting
+        internal fun getNotEmptyMimeType(mimeType: String?) = if (!mimeType.isNullOrEmpty()) {
+            mimeType
+        } else {
+            "*/*"
         }
 
         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/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`.