Avoid calling into MediaProvider from DownloadProvider.onCreate().
- Postpone marking few potential files in mediastore as downloads
by reusing the MediaScanTriggerJob.
- Take care of deleting/orphaning downloads belonging to uninstalled
packages upon receiving MEDIA_MOUNTED broadcast. This should also
prevent DownloadProvider from accessing external storage before
it is available.
Fixes: 140200277
Fixes: 154350921
Test: atest tests/app/src/android/app/cts/DownloadManagerTest.java
Test: manual
Change-Id: I57a05495b6aed3443c20ca3ca3c8a06ae87f3748
diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java
index 5c68071..2ad91d2 100644
--- a/src/com/android/providers/downloads/DownloadProvider.java
+++ b/src/com/android/providers/downloads/DownloadProvider.java
@@ -320,6 +320,7 @@
* Upgrade database from (version - 1) to version.
*/
private void upgradeTo(SQLiteDatabase db, int version) {
+ boolean scheduleMediaScanTriggerJob = false;
switch (version) {
case 100:
createDownloadsTable(db);
@@ -381,7 +382,7 @@
case 111:
addColumn(db, DB_TABLE, Downloads.Impl.COLUMN_MEDIASTORE_URI,
"TEXT DEFAULT NULL");
- addMediaStoreUris(db);
+ scheduleMediaScanTriggerJob = true;
break;
case 112:
@@ -394,12 +395,15 @@
case 114:
nullifyMediaStoreUris(db);
- MediaScanTriggerJob.schedule(getContext());
+ scheduleMediaScanTriggerJob = true;
break;
default:
throw new IllegalStateException("Don't know how to upgrade to " + version);
}
+ if (scheduleMediaScanTriggerJob) {
+ MediaScanTriggerJob.schedule(getContext());
+ }
}
/**
@@ -436,53 +440,6 @@
}
/**
- * Add {@link Downloads.Impl#COLUMN_MEDIASTORE_URI} for all successful downloads and
- * add/update corresponding entries in MediaProvider.
- */
- private void addMediaStoreUris(@NonNull SQLiteDatabase db) {
- final String[] selectionArgs = new String[] {
- Integer.toString(Downloads.Impl.DESTINATION_EXTERNAL),
- Integer.toString(Downloads.Impl.DESTINATION_FILE_URI),
- Integer.toString(Downloads.Impl.DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD),
- };
- final CallingIdentity token = clearCallingIdentity();
- try (Cursor cursor = db.query(DB_TABLE, null,
- "_data IS NOT NULL AND is_visible_in_downloads_ui != '0'"
- + " AND (destination=? OR destination=? OR destination=?)",
- selectionArgs, null, null, null);
- ContentProviderClient client = getContext().getContentResolver()
- .acquireContentProviderClient(MediaStore.AUTHORITY)) {
- if (cursor.getCount() == 0) {
- return;
- }
- final DownloadInfo.Reader reader
- = new DownloadInfo.Reader(getContext().getContentResolver(), cursor);
- final DownloadInfo info = new DownloadInfo(getContext());
- final ContentValues updateValues = new ContentValues();
- while (cursor.moveToNext()) {
- reader.updateFromDatabase(info);
- final ContentValues mediaValues;
- try {
- mediaValues = convertToMediaProviderValues(info);
- } catch (IllegalArgumentException e) {
- Log.e(Constants.TAG, "Error getting media content values from " + info, e);
- continue;
- }
- final Uri mediaStoreUri = updateMediaProvider(client, mediaValues);
- if (mediaStoreUri != null) {
- updateValues.clear();
- updateValues.put(Downloads.Impl.COLUMN_MEDIASTORE_URI,
- mediaStoreUri.toString());
- db.update(DB_TABLE, updateValues, Downloads.Impl._ID + "=?",
- new String[] { Long.toString(info.mId) });
- }
- }
- } finally {
- restoreCallingIdentity(token);
- }
- }
-
- /**
* DownloadProvider has been updated to use MediaStore.Downloads based uris
* for COLUMN_MEDIASTORE_URI but the existing entries would still have MediaStore.Files
* based uris. It's possible that in the future we might incorrectly assume that all the
@@ -635,14 +592,28 @@
mStorageManager = getContext().getSystemService(StorageManager.class);
- reconcileRemovedUidEntries();
+ // Grant access permissions for all known downloads to the owning apps.
+ final SQLiteDatabase db = mOpenHelper.getReadableDatabase();
+ try (Cursor cursor = db.query(DB_TABLE,
+ new String[] { _ID, Constants.UID }, null, null, null, null, null)) {
+ while (cursor.moveToNext()) {
+ final long id = cursor.getLong(0);
+ final int uid = cursor.getInt(1);
+ final String[] packageNames = getContext().getPackageManager()
+ .getPackagesForUid(uid);
+ // Potentially stale download, will be deleted after MEDIA_MOUNTED broadcast
+ // is received.
+ if (ArrayUtils.isEmpty(packageNames)) {
+ continue;
+ }
+ // We only need to grant to the first package, since the
+ // platform internally tracks based on UIDs.
+ grantAllDownloadsPermission(packageNames[0], id);
+ }
+ }
return true;
}
- private void reconcileRemovedUidEntries() {
- Helpers.handleRemovedUidEntries(getContext(), this, -1, this::grantAllDownloadsPermission);
- }
-
/**
* Returns the content-provider-style MIME types of the various
* types accessible through this content provider.
@@ -954,7 +925,7 @@
* If an entry corresponding to given mediaValues doesn't already exist in MediaProvider,
* add it, otherwise update that entry with the given values.
*/
- private Uri updateMediaProvider(@NonNull ContentProviderClient mediaProvider,
+ Uri updateMediaProvider(@NonNull ContentProviderClient mediaProvider,
@NonNull ContentValues mediaValues) {
final String filePath = mediaValues.getAsString(MediaStore.DownloadColumns.DATA);
Uri mediaStoreUri = getMediaStoreUri(mediaProvider, filePath);
@@ -997,7 +968,7 @@
return null;
}
- private ContentValues convertToMediaProviderValues(DownloadInfo info) {
+ ContentValues convertToMediaProviderValues(DownloadInfo info) {
final String filePath;
try {
filePath = new File(info.mFileName).getCanonicalPath();
diff --git a/src/com/android/providers/downloads/DownloadReceiver.java b/src/com/android/providers/downloads/DownloadReceiver.java
index 27b5d99..a5da475 100644
--- a/src/com/android/providers/downloads/DownloadReceiver.java
+++ b/src/com/android/providers/downloads/DownloadReceiver.java
@@ -39,6 +39,7 @@
import android.content.Intent;
import android.database.Cursor;
import android.net.Uri;
+import android.os.Process;
import android.provider.Downloads;
import android.text.TextUtils;
import android.util.Log;
@@ -70,21 +71,18 @@
if (Intent.ACTION_BOOT_COMPLETED.equals(action)
|| Intent.ACTION_MEDIA_MOUNTED.equals(action)) {
final PendingResult result = goAsync();
- getAsyncHandler().post(new Runnable() {
- @Override
- public void run() {
- handleBootCompleted(context);
- result.finish();
+ getAsyncHandler().post(() -> {
+ handleBootCompleted(context);
+ if (Intent.ACTION_MEDIA_MOUNTED.equals(action)) {
+ handleRemovedUidEntries(context);
}
+ result.finish();
});
} else if (Intent.ACTION_UID_REMOVED.equals(action)) {
final PendingResult result = goAsync();
- getAsyncHandler().post(new Runnable() {
- @Override
- public void run() {
- handleUidRemoved(context, intent);
- result.finish();
- }
+ getAsyncHandler().post(() -> {
+ handleUidRemoved(context, intent);
+ result.finish();
});
} else if (Constants.ACTION_OPEN.equals(action)
@@ -96,12 +94,9 @@
// TODO: remove this once test is refactored
handleNotificationBroadcast(context, intent);
} else {
- getAsyncHandler().post(new Runnable() {
- @Override
- public void run() {
- handleNotificationBroadcast(context, intent);
- result.finish();
- }
+ getAsyncHandler().post(() -> {
+ handleNotificationBroadcast(context, intent);
+ result.finish();
});
}
} else if (Constants.ACTION_CANCEL.equals(action)) {
@@ -139,6 +134,14 @@
DownloadIdleService.scheduleIdlePass(context);
}
+ private void handleRemovedUidEntries(Context context) {
+ try (ContentProviderClient cpc = context.getContentResolver()
+ .acquireContentProviderClient(AUTHORITY)) {
+ Helpers.handleRemovedUidEntries(context, cpc.getLocalContentProvider(),
+ Process.INVALID_UID);
+ }
+ }
+
private void handleUidRemoved(Context context, Intent intent) {
final int uid = intent.getIntExtra(Intent.EXTRA_UID, -1);
if (uid == -1) {
@@ -147,7 +150,7 @@
try (ContentProviderClient cpc = context.getContentResolver()
.acquireContentProviderClient(AUTHORITY)) {
- Helpers.handleRemovedUidEntries(context, cpc.getLocalContentProvider(), uid, null);
+ Helpers.handleRemovedUidEntries(context, cpc.getLocalContentProvider(), uid);
}
}
diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java
index 545a4a5..eafe86d 100644
--- a/src/com/android/providers/downloads/Helpers.java
+++ b/src/com/android/providers/downloads/Helpers.java
@@ -21,6 +21,8 @@
import static android.os.Environment.buildExternalStorageAppMediaDirs;
import static android.os.Environment.buildExternalStorageAppObbDirs;
import static android.os.Environment.buildExternalStoragePublicDirs;
+import static android.os.Process.INVALID_UID;
+import static android.provider.Downloads.Impl.AUTHORITY;
import static android.provider.Downloads.Impl.COLUMN_DESTINATION;
import static android.provider.Downloads.Impl.DESTINATION_EXTERNAL;
import static android.provider.Downloads.Impl.DESTINATION_FILE_URI;
@@ -37,6 +39,7 @@
import android.app.job.JobScheduler;
import android.content.ComponentName;
import android.content.ContentProvider;
+import android.content.ContentProviderClient;
import android.content.ContentResolver;
import android.content.ContentValues;
import android.content.Context;
@@ -667,12 +670,11 @@
@VisibleForTesting
public static void handleRemovedUidEntries(@NonNull Context context,
- @NonNull ContentProvider downloadProvider, int removedUid,
- @Nullable BiConsumer<String, Long> validEntryConsumer) {
+ ContentProvider downloadProvider, int removedUid) {
final SparseArray<String> knownUids = new SparseArray<>();
final ArrayList<Long> idsToDelete = new ArrayList<>();
final ArrayList<Long> idsToOrphan = new ArrayList<>();
- final String selection = removedUid == -1 ? Constants.UID + " IS NOT NULL"
+ final String selection = removedUid == INVALID_UID ? Constants.UID + " IS NOT NULL"
: Constants.UID + "=" + removedUid;
try (Cursor cursor = downloadProvider.query(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI,
new String[] { Downloads.Impl._ID, Constants.UID, COLUMN_DESTINATION, _DATA },
@@ -702,8 +704,6 @@
} else {
idsToDelete.add(downloadId);
}
- } else if (validEntryConsumer != null) {
- validEntryConsumer.accept(ownerPackageName, downloadId);
}
}
}
diff --git a/src/com/android/providers/downloads/MediaScanTriggerJob.java b/src/com/android/providers/downloads/MediaScanTriggerJob.java
index 6e69ed2..d889fa3 100644
--- a/src/com/android/providers/downloads/MediaScanTriggerJob.java
+++ b/src/com/android/providers/downloads/MediaScanTriggerJob.java
@@ -39,20 +39,26 @@
import android.content.Context;
import android.database.Cursor;
import android.net.Uri;
-import android.os.RemoteException;
+import android.os.Environment;
import android.provider.Downloads;
import android.provider.MediaStore;
+import android.util.Log;
import java.io.File;
/**
- * Clean-up job to force mediascan on downloads which should have been but didn't get mediascanned.
+ * Job to update MediaProvider with all the downloads and force mediascan on them.
*/
public class MediaScanTriggerJob extends JobService {
private volatile boolean mJobStopped;
@Override
public boolean onStartJob(JobParameters parameters) {
+ if (!Environment.getExternalStorageState().equals(Environment.MEDIA_MOUNTED)) {
+ // External storage is not available yet, so defer this job to a later time.
+ jobFinished(parameters, true /* reschedule */);
+ return false;
+ }
Helpers.getAsyncHandler().post(() -> {
final String selection = _DATA + " IS NOT NULL"
+ " AND (" + COLUMN_IS_VISIBLE_IN_DOWNLOADS_UI + "=1"
@@ -61,45 +67,53 @@
+ " OR " + COLUMN_DESTINATION + "=" + DESTINATION_FILE_URI
+ " OR " + COLUMN_DESTINATION + "=" + DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD
+ ")";
- try (ContentProviderClient downloadProviderClient
+ try (ContentProviderClient cpc
= getContentResolver().acquireContentProviderClient(Downloads.Impl.AUTHORITY);
ContentProviderClient mediaProviderClient
= getContentResolver().acquireContentProviderClient(MediaStore.AUTHORITY)) {
- try (Cursor cursor = downloadProviderClient.query(ALL_DOWNLOADS_CONTENT_URI,
- new String[] {_ID, _DATA, COLUMN_MEDIASTORE_URI},
- selection, null, null)) {
+ final DownloadProvider downloadProvider
+ = ((DownloadProvider) cpc.getLocalContentProvider());
+ try (Cursor cursor = downloadProvider.query(ALL_DOWNLOADS_CONTENT_URI,
+ null, selection, null, null)) {
+
+ final DownloadInfo.Reader reader
+ = new DownloadInfo.Reader(getContentResolver(), cursor);
+ final DownloadInfo info = new DownloadInfo(MediaScanTriggerJob.this);
while (cursor.moveToNext()) {
if (mJobStopped) {
return;
}
+ reader.updateFromDatabase(info);
// This indicates that this entry has been handled already (perhaps when
// this job ran earlier and got preempted), so skip.
- if (cursor.getString(2) != null) {
+ if (info.mMediaStoreUri != null) {
continue;
}
- final long id = cursor.getLong(0);
- final String filePath = cursor.getString(1);
- final ContentValues mediaValues = new ContentValues();
+ final ContentValues mediaValues;
+ try {
+ mediaValues = downloadProvider.convertToMediaProviderValues(info);
+ } catch (IllegalArgumentException e) {
+ Log.e(Constants.TAG,
+ "Error getting media content values from " + info, e);
+ continue;
+ }
+ // Overriding size column value to 0 for forcing the mediascan
+ // later (to address http://b/138419471).
mediaValues.put(MediaStore.Files.FileColumns.SIZE, 0);
- mediaProviderClient.update(Helpers.getContentUriForPath(this, filePath),
- mediaValues,
- MediaStore.Files.FileColumns.DATA + "=?",
- new String[] { filePath });
+ downloadProvider.updateMediaProvider(mediaProviderClient, mediaValues);
final Uri mediaStoreUri = Helpers.triggerMediaScan(mediaProviderClient,
- new File(filePath));
+ new File(info.mFileName));
if (mediaStoreUri != null) {
final ContentValues downloadValues = new ContentValues();
downloadValues.put(COLUMN_MEDIASTORE_URI, mediaStoreUri.toString());
- downloadProviderClient.update(ALL_DOWNLOADS_CONTENT_URI,
- downloadValues, _ID + "=" + id, null);
+ downloadProvider.update(ALL_DOWNLOADS_CONTENT_URI,
+ downloadValues, _ID + "=" + info.mId, null);
}
}
- } catch (RemoteException e) {
- // Should not happen
}
}
- jobFinished(parameters, false);
+ jobFinished(parameters, false /* reschedule */);
});
return true;
}
diff --git a/tests/src/com/android/providers/downloads/HelpersTest.java b/tests/src/com/android/providers/downloads/HelpersTest.java
index 9cb99ba..61515ce 100644
--- a/tests/src/com/android/providers/downloads/HelpersTest.java
+++ b/tests/src/com/android/providers/downloads/HelpersTest.java
@@ -41,6 +41,7 @@
import android.database.MatrixCursor;
import android.net.Uri;
import android.os.Environment;
+import android.os.Process;
import android.provider.Downloads;
import android.test.AndroidTestCase;
@@ -189,10 +190,9 @@
final ContentProvider downloadProvider = mock(ContentProvider.class);
when(downloadProvider.query(eq(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI),
any(String[].class), any(String.class),isNull(), isNull())).thenReturn(cursor);
- final BiConsumer<String, Long> validEntryConsumer = mock(BiConsumer.class);
// Call
- Helpers.handleRemovedUidEntries(context, downloadProvider, -1, validEntryConsumer);
+ Helpers.handleRemovedUidEntries(context, downloadProvider, Process.INVALID_UID);
// Verify
verify(downloadProvider).update(eq(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI),
@@ -204,9 +204,6 @@
argThat(selection -> Arrays.equals(
idsToRemove.toArray(), extractIdsFromSelection(selection))),
isNull());
- for (int i = 0; i < validEntries.size(); ++i) {
- verify(validEntryConsumer).accept(validEntries.valueAt(i), validEntries.keyAt(i));
- }
// Reset
@@ -222,7 +219,7 @@
any(String[].class), any(String.class),isNull(), isNull())).thenReturn(cursor2);
// Call
- Helpers.handleRemovedUidEntries(context, downloadProvider, TEST_UID2, null);
+ Helpers.handleRemovedUidEntries(context, downloadProvider, TEST_UID2);
// Verify
verify(downloadProvider).update(eq(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI),
