Commit 5131ec9f authored by MozLando's avatar MozLando
Browse files

Merge #6393

6393: Closes #6317: Prevent inserting duplicate record into content resolver r=Amejia481 a=csadilek

OK, this hole was deep :). The cause of this crash is that we were unconditionally inserting into the content resolver which may already have a row / record of the download URI: https://github.com/mozilla-mobile/android-components/issues/6317#issuecomment-601119812



This can happen if a download fails or gets cancelled before we write the file. We will have a unique file name generated based on existing files, but also need to check if we have a record of the file in the resolver. If so, use it, otherwise create a new record.

I've tried for a few hours to write a meaningful test for this, but there are simply too many static methods involved here and the resulting refactoring was terrible: `ContentUris.withAppendedId`, `MediaStore.setIncludePending`, `MediaStore.Downloads.getContentUri` etc. That's properly the reason we don't have an existing test for this method :(

This also makes sure we now won't crash if for some reason there's another problem inserting into the content resolver, but fail the download instead.
Co-authored-by: default avatarChristian Sadilek <christian.sadilek@gmail.com>
parents bda5b650 5597c3c0
......@@ -10,11 +10,13 @@ import android.app.DownloadManager.EXTRA_DOWNLOAD_ID
import android.app.Service
import android.content.ActivityNotFoundException
import android.content.BroadcastReceiver
import android.content.ContentUris
import android.content.ContentValues
import android.content.Context
import android.content.Intent
import android.content.Intent.ACTION_VIEW
import android.content.IntentFilter
import android.net.Uri
import android.os.Build
import android.os.Build.VERSION.SDK_INT
import android.os.Environment
......@@ -441,14 +443,39 @@ abstract class AbstractFetchDownloadService : Service() {
val resolver = applicationContext.contentResolver
val collection = MediaStore.Downloads.getContentUri(MediaStore.VOLUME_EXTERNAL_PRIMARY)
val item = resolver.insert(collection, values)
val pfd = resolver.openFileDescriptor(item!!, "w")
ParcelFileDescriptor.AutoCloseOutputStream(pfd).use(block)
// Query if we have a pending download with the same name. This can happen
// if a download was interrupted, failed or cancelled before the file was
// written to disk. Our logic above will have generated a unique file name
// based on existing files on the device, but we might already have a row
// for the download in the content resolver.
var downloadUri: Uri? = null
resolver.query(
MediaStore.setIncludePending(collection),
arrayOf(MediaStore.Downloads._ID),
"${MediaStore.Downloads.DISPLAY_NAME} = ?",
arrayOf("${download.fileName}"),
null
)?.use {
if (it.count > 0) {
val idColumnIndex = it.getColumnIndex(MediaStore.Downloads._ID)
it.moveToFirst()
downloadUri = ContentUris.withAppendedId(collection, it.getLong(idColumnIndex))
}
}
if (downloadUri == null) {
downloadUri = resolver.insert(collection, values)
}
downloadUri?.let {
val pfd = resolver.openFileDescriptor(it, "w")
ParcelFileDescriptor.AutoCloseOutputStream(pfd).use(block)
values.clear()
values.put(MediaStore.Downloads.IS_PENDING, 0)
resolver.update(item, values, null, null)
values.clear()
values.put(MediaStore.Downloads.IS_PENDING, 0)
resolver.update(it, values, null, null)
} ?: throw IOException("Failed to register download with content resolver")
}
@TargetApi(Build.VERSION_CODES.P)
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment