Merge "Fix potential crash in MessageViewFragment.openMessageSync"

src/com/android/email/activity/MessageFileViewFragment.java

@@ -35,7 +35,14 @@ import java.security.InvalidParameterException;
  * See {@link MessageViewBase} for the class relation diagram.
  */
 public class MessageFileViewFragment extends MessageViewFragmentBase {
+    /**
+     * URI of message to open.  Protect with {@link #mLock}.
+     */
     private Uri mFileEmailUri;
+
+    /** Lock object to protect {@link #mFileEmailUri} */
+    private final Object mLock = new Object();
+
     /**
      * # of instances of this class.  When it gets 0, and the last one is not destroying for
      * a config change, we delete all the EML files.
@@ -78,33 +85,48 @@ public class MessageFileViewFragment extends MessageViewFragmentBase {
         if (fileEmailUri == null) {
             throw new InvalidParameterException();
         }
-        mFileEmailUri = fileEmailUri;
+        synchronized (mLock) {
+            mFileEmailUri = fileEmailUri;
+        }
         openMessageIfStarted();
     }
 
     @Override
     public void clearContent() {
-        super.clearContent();
-        mFileEmailUri = null;
+        synchronized (mLock) {
+            super.clearContent();
+            mFileEmailUri = null;
+        }
     }
 
     @Override
     protected boolean isMessageSpecified() {
-        return mFileEmailUri != null;
+        synchronized (mLock) {
+            return mFileEmailUri != null;
+        }
     }
 
+    /**
+     * NOTE See the comment on the super method.  It's called on a worker thread.
+     */
     @Override
     protected Message openMessageSync() {
-        final Activity activity = getActivity();
-        // Put up a toast; this can take a little while...
-        Utility.showToast(activity, R.string.message_view_parse_message_toast);
-        Message msg = getController().loadMessageFromUri(mFileEmailUri);
-        if (msg == null) {
-            // Indicate that the attachment couldn't be loaded
-            Utility.showToast(activity, R.string.message_view_display_attachment_toast);
-            return null;
+        synchronized (mLock) {
+            final Activity activity = getActivity();
+            Uri messageUri = mFileEmailUri;
+            if (messageUri == null) {
+                return null; // Called after clearContent().
+            }
+            // Put up a toast; this can take a little while...
+            Utility.showToast(activity, R.string.message_view_parse_message_toast);
+            Message msg = getController().loadMessageFromUri(messageUri);
+            if (msg == null) {
+                // Indicate that the attachment couldn't be loaded
+                Utility.showToast(activity, R.string.message_view_display_attachment_toast);
+                return null;
+            }
+            return msg;
         }
-        return msg;
     }
 
     /**

src/com/android/email/activity/MessageViewFragment.java

@@ -44,7 +44,6 @@ import java.security.InvalidParameterException;
  * See {@link MessageViewBase} for the class relation diagram.
  */
 public class MessageViewFragment extends MessageViewFragmentBase {
-
     private ImageView mFavoriteIcon;
     private View mInviteSection;
 
@@ -62,9 +61,14 @@ public class MessageViewFragment extends MessageViewFragmentBase {
     private Drawable mFavoriteIconOn;
     private Drawable mFavoriteIconOff;
 
-    /** ID of the message that will be loaded */
+    /**
+     * ID of the message that will be loaded.  Protect with {@link #mLock}.
+     */
     private long mMessageIdToOpen = -1;
 
+    /** Lock object to protect {@link #mMessageIdToOpen} */
+    private final Object mLock = new Object();
+
     /** ID of the currently shown message */
     private long mCurrentMessageId = -1;
 
@@ -205,14 +209,18 @@ public class MessageViewFragment extends MessageViewFragmentBase {
         if (messageId == -1) {
             throw new InvalidParameterException();
         }
-        mMessageIdToOpen = messageId;
+        synchronized (mLock) {
+            mMessageIdToOpen = messageId;
+        }
         openMessageIfStarted();
     }
 
     @Override
     public void clearContent() {
-        super.clearContent();
-        mMessageIdToOpen = -1;
+        synchronized (mLock) {
+            super.clearContent();
+            mMessageIdToOpen = -1;
+        }
     }
 
     @Override
@@ -223,12 +231,23 @@ public class MessageViewFragment extends MessageViewFragmentBase {
 
     @Override
     protected boolean isMessageSpecified() {
-        return mMessageIdToOpen != -1;
+        synchronized (mLock) {
+            return mMessageIdToOpen != -1;
+        }
     }
 
+    /**
+     * NOTE See the comment on the super method.  It's called on a worker thread.
+     */
     @Override
     protected Message openMessageSync() {
-        return Message.restoreMessageWithId(getActivity(), mMessageIdToOpen);
+        synchronized (mLock) {
+            long messageId = mMessageIdToOpen;
+            if (messageId < 0) {
+                return null; // Called after clearContent().
+            }
+            return Message.restoreMessageWithId(getActivity(), messageId);
+        }
     }
 
     @Override

src/com/android/email/activity/MessageViewFragmentBase.java

@@ -769,8 +769,12 @@ public abstract class MessageViewFragmentBase extends Fragment implements View.O
     }
 
     /**
-     * Called on a worker thread by {@link LoadMessageTask} to load a message in a subclass specific
-     * way.
+     * Called by {@link LoadMessageTask} and {@link ReloadMessageTask} to load a message in a
+     * subclass specific way.
+     *
+     * NOTE This method is called on a worker thread!  Implementations must properly synchronize
+     * when accessing members.  This method may be called after or even at the same time as
+     * {@link #clearContent()}.
      */
     protected abstract Message openMessageSync();
 
@@ -803,16 +807,6 @@ public abstract class MessageViewFragmentBase extends Fragment implements View.O
 
         @Override
         protected void onPostExecute(Message message) {
-            /* doInBackground() may return null result (due to restoreMessageWithId())
-             * and in that situation we want to Activity.finish().
-             *
-             * OTOH we don't want to Activity.finish() for isCancelled() because this
-             * would introduce a surprise side-effect to task cancellation: every task
-             * cancelation would also result in finish().
-             *
-             * Right now LoadMesageTask is cancelled not only from onDestroy(),
-             * and it would be a bug to also finish() the activity in that situation.
-             */
             if (isCancelled()) {
                 return;
             }