From be2eaa55bb3dca3422da25fe907b50a4ad70df17 Mon Sep 17 00:00:00 2001 From: Vasu Nori Date: Thu, 14 Oct 2010 22:57:46 -0700 Subject: bug:3099994 NPE in DownloadManager when deleting non-media file DownloadService always scans files and assumes MediaProvider returns a valid Uri. But MediaProvider returns null for return param 'uri' if the file is not audio/video/image etc media type file (for example, pdf) Change-Id: If32bd1895b00b5406973a5e240ad3558d46f9f4a --- .../providers/downloads/DownloadService.java | 111 +++++++++++---------- src/com/android/providers/downloads/Helpers.java | 16 +++ 2 files changed, 73 insertions(+), 54 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadService.java b/src/com/android/providers/downloads/DownloadService.java index 0850af74..169ef970 100644 --- a/src/com/android/providers/downloads/DownloadService.java +++ b/src/com/android/providers/downloads/DownloadService.java @@ -20,6 +20,7 @@ import android.app.AlarmManager; import android.app.PendingIntent; import android.app.Service; import android.content.ComponentName; +import android.content.ContentResolver; import android.content.ContentUris; import android.content.ContentValues; import android.content.Context; @@ -54,6 +55,9 @@ import java.util.Set; * Performs the background downloads requested by applications that use the Downloads provider. */ public class DownloadService extends Service { + /** amount of time to wait to connect to MediaScannerService before timing out */ + private static final long WAIT_TIMEOUT = 10 * 1000; + /** Observer to get notified when the content observer's data changes */ private DownloadManagerContentObserver mObserver; @@ -97,18 +101,6 @@ public class DownloadService extends Service { @VisibleForTesting SystemFacade mSystemFacade; - /** Before calling - * {@link IMediaScannerService#requestScanFile(String, String, IMediaScannerListener)}, - * a (key,value) pair of (filepath, uri-to-update-the-row-in-dowloads-db) - * is stored in the following struct. - * When the callback from - * {@link IMediaScannerService#requestScanFile(String, String, IMediaScannerListener)} is - * received with params (filepath, uri-to-update-the-row-in-mediaprovider-db), - * this struct comes in handy to locate the row in downloads table whose mediaprovier-uri - * column is to be updated with the value returned by this callback method. - */ - private final HashMap downloadsToBeUpdated = new HashMap(); - /** * Receives notifications when the data in the content provider changes */ @@ -141,13 +133,16 @@ public class DownloadService extends Service { Log.v(Constants.TAG, "Connected to Media Scanner"); } synchronized (DownloadService.this) { - mMediaScannerConnecting = false; - mMediaScannerService = IMediaScannerService.Stub.asInterface(service); - if (mMediaScannerService != null) { - updateFromProvider(); + try { + mMediaScannerConnecting = false; + mMediaScannerService = IMediaScannerService.Stub.asInterface(service); + if (mMediaScannerService != null) { + updateFromProvider(); + } + } finally { + // notify anyone waiting on successful connection to MediaService + DownloadService.this.notifyAll(); } - // notify anyone waiting on successful connection to MediaService - DownloadService.this.notifyAll(); } } @@ -163,22 +158,26 @@ public class DownloadService extends Service { unbindService(this); } catch (IllegalArgumentException ex) { Log.w(Constants.TAG, "unbindService failed: " + ex); + } finally { + // notify anyone waiting on unsuccessful connection to MediaService + DownloadService.this.notifyAll(); } } - // notify anyone waiting on unsuccessful connection to MediaService - DownloadService.this.notifyAll(); } } public void onServiceDisconnected(ComponentName className) { - if (Constants.LOGVV) { - Log.v(Constants.TAG, "Disconnected from Media Scanner"); - } - synchronized (DownloadService.this) { - mMediaScannerService = null; - mMediaScannerConnecting = false; - // notify anyone waiting on disconnect from MediaService - DownloadService.this.notifyAll(); + try { + if (Constants.LOGVV) { + Log.v(Constants.TAG, "Disconnected from Media Scanner"); + } + } finally { + synchronized (DownloadService.this) { + mMediaScannerService = null; + mMediaScannerConnecting = false; + // notify anyone waiting on disconnect from MediaService + DownloadService.this.notifyAll(); + } } } } @@ -314,7 +313,7 @@ public class DownloadService extends Service { info = insertDownload(reader, now); } - if (info.shouldScanFile() && !scanFile(info, true)) { + if (info.shouldScanFile() && !scanFile(info, true, false)) { mustScan = true; keepService = true; } @@ -361,10 +360,16 @@ public class DownloadService extends Service { // this row is to be deleted from the database. but does it have // mediaProviderUri? if (TextUtils.isEmpty(info.mMediaProviderUri)) { - // initiate rescan of the file to - which will populate mediaProviderUri - // column in this row - if (!scanFile(info, true)) { - throw new IllegalStateException("scanFile failed!"); + if (info.shouldScanFile()) { + // initiate rescan of the file to - which will populate + // mediaProviderUri column in this row + if (!scanFile(info, true, false)) { + throw new IllegalStateException("scanFile failed!"); + } + } else { + // this file should NOT be scanned. delete the file. + Helpers.deleteFile(getContentResolver(), info.mId, info.mFileName, + info.mMimeType); } } else { // yes it has mediaProviderUri column already filled in. @@ -528,7 +533,7 @@ public class DownloadService extends Service { private void deleteDownload(long id) { DownloadInfo info = mDownloads.get(id); if (info.shouldScanFile()) { - scanFile(info, false); + scanFile(info, false, false); } if (info.mStatus == Downloads.Impl.STATUS_RUNNING) { info.mStatus = Downloads.Impl.STATUS_CANCELED; @@ -544,7 +549,8 @@ public class DownloadService extends Service { * Attempts to scan the file if necessary. * @return true if the file has been properly scanned. */ - private boolean scanFile(DownloadInfo info, boolean updateDatabase) { + private boolean scanFile(DownloadInfo info, final boolean updateDatabase, + final boolean deleteFile) { synchronized (this) { if (mMediaScannerService == null) { // not bound to mediaservice. but if in the process of connecting to it, wait until @@ -552,7 +558,7 @@ public class DownloadService extends Service { while (mMediaScannerConnecting) { Log.d(Constants.TAG, "waiting for mMediaScannerService service: "); try { - this.wait(); + this.wait(WAIT_TIMEOUT); } catch (InterruptedException e1) { throw new IllegalStateException("wait interrupted"); } @@ -566,33 +572,30 @@ public class DownloadService extends Service { if (Constants.LOGV) { Log.v(Constants.TAG, "Scanning file " + info.mFileName); } - if (updateDatabase) { - synchronized(downloadsToBeUpdated) { - Uri value = info.getAllDownloadsUri(); - if (value != null) { - downloadsToBeUpdated.put(info.mFileName, value); - } - } - } try { + final Uri key = info.getAllDownloadsUri(); + final String mimeType = info.mMimeType; + final ContentResolver resolver = getContentResolver(); + final long id = info.mId; mMediaScannerService.requestScanFile(info.mFileName, info.mMimeType, new IMediaScannerListener.Stub() { public void scanCompleted(String path, Uri uri) { - Uri key; - boolean updateMediaproviderUriColumn; - synchronized(downloadsToBeUpdated) { - key = downloadsToBeUpdated.get(path); - updateMediaproviderUriColumn = (key != null); - } - if (updateMediaproviderUriColumn) { + if (uri != null && updateDatabase) { + // file is scanned and mediaprovider returned uri. store it in downloads + // table (i.e., update this downloaded file's row) ContentValues values = new ContentValues(); values.put(Constants.MEDIA_SCANNED, 1); values.put(Downloads.Impl.COLUMN_MEDIAPROVIDER_URI, uri.toString()); getContentResolver().update(key, values, null, null); - synchronized(downloadsToBeUpdated) { - downloadsToBeUpdated.remove(path); - } + } else if (uri == null && deleteFile) { + // callback returned NO uri..that means this file doesn't + // exist in MediaProvider. but it still needs to be deleted + // TODO don't scan files that are not scannable by MediaScanner. + // create a public method in MediaFile.java to return false + // if the given file's mimetype is not any of the types + // the mediaprovider is interested in. + Helpers.deleteFile(resolver, id, path, mimeType); } } }); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index cbcae5f4..855cba28 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -16,6 +16,7 @@ package com.android.providers.downloads; +import android.content.ContentResolver; import android.content.ContentUris; import android.content.Context; import android.content.Intent; @@ -799,4 +800,19 @@ public class Helpers { (c >= '0' && c <= '9'); } } + + /** + * Delete the given file from device + * and delete its row from the downloads database. + */ + /* package */ static void deleteFile(ContentResolver resolver, long id, String path, String mimeType) { + try { + File file = new File(path); + file.delete(); + } catch (Exception e) { + Log.w(Constants.TAG, "file: '" + path + "' couldn't be deleted", e); + } + resolver.delete(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, Downloads.Impl._ID + " = ? ", + new String[]{String.valueOf(id)}); + } } -- cgit v1.2.3