ADHOC
/
replicant-packages_apps_Email
3
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

MailboxFinder should ignore callback for non-target account 

There were two cases where MailboxFinder responded to updateMailboxListCallback
when it shoulnd't.
- Callbacks for non-target accounts
- Callbacks arrived after the operation is finished

Make sure these callbacks are properly ignored.
Also, make sure startLookup() can't be called more than once.

Change-Id: I823c11ab5f96df4eb84594c08d3325d12319f708
This commit is contained in:
Makoto Onuki 2010-08-27 10:16:08 -07:00
parent 3e376afcb3
commit 086aac2386
5 changed files with 178 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
src/com/android/email/Controller.java
View File

@ -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<Result> getResultCallbacksForTest() {
return mListeners;
}
/**
* Delete all Messages that live in the attachment mailbox
*/

97
src/com/android/email/activity/MailboxFinder.java
View File

@ -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;
}

2
src/com/android/email/activity/MessageList.java
View File

@ -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);

2
src/com/android/email/activity/MessageListXLFragmentManager.java
View File

@ -448,7 +448,7 @@ class MessageListXLFragmentManager {
private void closeMailboxFinder() {
if (mMailboxFinder != null) {
mMailboxFinder.close();
mMailboxFinder.cancel();
mMailboxFinder = null;
}
}

133
tests/src/com/android/email/activity/MailboxFinderTest.java
View File

@ -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());
}
/**