diff --git a/src/com/android/email/Controller.java b/src/com/android/email/Controller.java index da9e8cbd8..132f2daea 100644 --- a/src/com/android/email/Controller.java +++ b/src/com/android/email/Controller.java @@ -53,6 +53,7 @@ import android.util.Log; import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; +import java.util.Collection; import java.util.HashSet; import java.util.concurrent.ConcurrentHashMap; @@ -176,6 +177,10 @@ public class Controller { } } + public Collection getResultCallbacksForTest() { + return mListeners; + } + /** * Delete all Messages that live in the attachment mailbox */ diff --git a/src/com/android/email/activity/MailboxFinder.java b/src/com/android/email/activity/MailboxFinder.java index c1f8cf7da..45f58bbb5 100644 --- a/src/com/android/email/activity/MailboxFinder.java +++ b/src/com/android/email/activity/MailboxFinder.java @@ -34,17 +34,22 @@ import android.util.Log; * * If an account doesn't have a mailbox of a specified type, it refreshes the mailbox list and * try looking for again. + * + * This is a "one-shot" class. You create an instance, call {@link #startLookup}, get a result + * or call {@link #cancel}, and that's it. The instance can't be re-used. */ public class MailboxFinder { private final Context mContext; private final Controller mController; - private final Controller.Result mControllerResults; + private Controller.Result mControllerResults; // Not final, we null it out when done. private final long mAccountId; private final int mMailboxType; private final Callback mCallback; private FindMailboxTask mTask; + private boolean mStarted; + private boolean mClosed; /** * Callback for results. @@ -81,41 +86,56 @@ public class MailboxFinder { * Must be called on the UI thread. */ public void startLookup() { - stop(); + if (mStarted) { + throw new IllegalStateException(); // Can't start twice. + } + mStarted = true; mTask = new FindMailboxTask(true); mTask.execute(); } /** - * Stop the running worker task, if exists. + * Cancel the operation. It's safe to call it multiple times, or even if the operation is + * already finished. */ - public void stop() { - Utility.cancelTaskInterrupt(mTask); - mTask = null; + public void cancel() { + if (!mClosed) { + close(); + } } /** - * Stop the running task, if exists, and clean up internal resources. (MUST be called.) + * Stop the running task, if exists, and clean up internal resources. */ - public void close() { - mController.removeResultCallback(mControllerResults); - stop(); + private void close() { + mClosed = true; + if (mControllerResults != null) { + mController.removeResultCallback(mControllerResults); + mControllerResults = null; + } + Utility.cancelTaskInterrupt(mTask); + mTask = null; } private class ControllerResults extends Controller.Result { @Override public void updateMailboxListCallback(MessagingException result, long accountId, int progress) { + if (mClosed || (accountId != mAccountId)) { + return; // Already closed, or non-target account. + } Log.i(Email.LOG_TAG, "MailboxFinder: updateMailboxListCallback"); if (result != null) { // Error while updating the mailbox list. Notify the UI... - mCallback.onMailboxNotFound(mAccountId); - } else { - // Messagebox list updated, look for mailbox again... - if (progress == 100 && accountId == mAccountId) { - mTask = new FindMailboxTask(false); - mTask.execute(); + try { + mCallback.onMailboxNotFound(mAccountId); + } finally { + close(); } + } else if (progress == 100) { + // Mailbox list updated, look for mailbox again... + mTask = new FindMailboxTask(false); + mTask.execute(); } } } @@ -180,26 +200,42 @@ public class MailboxFinder { switch (mResult) { case RESULT_ACCOUNT_SECURITY_HOLD: Log.w(Email.LOG_TAG, "MailboxFinder: Account security hold."); - mCallback.onAccountSecurityHold(mAccountId); + try { + mCallback.onAccountSecurityHold(mAccountId); + } finally { + close(); + } return; case RESULT_ACCOUNT_NOT_FOUND: Log.w(Email.LOG_TAG, "MailboxFinder: Account not found."); - mCallback.onAccountNotFound(); + try { + mCallback.onAccountNotFound(); + } finally { + close(); + } return; case RESULT_MAILBOX_NOT_FOUND: Log.w(Email.LOG_TAG, "MailboxFinder: Mailbox not found."); - mCallback.onMailboxNotFound(mAccountId); - return; - case RESULT_START_NETWORK_LOOK_UP: - // Not found locally. Let's sync the mailbox list... - Log.i(Email.LOG_TAG, "MailboxFinder: Starting network lookup."); - mController.updateMailboxList(mAccountId); + try { + mCallback.onMailboxNotFound(mAccountId); + } finally { + close(); + } return; case RESULT_MAILBOX_FOUND: if (Email.DEBUG_LIFECYCLE && Email.DEBUG) { Log.d(Email.LOG_TAG, "MailboxFinder: mailbox found: id=" + mailboxId); } - mCallback.onMailboxFound(mAccountId, mailboxId); + try { + mCallback.onMailboxFound(mAccountId, mailboxId); + } finally { + close(); + } + return; + case RESULT_START_NETWORK_LOOK_UP: + // Not found locally. Let's sync the mailbox list... + Log.i(Email.LOG_TAG, "MailboxFinder: Starting network lookup."); + mController.updateMailboxList(mAccountId); return; default: throw new RuntimeException(); @@ -207,6 +243,17 @@ public class MailboxFinder { } } + /* package */ boolean isStartedForTest() { + return mStarted; + } + + /** + * Called by unit test. Return true if all the internal resources are really released. + */ + /* package */ boolean isReallyClosedForTest() { + return mClosed && (mTask == null) && (mControllerResults == null); + } + /* package */ Controller.Result getControllerResultsForTest() { return mControllerResults; } diff --git a/src/com/android/email/activity/MessageList.java b/src/com/android/email/activity/MessageList.java index d996a8819..69dc0e329 100644 --- a/src/com/android/email/activity/MessageList.java +++ b/src/com/android/email/activity/MessageList.java @@ -252,7 +252,7 @@ public class MessageList extends Activity implements OnClickListener, super.onDestroy(); if (mMailboxFinder != null) { - mMailboxFinder.close(); + mMailboxFinder.cancel(); mMailboxFinder = null; } Utility.cancelTaskInterrupt(mSetTitleTask); diff --git a/src/com/android/email/activity/MessageListXLFragmentManager.java b/src/com/android/email/activity/MessageListXLFragmentManager.java index 4793db030..00c162919 100644 --- a/src/com/android/email/activity/MessageListXLFragmentManager.java +++ b/src/com/android/email/activity/MessageListXLFragmentManager.java @@ -448,7 +448,7 @@ class MessageListXLFragmentManager { private void closeMailboxFinder() { if (mMailboxFinder != null) { - mMailboxFinder.close(); + mMailboxFinder.cancel(); mMailboxFinder = null; } } diff --git a/tests/src/com/android/email/activity/MailboxFinderTest.java b/tests/src/com/android/email/activity/MailboxFinderTest.java index e26219505..4e4cc9c1c 100644 --- a/tests/src/com/android/email/activity/MailboxFinderTest.java +++ b/tests/src/com/android/email/activity/MailboxFinderTest.java @@ -67,15 +67,31 @@ public class MailboxFinderTest extends InstrumentationTestCase { mCallback = new MockCallback(); mMockController = new MockController(getContext()); Controller.injectMockControllerForTest(mMockController); + assertEquals(0, mMockController.getResultCallbacksForTest().size()); } @Override protected void tearDown() throws Exception { super.tearDown(); + if (mMailboxFinder != null) { + mMailboxFinder.cancel(); + + // MailboxFinder should unregister its listener when closed. + checkControllerResultRemoved(mMockController); + } mMockController.cleanupForTest(); Controller.injectMockControllerForTest(null); } + /** + * Make sure no {@link MailboxFinder.Callback} is left registered to the controller. + */ + private static void checkControllerResultRemoved(Controller controller) { + for (Controller.Result callback : controller.getResultCallbacksForTest()) { + assertFalse(callback instanceof MailboxFinder.Callback); + } + } + /** * Create an account and returns the ID. */ @@ -113,6 +129,7 @@ public class MailboxFinderTest extends InstrumentationTestCase { mMailboxFinder = new MailboxFinder(mProviderContext, accountId, mailboxType, mCallback); mMailboxFinder.startLookup(); + assertTrue(mMailboxFinder.isStartedForTest()); } }); } @@ -144,6 +161,8 @@ public class MailboxFinderTest extends InstrumentationTestCase { assertFalse(mCallback.mCalledMailboxFound); assertFalse(mCallback.mCalledMailboxNotFound); assertFalse(mMockController.mCalledUpdateMailboxList); + + assertTrue(mMailboxFinder.isReallyClosedForTest()); } /** @@ -158,6 +177,8 @@ public class MailboxFinderTest extends InstrumentationTestCase { assertFalse(mCallback.mCalledMailboxFound); assertFalse(mCallback.mCalledMailboxNotFound); assertFalse(mMockController.mCalledUpdateMailboxList); + + assertTrue(mMailboxFinder.isReallyClosedForTest()); } /** @@ -178,19 +199,19 @@ public class MailboxFinderTest extends InstrumentationTestCase { assertEquals(accountId, mCallback.mAccountId); assertEquals(mailboxId, mCallback.mMailboxId); + + assertTrue(mMailboxFinder.isReallyClosedForTest()); } /** - * Test: Account exists, but mailbox doesn't -> Get {@link Controller} to update the mailbox - * list -> mailbox still doesn't exist. + * Common initialization for tests that involves network-lookup. */ - public void testMailboxNotFound() throws Throwable { - final long accountId = createAccount(false); - + private void prepareForNetworkLookupTest(final long accountId) throws Throwable { + // Look for non-existing mailbox. createAndStartFinder(accountId, Mailbox.TYPE_INBOX); waitUntilCallbackCalled(); - // Mailbox not found, so the finder try network-looking up. + // Mailbox not found, so the finder should try network-looking up. assertFalse(mCallback.mCalledAccountNotFound); assertFalse(mCallback.mCalledAccountSecurityHold); assertFalse(mCallback.mCalledMailboxFound); @@ -202,6 +223,18 @@ public class MailboxFinderTest extends InstrumentationTestCase { mMockController.reset(); + assertFalse(mMailboxFinder.isReallyClosedForTest()); // Not closed yet + } + + /** + * Test: Account exists, but mailbox doesn't -> Get {@link Controller} to update the mailbox + * list -> mailbox still doesn't exist. + */ + public void testMailboxNotFound() throws Throwable { + final long accountId = createAccount(false); + + prepareForNetworkLookupTest(accountId); + // Imitate the mCallback... runTestOnUiThread(new Runnable() { @Override @@ -219,6 +252,8 @@ public class MailboxFinderTest extends InstrumentationTestCase { assertFalse(mCallback.mCalledMailboxFound); assertTrue(mCallback.mCalledMailboxNotFound); assertFalse(mMockController.mCalledUpdateMailboxList); + + assertTrue(mMailboxFinder.isReallyClosedForTest()); } /** @@ -228,20 +263,7 @@ public class MailboxFinderTest extends InstrumentationTestCase { public void testMailboxFoundOnNetwork() throws Throwable { final long accountId = createAccount(false); - createAndStartFinder(accountId, Mailbox.TYPE_INBOX); - waitUntilCallbackCalled(); - - // Mailbox not found, so the finder try network-looking up. - assertFalse(mCallback.mCalledAccountNotFound); - assertFalse(mCallback.mCalledAccountSecurityHold); - assertFalse(mCallback.mCalledMailboxFound); - assertFalse(mCallback.mCalledMailboxNotFound); - - // Controller.updateMailboxList() should have been called, with the account id. - assertTrue(mMockController.mCalledUpdateMailboxList); - assertEquals(accountId, mMockController.mPassedAccountId); - - mMockController.reset(); + prepareForNetworkLookupTest(accountId); // Create mailbox at this point. final long mailboxId = createMailbox(accountId, Mailbox.TYPE_INBOX); @@ -266,6 +288,8 @@ public class MailboxFinderTest extends InstrumentationTestCase { assertEquals(accountId, mCallback.mAccountId); assertEquals(mailboxId, mCallback.mMailboxId); + + assertTrue(mMailboxFinder.isReallyClosedForTest()); } /** @@ -275,20 +299,7 @@ public class MailboxFinderTest extends InstrumentationTestCase { public void testMailboxNotFoundNetworkError() throws Throwable { final long accountId = createAccount(false); - createAndStartFinder(accountId, Mailbox.TYPE_INBOX); - waitUntilCallbackCalled(); - - // Mailbox not found, so the finder try network-looking up. - assertFalse(mCallback.mCalledAccountNotFound); - assertFalse(mCallback.mCalledAccountSecurityHold); - assertFalse(mCallback.mCalledMailboxFound); - assertFalse(mCallback.mCalledMailboxNotFound); - - // Controller.updateMailboxList() should have been called, with the account id. - assertTrue(mMockController.mCalledUpdateMailboxList); - assertEquals(accountId, mMockController.mPassedAccountId); - - mMockController.reset(); + prepareForNetworkLookupTest(accountId); // Imitate the mCallback... runTestOnUiThread(new Runnable() { @@ -305,6 +316,36 @@ public class MailboxFinderTest extends InstrumentationTestCase { assertFalse(mCallback.mCalledMailboxFound); assertTrue(mCallback.mCalledMailboxNotFound); assertFalse(mMockController.mCalledUpdateMailboxList); + + assertTrue(mMailboxFinder.isReallyClosedForTest()); + } + + /** + * Test: updateMailboxListCallback won't respond to update of a non-target account. + */ + public void testUpdateMailboxListCallbackNonTarget() throws Throwable { + final long accountId = createAccount(false); + + prepareForNetworkLookupTest(accountId); + + // Callback from Controller, but for a different account. + runTestOnUiThread(new Runnable() { + @Override + public void run() { + long nonTargetAccountId = accountId + 1; + mMailboxFinder.getControllerResultsForTest().updateMailboxListCallback( + new MessagingException("Network error"), nonTargetAccountId, 0); + } + }); + + // Nothing happened. + assertFalse(mCallback.mCalledAccountNotFound); + assertFalse(mCallback.mCalledAccountSecurityHold); + assertFalse(mCallback.mCalledMailboxFound); + assertFalse(mCallback.mCalledMailboxNotFound); + assertFalse(mMockController.mCalledUpdateMailboxList); + + assertFalse(mMailboxFinder.isReallyClosedForTest()); // Not closed yet } /** @@ -322,6 +363,30 @@ public class MailboxFinderTest extends InstrumentationTestCase { assertFalse(mCallback.mCalledMailboxFound); assertFalse(mCallback.mCalledMailboxNotFound); assertTrue(mMockController.mCalledUpdateMailboxList); + + assertFalse(mMailboxFinder.isReallyClosedForTest()); // Not closed yet -- network lookup. + } + + /** + * Test: Call {@link MailboxFinder#startLookup()} twice, which should throw an ISE. + */ + public void testRunTwice() throws Throwable { + final long accountId = createAccount(true); + + createAndStartFinder(accountId, Mailbox.TYPE_INBOX); + try { + mMailboxFinder.startLookup(); + fail("Expected exception not thrown"); + } catch (IllegalStateException ok) { + } + } + + public void testCancel() throws Throwable { + final long accountId = createAccount(true); + + createAndStartFinder(accountId, Mailbox.TYPE_INBOX); + mMailboxFinder.cancel(); + assertTrue(mMailboxFinder.isReallyClosedForTest()); } /**