From ed9938cd9caf4937eadb3e35333e2fe2b157bde5 Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Wed, 26 Jan 2011 09:03:11 -0800 Subject: [PATCH] Protect against threading issues in AttachmentDownloadService Bug: 3391148 Change-Id: I513b5678815db262ea5660661336432f1e203c9e --- .../service/AttachmentDownloadService.java | 51 +++++++++------- .../AttachmentDownloadServiceTests.java | 60 +++++++++++++++---- 2 files changed, 78 insertions(+), 33 deletions(-) diff --git a/src/com/android/email/service/AttachmentDownloadService.java b/src/com/android/email/service/AttachmentDownloadService.java index dc17b20e4..194e97621 100644 --- a/src/com/android/email/service/AttachmentDownloadService.java +++ b/src/com/android/email/service/AttachmentDownloadService.java @@ -27,7 +27,6 @@ import com.android.email.provider.AttachmentProvider; import com.android.email.provider.EmailContent; import com.android.email.provider.EmailContent.Account; import com.android.email.provider.EmailContent.Attachment; -import com.android.email.provider.EmailContent.AttachmentColumns; import com.android.email.provider.EmailContent.Message; import com.android.exchange.ExchangeService; @@ -88,10 +87,12 @@ public class AttachmentDownloadService extends Service implements Runnable { // Limit on the number of attachments we'll check for background download private static final int MAX_ATTACHMENTS_TO_CHECK = 25; - /*package*/ static AttachmentDownloadService sRunningService = null; + // sRunningService is only set in the UI thread; it's visibility elsewhere is guaranteed + // by the use of "volatile" + /*package*/ static volatile AttachmentDownloadService sRunningService = null; /*package*/ Context mContext; - private EmailConnectivityManager mConnectivityManager; + /*package*/ EmailConnectivityManager mConnectivityManager; /*package*/ final DownloadSet mDownloadSet = new DownloadSet(new DownloadComparator()); @@ -247,7 +248,7 @@ public class AttachmentDownloadService extends Service implements Runnable { * necessary that we detect a deleted attachment, as the code always checks for the * existence of an attachment before acting on it. */ - public synchronized void onChange(Attachment att) { + public synchronized void onChange(Context context, Attachment att) { DownloadRequest req = findDownloadRequest(att.mId); long priority = getPriority(att); if (priority == PRIORITY_NONE) { @@ -268,7 +269,7 @@ public class AttachmentDownloadService extends Service implements Runnable { if (mDownloadsInProgress.containsKey(att.mId)) return; // If this is new, add the request to the queue if (req == null) { - req = new DownloadRequest(mContext, att); + req = new DownloadRequest(context, att); add(req); } // If the request already existed, we'll update the priority (so that the time is @@ -693,7 +694,7 @@ public class AttachmentDownloadService extends Service implements Runnable { } /*package*/ void onChange(Attachment att) { - mDownloadSet.onChange(att); + mDownloadSet.onChange(this, att); } /*package*/ boolean isQueued(long attachmentId) { @@ -721,8 +722,9 @@ public class AttachmentDownloadService extends Service implements Runnable { * @return the number of items queued for download */ public static int getQueueSize() { - if (sRunningService != null) { - return sRunningService.getSize(); + AttachmentDownloadService service = sRunningService; + if (service != null) { + return service.getSize(); } return 0; } @@ -733,8 +735,9 @@ public class AttachmentDownloadService extends Service implements Runnable { * @return whether or not the attachment is queued for download */ public static boolean isAttachmentQueued(long attachmentId) { - if (sRunningService != null) { - return sRunningService.isQueued(attachmentId); + AttachmentDownloadService service = sRunningService; + if (service != null) { + return service.isQueued(attachmentId); } return false; } @@ -745,15 +748,17 @@ public class AttachmentDownloadService extends Service implements Runnable { * @return whether or not the attachment was removed from the queue */ public static boolean cancelQueuedAttachment(long attachmentId) { - if (sRunningService != null) { - return sRunningService.dequeue(attachmentId); + AttachmentDownloadService service = sRunningService; + if (service != null) { + return service.dequeue(attachmentId); } return false; } public static void watchdogAlarm() { - if (sRunningService != null) { - sRunningService.mDownloadSet.onWatchdogAlarm(); + AttachmentDownloadService service = sRunningService; + if (service != null) { + service.mDownloadSet.onWatchdogAlarm(); } } @@ -763,17 +768,18 @@ public class AttachmentDownloadService extends Service implements Runnable { * @param flags the new flags for the attachment */ public static void attachmentChanged(final long id, final int flags) { - if (sRunningService == null) return; + final AttachmentDownloadService service = sRunningService; + if (service == null) return; Utility.runAsync(new Runnable() { public void run() { final Attachment attachment = - Attachment.restoreAttachmentWithId(sRunningService, id); + Attachment.restoreAttachmentWithId(service, id); if (attachment != null) { // Store the flags we got from EmailProvider; given that all of this // activity is asynchronous, we need to use the newest data from // EmailProvider attachment.mFlags = flags; - sRunningService.onChange(attachment); + service.onChange(attachment); } }}); } @@ -832,7 +838,9 @@ public class AttachmentDownloadService extends Service implements Runnable { } public void run() { + // These fields are only used within the service thread mContext = this; + mConnectivityManager = new EmailConnectivityManager(this, TAG); mAccountManagerStub = new AccountManagerStub(this); // Run through all attachments in the database that require download and add them to @@ -847,7 +855,7 @@ public class AttachmentDownloadService extends Service implements Runnable { Attachment attachment = Attachment.restoreAttachmentWithId( this, c.getLong(EmailContent.ID_PROJECTION_COLUMN)); if (attachment != null) { - mDownloadSet.onChange(attachment); + mDownloadSet.onChange(this, attachment); } } } catch (Exception e) { @@ -869,6 +877,9 @@ public class AttachmentDownloadService extends Service implements Runnable { } } } + + // Unregister now that we're done + mConnectivityManager.unregister(); } @Override @@ -885,7 +896,6 @@ public class AttachmentDownloadService extends Service implements Runnable { */ @Override public void onCreate() { - mConnectivityManager = new EmailConnectivityManager(this, TAG); // Start up our service thread new Thread(this, "AttachmentDownloadService").start(); } @@ -902,7 +912,6 @@ public class AttachmentDownloadService extends Service implements Runnable { kick(); } sRunningService = null; - mConnectivityManager.unregister(); } @Override @@ -918,7 +927,7 @@ public class AttachmentDownloadService extends Service implements Runnable { pw.println(" Account: " + req.accountId + ", Attachment: " + req.attachmentId); pw.println(" Priority: " + req.priority + ", Time: " + req.time + (req.inProgress ? " [In progress]" : "")); - Attachment att = Attachment.restoreAttachmentWithId(mContext, req.attachmentId); + Attachment att = Attachment.restoreAttachmentWithId(this, req.attachmentId); if (att == null) { pw.println(" Attachment not in database?"); } else if (att.mFileName != null) { diff --git a/tests/src/com/android/email/service/AttachmentDownloadServiceTests.java b/tests/src/com/android/email/service/AttachmentDownloadServiceTests.java index ba5ac9fbf..004bf4e57 100644 --- a/tests/src/com/android/email/service/AttachmentDownloadServiceTests.java +++ b/tests/src/com/android/email/service/AttachmentDownloadServiceTests.java @@ -17,6 +17,7 @@ package com.android.email.service; import com.android.email.AccountTestCase; +import com.android.email.EmailConnectivityManager; import com.android.email.ExchangeUtils.NullEmailService; import com.android.email.provider.EmailContent.Account; import com.android.email.provider.EmailContent.Attachment; @@ -56,8 +57,10 @@ public class AttachmentDownloadServiceTests extends AccountTestCase { // Set up an account and mailbox mAccount = ProviderTestUtils.setupAccount("account", false, mMockContext); + mAccount.mFlags |= Account.FLAGS_BACKGROUND_ATTACHMENTS; mAccount.save(mMockContext); mAccountId = mAccount.mId; + mMailbox = ProviderTestUtils.setupMailbox("mailbox", mAccountId, true, mMockContext); mMailboxId = mMailbox.mId; @@ -68,6 +71,7 @@ public class AttachmentDownloadServiceTests extends AccountTestCase { mService.addServiceClass(mAccountId, NullEmailService.class); mAccountManagerStub = new AttachmentDownloadService.AccountManagerStub(null); mService.mAccountManagerStub = mAccountManagerStub; + mService.mConnectivityManager = new MockConnectivityManager(getContext(), "mock"); mDownloadSet = mService.mDownloadSet; mMockDirectory = new MockDirectory(mService.mContext.getCacheDir().getAbsolutePath()); @@ -96,10 +100,10 @@ public class AttachmentDownloadServiceTests extends AccountTestCase { Attachment att4 = ProviderTestUtils.setupAttachment(message.mId, "filename4", 1000, Attachment.FLAG_DOWNLOAD_USER_REQUEST, true, mMockContext); // Indicate that these attachments have changed; they will be added to the queue - mDownloadSet.onChange(att1); - mDownloadSet.onChange(att2); - mDownloadSet.onChange(att3); - mDownloadSet.onChange(att4); + mDownloadSet.onChange(mMockContext, att1); + mDownloadSet.onChange(mMockContext, att2); + mDownloadSet.onChange(mMockContext, att3); + mDownloadSet.onChange(mMockContext, att4); Iterator iterator = mDownloadSet.descendingIterator(); // Check the expected ordering; 1 & 4 are higher priority than 2 & 3 // 1 and 3 were created earlier than their priority equals @@ -156,7 +160,7 @@ public class AttachmentDownloadServiceTests extends AccountTestCase { * A mock file directory containing a single (Mock)File. The total space, usable space, and * length of the single file can be set */ - static class MockDirectory extends File { + private static class MockDirectory extends File { private static final long serialVersionUID = 1L; private long mTotalSpace; private long mUsableSpace; @@ -195,7 +199,7 @@ public class AttachmentDownloadServiceTests extends AccountTestCase { /** * A mock file that reports back a pre-set length */ - static class MockFile extends File { + private static class MockFile extends File { private static final long serialVersionUID = 1L; private long mLength = 0; @@ -208,6 +212,21 @@ public class AttachmentDownloadServiceTests extends AccountTestCase { } } + private static class MockConnectivityManager extends EmailConnectivityManager { + public MockConnectivityManager(Context context, String name) { + super(context, name); + } + + @Override + public void waitForConnectivity() { + } + + @Override + public boolean isBackgroundDataAllowed() { + return true; + } + } + public void testCanPrefetchForAccount() { // First, test our "global" limits (based on free storage) // Mock storage @ 100 total and 26 available @@ -216,24 +235,41 @@ public class AttachmentDownloadServiceTests extends AccountTestCase { // Mock 2 accounts in total mAccountManagerStub.setNumberOfAccounts(2); // With 26% available, we should be ok to prefetch - assertTrue(mService.canPrefetchForAccount(1, mMockDirectory)); + assertTrue(mService.canPrefetchForAccount(mAccountId, mMockDirectory)); // Now change to 24 available mMockDirectory.setTotalAndUsableSpace(100L, 24L); // With 24% available, we should NOT be ok to prefetch - assertFalse(mService.canPrefetchForAccount(1, mMockDirectory)); + assertFalse(mService.canPrefetchForAccount(mAccountId, mMockDirectory)); // Now, test per-account storage // Mock storage @ 100 total and 50 available mMockDirectory.setTotalAndUsableSpace(100L, 50L); // Mock a file of length 12, but need to uncache previous amount first - mService.mAttachmentStorageMap.remove(1L); + mService.mAttachmentStorageMap.remove(mAccountId); mMockDirectory.setFileLength(11); // We can prefetch since 11 < 50/4 - assertTrue(mService.canPrefetchForAccount(1, mMockDirectory)); + assertTrue(mService.canPrefetchForAccount(mAccountId, mMockDirectory)); // Mock a file of length 13, but need to uncache previous amount first - mService.mAttachmentStorageMap.remove(1L); + mService.mAttachmentStorageMap.remove(mAccountId); mMockDirectory.setFileLength(13); // We can't prefetch since 13 > 50/4 - assertFalse(mService.canPrefetchForAccount(1, mMockDirectory)); + assertFalse(mService.canPrefetchForAccount(mAccountId, mMockDirectory)); + } + + public void testCanPrefetchForAccountNoBackgroundDownload() { + Account account = ProviderTestUtils.setupAccount("account2", false, mMockContext); + account.mFlags &= ~Account.FLAGS_BACKGROUND_ATTACHMENTS; + account.save(mMockContext); + + // First, test our "global" limits (based on free storage) + // Mock storage @ 100 total and 26 available + // Note that all file lengths in this test are in arbitrary units + mMockDirectory.setTotalAndUsableSpace(100L, 26L); + // Mock 2 accounts in total + mAccountManagerStub.setNumberOfAccounts(2); + + // With 26% available, we should be ok to prefetch, + // *but* bg download is disabled on the account. + assertFalse(mService.canPrefetchForAccount(account.mId, mMockDirectory)); } }