Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Commit

Permalink
Closes #9821 - DownloadService will use a default mime type if otherw…
Browse files Browse the repository at this point in the history 
…ise empty (#9822)

Speculative fix (cannot reproduce the issue) for crashes where based on the
stacktrace the download's mime type was empty.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
@Mugurell @mergify
Mugurell and mergify[bot] committed Mar 10, 2021
1 parent 7e3b610 commit dcc78fd
Show file tree
Hide file tree
Showing 5 changed files with 204 additions and 27 deletions.
36 changes: 22 additions & 14 deletions ...nloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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))
Expand All @@ -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"
Expand Down
161 changes: 148 additions & 13 deletions ...ds/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt
View file
Expand Up @@ -12,28 +12,31 @@ import android.app.Service
import android.content.ContentResolver
import android.content.Context
import android.content.Intent
import android.net.Uri
import android.os.Build
import android.os.Environment
import androidx.core.app.NotificationManagerCompat
import androidx.core.content.FileProvider
import androidx.core.content.getSystemService
import androidx.core.net.toUri
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
Expand Down Expand Up @@ -63,16 +66,16 @@ 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.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
Expand All @@ -89,10 +92,13 @@ import org.mockito.Mockito.verifyZeroInteractions
import org.mockito.MockitoAnnotations.initMocks
import org.robolectric.Shadows.shadowOf
import org.robolectric.annotation.Config
import org.robolectric.annotation.Implementation
import org.robolectric.annotation.Implements
import org.robolectric.shadows.ShadowNotificationManager
import java.io.File
import java.io.IOException
import java.io.InputStream
import java.lang.IllegalArgumentException
import kotlin.random.Random

@RunWith(AndroidJUnit4::class)
Expand Down Expand Up @@ -1405,7 +1411,7 @@ class AbstractFetchDownloadServiceTest {
}

