From 2f6cbb021cd97e2450c29b72a27236ba4ef20823 Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Wed, 11 May 2011 17:15:59 -0700 Subject: [PATCH] Fix a race condition in which an Attachment might be wrongly deleted * This is a serious bug dating back to the first Honeycomb release * It was possible that a newly created Message could not yet be committed to the database when the AttachmentDownloadService tries to download one of that message's attachments. * ADS, when it sees that the message (apparently) doesn't exist, deletes the Attachment (it appears to be orphaned) * The effect is that the user never sees one of the attachments in a message. * This bug has been reported externally * The fix is simply to check for the message's existence before deciding to delete it (this check will always work properly) Bug: 4409692 Change-Id: I106ed2fe88d2435ad7a462fced5cb307c2559fd6 --- .../android/emailcommon/provider/EmailContent.java | 10 +++++++--- .../email/service/AttachmentDownloadService.java | 14 +++++++++++--- .../email/provider/AttachmentProviderTests.java | 4 ++-- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/emailcommon/src/com/android/emailcommon/provider/EmailContent.java b/emailcommon/src/com/android/emailcommon/provider/EmailContent.java index f2de83ae6..e3bd67d6e 100644 --- a/emailcommon/src/com/android/emailcommon/provider/EmailContent.java +++ b/emailcommon/src/com/android/emailcommon/provider/EmailContent.java @@ -1961,16 +1961,20 @@ public abstract class EmailContent { }; // All attachments with an empty URI, regardless of mailbox - public static final String EMPTY_URI_SELECTION = + public static final String PRECACHE_SELECTION = AttachmentColumns.CONTENT_URI + " isnull AND " + Attachment.FLAGS + "=0"; // Attachments with an empty URI that are in an inbox - public static final String EMPTY_URI_INBOX_SELECTION = - EMPTY_URI_SELECTION + " AND " + AttachmentColumns.MESSAGE_KEY + " IN (" + public static final String PRECACHE_INBOX_SELECTION = + PRECACHE_SELECTION + " AND " + AttachmentColumns.MESSAGE_KEY + " IN (" + "SELECT " + MessageColumns.ID + " FROM " + Message.TABLE_NAME + " WHERE " + Message.INBOX_SELECTION + ")"; // Bits used in mFlags + // WARNING: AttachmentDownloadService relies on the fact that ALL of the flags below + // disqualify attachments for precaching. If you add a flag that does NOT disqualify an + // attachment for precaching, you MUST change the PRECACHE_SELECTION definition above + // Instruct Rfc822Output to 1) not use Content-Disposition and 2) use multipart/alternative // with this attachment. This is only valid if there is one and only one attachment and // that attachment has this flag set diff --git a/src/com/android/email/service/AttachmentDownloadService.java b/src/com/android/email/service/AttachmentDownloadService.java index e52c2da72..e7824e1d8 100644 --- a/src/com/android/email/service/AttachmentDownloadService.java +++ b/src/com/android/email/service/AttachmentDownloadService.java @@ -342,7 +342,7 @@ public class AttachmentDownloadService extends Service implements Runnable { Uri lookupUri = EmailContent.uriWithLimit(Attachment.CONTENT_URI, MAX_ATTACHMENTS_TO_CHECK); Cursor c = mContext.getContentResolver().query(lookupUri, AttachmentInfo.PROJECTION, - EmailContent.Attachment.EMPTY_URI_INBOX_SELECTION, + EmailContent.Attachment.PRECACHE_INBOX_SELECTION, null, Attachment.RECORD_ID + " DESC"); File cacheDir = mContext.getCacheDir(); try { @@ -481,6 +481,7 @@ public class AttachmentDownloadService extends Service implements Runnable { mWatchdogPendingIntent = PendingIntent.getBroadcast(context, 0, alarmIntent, 0); mAlarmManager = (AlarmManager)context.getSystemService(Context.ALARM_SERVICE); } + private void cancelDownload(DownloadRequest req) { mDownloadsInProgress.remove(req.attachmentId); req.inProgress = false; @@ -572,8 +573,15 @@ public class AttachmentDownloadService extends Service implements Runnable { } } if (statusCode == EmailServiceStatus.MESSAGE_NOT_FOUND) { - // If there's no associated message, delete the attachment - EmailContent.delete(mContext, Attachment.CONTENT_URI, attachment.mId); + Message msg = Message.restoreMessageWithId(mContext, attachment.mMessageKey); + if (msg == null) { + // If there's no associated message, delete the attachment + EmailContent.delete(mContext, Attachment.CONTENT_URI, attachment.mId); + } else { + // If there really is a message, retry + kick(); + return; + } } else if (!deleted) { // Clear the download flags, since we're done for now. Note that this happens // only for non-recoverable errors. When these occur for forwarded mail, we can diff --git a/tests/src/com/android/email/provider/AttachmentProviderTests.java b/tests/src/com/android/email/provider/AttachmentProviderTests.java index d68a5225a..b25c8a4e2 100644 --- a/tests/src/com/android/email/provider/AttachmentProviderTests.java +++ b/tests/src/com/android/email/provider/AttachmentProviderTests.java @@ -235,7 +235,7 @@ public class AttachmentProviderTests extends ProviderTestCase2