From 977a7d206a866f07774d98aa2ffa2c51aa057de1 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Tue, 1 Jun 2010 13:36:53 -0700 Subject: [PATCH] Always destroy ImapResponses. Unfortunately it's hard to write tests for this change, but at least all tests pass with Idc7b88c4. Change-Id: If0335a848dfcc23aecea22c21b2cce73dac7ff6f --- .../android/email/mail/store/ImapStore.java | 110 ++++++++++---- .../mail/store/imap/ImapResponseParser.java | 137 ++++++++++++------ .../mail/store/imap/ImapTempFileLiteral.java | 4 +- 3 files changed, 173 insertions(+), 78 deletions(-) diff --git a/src/com/android/email/mail/store/ImapStore.java b/src/com/android/email/mail/store/ImapStore.java index 1bc5c5059..20c87ba9e 100644 --- a/src/com/android/email/mail/store/ImapStore.java +++ b/src/com/android/email/mail/store/ImapStore.java @@ -414,18 +414,21 @@ public class ImapStore extends Store { connection.close(); throw new MessagingException("Unable to get folder list.", ioe); } finally { + connection.destroyResponses(); poolConnection(connection); } } @Override public void checkSettings() throws MessagingException { + ImapConnection connection = new ImapConnection(); try { - ImapConnection connection = new ImapConnection(); connection.open(); connection.close(); } catch (IOException ioe) { throw new MessagingException(MessagingException.IOERROR, ioe.toString()); + } finally { + connection.destroyResponses(); } } @@ -442,6 +445,8 @@ public class ImapStore extends Store { // Fall through } catch (IOException e) { // Fall through + } finally { + connection.destroyResponses(); } connection.close(); connection = null; @@ -522,6 +527,8 @@ public class ImapStore extends Store { } catch (IOException ioe) { ioExceptionHandler(mConnection, ioe); + } finally { + mConnection.destroyResponses(); } } else { // Return the connection to the pool, if exists. @@ -576,6 +583,8 @@ public class ImapStore extends Store { } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); + } finally { + mConnection.destroyResponses(); } } catch (MessagingException e) { mExists = false; @@ -641,6 +650,7 @@ public class ImapStore extends Store { throw ioExceptionHandler(connection, ioe); } finally { + connection.destroyResponses(); if (mConnection == null) { mStore.poolConnection(connection); } @@ -680,6 +690,7 @@ public class ImapStore extends Store { throw ioExceptionHandler(connection, ioe); } finally { + connection.destroyResponses(); if (mConnection == null) { mStore.poolConnection(connection); } @@ -697,6 +708,8 @@ public class ImapStore extends Store { encodeFolderName(folder.getName()))); } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); + } finally { + mConnection.destroyResponses(); } } @@ -723,6 +736,8 @@ public class ImapStore extends Store { return unreadMessageCount; } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); + } finally { + mConnection.destroyResponses(); } } @@ -736,32 +751,36 @@ public class ImapStore extends Store { checkOpen(); List responses; try { - responses = mConnection.executeSimpleCommand( - ImapConstants.UID_SEARCH + " " + searchCriteria); - } catch (ImapException e) { - return Utility.EMPTY_STRINGS; // not found; - } catch (IOException ioe) { - throw ioExceptionHandler(mConnection, ioe); - } - // S: * SEARCH 2 3 6 - for (ImapResponse response : responses) { - if (!response.isDataResponse(0, ImapConstants.SEARCH)) { - continue; + try { + responses = mConnection.executeSimpleCommand( + ImapConstants.UID_SEARCH + " " + searchCriteria); + } catch (ImapException e) { + return Utility.EMPTY_STRINGS; // not found; + } catch (IOException ioe) { + throw ioExceptionHandler(mConnection, ioe); } - // Found SEARCH response data - final int count = response.size() - 1; - if (count <= 0) { - return Utility.EMPTY_STRINGS; // ... but no UIDs in it! Return empty array. - } - - ArrayList ret = new ArrayList(count); - for (int i = 1; i < response.size(); i++) { - ImapString s = response.getStringOrEmpty(i); - if (s.isString()) { - ret.add(s.getString()); + // S: * SEARCH 2 3 6 + for (ImapResponse response : responses) { + if (!response.isDataResponse(0, ImapConstants.SEARCH)) { + continue; } + // Found SEARCH response data + final int count = response.size() - 1; + if (count <= 0) { + return Utility.EMPTY_STRINGS; // ... but no UIDs in it! Return empty array. + } + + ArrayList ret = new ArrayList(count); + for (int i = 1; i < response.size(); i++) { + ImapString s = response.getStringOrEmpty(i); + if (s.isString()) { + ret.add(s.getString()); + } + } + return ret.toArray(Utility.EMPTY_STRINGS); } - return ret.toArray(Utility.EMPTY_STRINGS); + } finally { + mConnection.destroyResponses(); } return Utility.EMPTY_STRINGS; } @@ -826,6 +845,8 @@ public class ImapStore extends Store { Log.w(Email.LOG_TAG, "Exception detected: " + e.getMessage()); mConnection.logLastDiscourse(); throw e; + } finally { + mConnection.destroyResponses(); } } @@ -978,9 +999,7 @@ public class ImapStore extends Store { listener.messageRetrieved(message); } } finally { - if (response != null) { - response.destroy(); - } + mConnection.destroyResponses(); } } while (!response.isTagged()); } catch (IOException ioe) { @@ -1273,6 +1292,8 @@ public class ImapStore extends Store { } } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); + } finally { + mConnection.destroyResponses(); } } @@ -1283,6 +1304,8 @@ public class ImapStore extends Store { handleUntaggedResponses(mConnection.executeSimpleCommand(ImapConstants.EXPUNGE)); } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); + } finally { + mConnection.destroyResponses(); } return null; } @@ -1316,6 +1339,8 @@ public class ImapStore extends Store { } catch (IOException ioe) { throw ioExceptionHandler(mConnection, ioe); + } finally { + mConnection.destroyResponses(); } } @@ -1371,7 +1396,7 @@ public class ImapStore extends Store { mTransport.open(); mTransport.setSoTimeout(MailTransport.SOCKET_READ_TIMEOUT); - mParser = new ImapResponseParser(mTransport.getInputStream(), mDiscourse); + createParser(); // BANNER mParser.readResponse(); @@ -1395,8 +1420,7 @@ public class ImapStore extends Store { mTransport.reopenTls(); mTransport.setSoTimeout(MailTransport.SOCKET_READ_TIMEOUT); - mParser = new ImapResponseParser(mTransport.getInputStream(), - mDiscourse); + createParser(); } else { if (Config.LOGD && Email.DEBUG) { Log.d(Email.LOG_TAG, "TLS not supported but required"); @@ -1457,6 +1481,8 @@ public class ImapStore extends Store { Log.d(Email.LOG_TAG, ioe.toString()); } throw ioe; + } finally { + destroyResponses(); } } @@ -1467,6 +1493,24 @@ public class ImapStore extends Store { } } + /** + * Create an {@link ImapResponseParser} from {@code mTransport.getInputStream()} and + * set it to {@link #mParser}. + * + * If we already have an {@link ImapResponseParser}, we + * {@link #destroyResponses()} and throw it away. + */ + private void createParser() { + destroyResponses(); + mParser = new ImapResponseParser(mTransport.getInputStream(), mDiscourse); + } + + public void destroyResponses() { + if (mParser != null) { + mParser.destroyResponses(); + } + } + /* package */ boolean isTransportOpenForTest() { return mTransport != null ? mTransport.isOpen() : false; } @@ -1508,8 +1552,10 @@ public class ImapStore extends Store { responses.add(response); } while (!response.isTagged()); if (!response.isOk()) { - throw new ImapException(response.toString(), - response.getAlertTextOrEmpty().getString()); + final String toString = response.toString(); + final String alert = response.getAlertTextOrEmpty().getString(); + destroyResponses(); + throw new ImapException(toString, alert); } return responses; } diff --git a/src/com/android/email/mail/store/imap/ImapResponseParser.java b/src/com/android/email/mail/store/imap/ImapResponseParser.java index ed37e9150..70969b4de 100644 --- a/src/com/android/email/mail/store/imap/ImapResponseParser.java +++ b/src/com/android/email/mail/store/imap/ImapResponseParser.java @@ -29,6 +29,7 @@ import android.util.Log; import java.io.IOException; import java.io.InputStream; +import java.util.ArrayList; /** * IMAP response parser. @@ -59,6 +60,12 @@ public class ImapResponseParser { /** StringBuilder used by parseBareString() */ private final StringBuilder mParseBareString = new StringBuilder(); + /** + * We store all {@link ImapResponse} in it. {@link #destroyResponses()} must be called from + * time to time to destroy them and clear it. + */ + private final ArrayList mResponsesToDestroy = new ArrayList(); + /** * Exception thrown when we receive BYE. It derives from IOException, so it'll be treated * in the same way EOF does. @@ -127,15 +134,31 @@ public class ImapResponseParser { return next; } + /** + * Destroy all the {@link ImapResponse}s stored in the internal storage and clear it. + * + * @see #readResponse() + */ + public void destroyResponses() { + for (ImapResponse r : mResponsesToDestroy) { + r.destroy(); + } + mResponsesToDestroy.clear(); + } + /** * Reads the next response available on the stream and returns an * {@link ImapResponse} object that represents it. * + *

When this method successfully returns an {@link ImapResponse}, the {@link ImapResponse} + * is stored in the internal storage. When the {@link ImapResponse} is no longer used + * {@link #destroyResponses} should be called to destroy all the responses in the array. + * * @return the parsed {@link ImapResponse} object. * @exception ByeException when detects BYE. */ public ImapResponse readResponse() throws IOException, MessagingException { - final ImapResponse response; + ImapResponse response = null; try { response = parseResponse(); if (Config.LOGD && Email.DEBUG) { @@ -155,8 +178,10 @@ public class ImapResponseParser { // Handle this outside of try-catch. We don't have to dump protocol log when getting BYE. if (response.is(0, ImapConstants.BYE)) { Log.w(Email.LOG_TAG, ByeException.MESSAGE); + response.destroy(); throw new ByeException(); } + mResponsesToDestroy.add(response); return response; } @@ -221,61 +246,85 @@ public class ImapResponseParser { * Parse and return the response line. */ private ImapResponse parseResponse() throws IOException, MessagingException { - final int ch = peek(); - if (ch == '+') { // Continuation request - readByte(); // skip + - expect(' '); - ImapResponse response = new ImapResponse(null, true); + // We need to destroy the response if we get an exception. + // So, we first store the response that's being built in responseToDestroy, until it's + // completely built, at which point we copy it into responseToReturn and null out + // responseToDestroyt. + // If responseToDestroy is not null in finally, we destroy it because that means + // we got an exception somewhere. + ImapResponse responseToDestroy = null; + final ImapResponse responseToReturn; - // If it's continuation request, we don't really care what's in it. - response.add(new ImapSimpleString(readUntilEol())); - return response; - } + try { + final int ch = peek(); + if (ch == '+') { // Continuation request + readByte(); // skip + + expect(' '); + responseToDestroy = new ImapResponse(null, true); - // Status response or response data - final String tag; - if (ch == '*') { - tag = null; - readByte(); // skip * - expect(' '); - } else { - tag = readUntil(' '); - } - final ImapResponse response = new ImapResponse(tag, false); + // If it's continuation request, we don't really care what's in it. + responseToDestroy.add(new ImapSimpleString(readUntilEol())); - final ImapString firstString = parseBareString(); - response.add(firstString); + // Response has successfully been built. Let's return it. + responseToReturn = responseToDestroy; + responseToDestroy = null; + } else { + // Status response or response data + final String tag; + if (ch == '*') { + tag = null; + readByte(); // skip * + expect(' '); + } else { + tag = readUntil(' '); + } + responseToDestroy = new ImapResponse(tag, false); - // parseBareString won't eat a space after the string, so we need to skip it, if exists. - // If the next char is not ' ', it should be EOL. - if (peek() == ' ') { - readByte(); // skip ' ' + final ImapString firstString = parseBareString(); + responseToDestroy.add(firstString); - if (response.isStatusResponse()) { // It's a status response + // parseBareString won't eat a space after the string, so we need to skip it, + // if exists. + // If the next char is not ' ', it should be EOL. + if (peek() == ' ') { + readByte(); // skip ' ' - // Is there a response code? - final int next = peek(); - if (next == '[') { - response.add(parseList('[', ']')); - if (peek() == ' ') { // Skip following space - readByte(); + if (responseToDestroy.isStatusResponse()) { // It's a status response + + // Is there a response code? + final int next = peek(); + if (next == '[') { + responseToDestroy.add(parseList('[', ']')); + if (peek() == ' ') { // Skip following space + readByte(); + } + } + + String rest = readUntilEol(); + if (!TextUtils.isEmpty(rest)) { + // The rest is free-form text. + responseToDestroy.add(new ImapSimpleString(rest)); + } + } else { // It's a response data. + parseElements(responseToDestroy, '\0'); } + } else { + expect('\r'); + expect('\n'); } - String rest = readUntilEol(); - if (!TextUtils.isEmpty(rest)) { - // The rest is free-form text. - response.add(new ImapSimpleString(rest)); - } - } else { // It's a response data. - parseElements(response, '\0'); + // Response has successfully been built. Let's return it. + responseToReturn = responseToDestroy; + responseToDestroy = null; + } + } finally { + if (responseToDestroy != null) { + // We get an exception. + responseToDestroy.destroy(); } - } else { - expect('\r'); - expect('\n'); } - return response; + return responseToReturn; } private ImapElement parseElement() throws IOException, MessagingException { diff --git a/src/com/android/email/mail/store/imap/ImapTempFileLiteral.java b/src/com/android/email/mail/store/imap/ImapTempFileLiteral.java index 96882e39d..ff14d44ab 100644 --- a/src/com/android/email/mail/store/imap/ImapTempFileLiteral.java +++ b/src/com/android/email/mail/store/imap/ImapTempFileLiteral.java @@ -58,9 +58,9 @@ public class ImapTempFileLiteral extends ImapString { } /** - * Because we can't use File.deleteOnExit(), let finalizer clean up the temp files.... + * Make sure we delete the temp file. * - * TODO: Don't rely on this. Always explicitly call destroy(). + * We should always be calling {@link ImapResponse#destroy()}, but it's here as a last resort. */ @Override protected void finalize() throws Throwable {