Protect against threading issues in AttachmentDownloadService

Bug: 3391148
Change-Id: I513b5678815db262ea5660661336432f1e203c9e

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) {
+        AttachmentDownloadService service = sRunningService;
-            return sRunningService.getSize();
+        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) {
+        AttachmentDownloadService service = sRunningService;
-            return sRunningService.isQueued(attachmentId);
+        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) {
+        AttachmentDownloadService service = sRunningService;
-            return sRunningService.dequeue(attachmentId);
+        if (service != null) {
+            return service.dequeue(attachmentId);
         }
         return false;
     }
 
     public static void watchdogAlarm() {
-        if (sRunningService != null) {
+        AttachmentDownloadService service = sRunningService;
-            sRunningService.mDownloadSet.onWatchdogAlarm();
+        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) {

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(mMockContext, att1);
-        mDownloadSet.onChange(att2);
+        mDownloadSet.onChange(mMockContext, att2);
-        mDownloadSet.onChange(att3);
+        mDownloadSet.onChange(mMockContext, att3);
-        mDownloadSet.onChange(att4);
+        mDownloadSet.onChange(mMockContext, att4);
         Iterator<DownloadRequest> 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));
     }
 }