@Test
@Config(sdk = [Build.VERSION_CODES.P])
@Config(sdk = [Build.VERSION_CODES.P], shadows = [ShadowFileProvider::class])
fun `WHEN a download is completed and the scoped storage is not used it MUST be added manually to the download system database`() = runBlockingTest {
val download = DownloadState(
url = "http://www.mozilla.org",
Expand Down Expand Up @@ -1516,7 +1522,7 @@ class AbstractFetchDownloadServiceTest {
}

@Test
@Config(sdk = [Build.VERSION_CODES.P])
@Config(sdk = [Build.VERSION_CODES.P], shadows = [ShadowFileProvider::class])
@Suppress("Deprecation")
fun `do not pass non-http(s) url to addCompletedDownload`() = runBlockingTest {
val download = DownloadState(
Expand All @@ -1541,7 +1547,7 @@ class AbstractFetchDownloadServiceTest {
}

@Test
@Config(sdk = [Build.VERSION_CODES.P])
@Config(sdk = [Build.VERSION_CODES.P], shadows = [ShadowFileProvider::class])
@Suppress("Deprecation")
fun `pass http(s) url to addCompletedDownload`() = runBlockingTest {
val download = DownloadState(
Expand All @@ -1565,6 +1571,42 @@ class AbstractFetchDownloadServiceTest {
verify(downloadManager).addCompletedDownload(anyString(), anyString(), anyBoolean(), anyString(), anyString(), anyLong(), anyBoolean(), any(), any())
}

@Test
@Config(sdk = [Build.VERSION_CODES.P], shadows = [ShadowFileProvider::class])
@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")
Expand Down Expand Up @@ -1769,7 +1811,7 @@ class AbstractFetchDownloadServiceTest {
doReturn(contentTypeFromFile).`when`(contentResolver).getType(any())
doReturn(contentResolver).`when`(spyContext).contentResolver

val result = AbstractFetchDownloadService.getSafeContentType(spyContext, mock(), "any")
val result = AbstractFetchDownloadService.getSafeContentType(spyContext, mock<Uri>(), "any")

assertEquals("application/pdf", result)
}
Expand All @@ -1779,25 +1821,118 @@ class AbstractFetchDownloadServiceTest {
val contentType = " application/pdf "
val spyContext = spy(testContext)
val contentResolver = mock<ContentResolver>()
doReturn(contentResolver).`when`(spyContext).contentResolver

doReturn(null).`when`(contentResolver).getType(any())
var result = AbstractFetchDownloadService.getSafeContentType(spyContext, mock<Uri>(), contentType)
assertEquals("application/pdf", result)

doReturn("").`when`(contentResolver).getType(any())
result = AbstractFetchDownloadService.getSafeContentType(spyContext, mock<Uri>(), contentType)
assertEquals("application/pdf", result)
}

@Test
fun `getSafeContentType - WHEN none of the provided content types are available THEN return a generic content type`() {
val spyContext = spy(testContext)
val contentResolver = mock<ContentResolver>()
doReturn(contentResolver).`when`(spyContext).contentResolver

doReturn(null).`when`(contentResolver).getType(any())
var result = AbstractFetchDownloadService.getSafeContentType(spyContext, mock<Uri>(), null)
assertEquals("*/*", result)

doReturn("").`when`(contentResolver).getType(any())
result = AbstractFetchDownloadService.getSafeContentType(spyContext, mock<Uri>(), null)
assertEquals("*/*", result)
}

// Following 3 tests use the String version of #getSafeContentType while the above 3 tested the Uri version
// The String version just overloads and delegates the Uri one but being in a companion object we cannot
// verify the delegation so we are left to verify the result to prevent any regressions.
@Test
@Config(shadows = [ShadowFileProvider::class])
fun `getSafeContentType2 - WHEN the file content type is available THEN use it`() {
val contentTypeFromFile = "application/pdf; qs=0.001"
val spyContext = spy(testContext)
val contentResolver = mock<ContentResolver>()

doReturn(contentTypeFromFile).`when`(contentResolver).getType(any())
doReturn(contentResolver).`when`(spyContext).contentResolver

val result = AbstractFetchDownloadService.getSafeContentType(spyContext, mock(), contentType)
val result = AbstractFetchDownloadService.getSafeContentType(spyContext, "any", "any")

assertEquals("application/pdf", result)
}

@Test
fun `getSafeContentType - WHEN none of the provided content types are available THEN return a generic content type`() {
@Config(shadows = [ShadowFileProvider::class])
fun `getSafeContentType2 - WHEN the file content type is not available THEN use the provided content type`() {
val contentType = " application/pdf "
val spyContext = spy(testContext)
val contentResolver = mock<ContentResolver>()
doReturn(contentResolver).`when`(spyContext).contentResolver

doReturn(null).`when`(contentResolver).getType(any())
var result = AbstractFetchDownloadService.getSafeContentType(spyContext, "any", contentType)
assertEquals("application/pdf", result)

doReturn("").`when`(contentResolver).getType(any())
result = AbstractFetchDownloadService.getSafeContentType(spyContext, "any", contentType)
assertEquals("application/pdf", result)
}

@Test
@Config(shadows = [ShadowFileProvider::class])
fun `getSafeContentType2 - WHEN none of the provided content types are available THEN return a generic content type`() {
val spyContext = spy(testContext)
val contentResolver = mock<ContentResolver>()
doReturn(contentResolver).`when`(spyContext).contentResolver

val result = AbstractFetchDownloadService.getSafeContentType(spyContext, mock(), null)
doReturn(null).`when`(contentResolver).getType(any())
var result = AbstractFetchDownloadService.getSafeContentType(spyContext, "any", null)
assertEquals("*/*", result)

doReturn("").`when`(contentResolver).getType(any())
result = AbstractFetchDownloadService.getSafeContentType(spyContext, "any", null)
assertEquals("*/*", result)
}

// Hard to test #getFilePathUri since it only returns the result of a certain Android api call.
// But let's try.
@Test
fun `getFilePathUri - WHEN called without a registered provider THEN exception is thrown`() {
// There is no app registered provider that could expose a file from the filesystem of the machine running this test.
// Peeking into the exception would indicate whether the code really called "FileProvider.getUriForFile" as expected.
var exception: IllegalArgumentException? = null
try {
AbstractFetchDownloadService.getFilePathUri(testContext, "test.txt")
} catch (e: IllegalArgumentException) {
exception = e
}

assertTrue(exception!!.stackTrace[0].fileName.contains("FileProvider"))
assertTrue(exception.stackTrace[0].methodName == "getUriForFile")
}

@Test
@Config(shadows = [ShadowFileProvider::class])
fun `getFilePathUri - WHEN called THEN return a file provider path for the filePath`() {
// Test that the String filePath is passed to the provider from which we expect a Uri path
val result = AbstractFetchDownloadService.getFilePathUri(testContext, "location/test.txt")

assertTrue(result.toString().endsWith("location/test.txt"))
}
}

@Implements(FileProvider::class)
object ShadowFileProvider {
@Implementation
@JvmStatic
@Suppress("UNUSED_PARAMETER")
fun getUriForFile(
context: Context?,
authority: String?,
file: File
) = "content://authority/random/location/${file.name}".toUri()
}
7 changes: 7 additions & 0 deletions components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt
View file
Expand Up @@ -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
24 changes: 24 additions & 0 deletions components/support/ktx/src/test/java/mozilla/components/support/ktx/kotlin/StringTest.kt
View file
Expand Up @@ -9,6 +9,7 @@ import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertSame
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
Expand Down Expand Up @@ -209,4 +210,27 @@ class StringTest {

assertEquals("gif", result)
}

@Test
fun `ifNullOrEmpty returns the same if this CharSequence is not null and not empty`() {
val randomString = "something"

assertSame(randomString, randomString.ifNullOrEmpty { "something else" })
}

@Test
fun `ifNullOrEmpty returns the invocation of the passed in argument if this CharSequence is null`() {
val nullString: String? = null
val validResult = "notNullString"

assertSame(validResult, nullString.ifNullOrEmpty { validResult })
}

@Test
fun `ifNullOrEmpty returns the invocation of the passed in argument if this CharSequence is empty`() {
val nullString = ""
val validResult = "notEmptyString"

assertSame(validResult, nullString.ifNullOrEmpty { validResult })
}
}
3 changes: 3 additions & 0 deletions docs/changelog.md
View file
Expand Up @@ -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.

* **intent-processing**
* 🌟️ Added support for opening links from ACTION_MAIN Intents. This Intents could the result of `Intent.makeMainSelectorActivity(Intent.ACTION_MAIN, Intent.CATEGORY_APP_BROWSER)` calls.

Expand Down

0 comments on commit dcc78fd

Please sign in to comment.