Fix potential crash in MessageViewFragment.openMessageSync
The problem is that ths method is called in a worker thread, so there's nothing to prevent it from running just after/at the same time as clearContent() (which sets -1 to mMessageIdToOpen). If it does, it passes -1 to restoreMessageWithId() and crashes. Also removed a half-obsolete comment which is a bit too obvious for its length. Bug 3077387 Change-Id: I736d696046e6d8964a16c80515544c582aca3943
This commit is contained in:
parent
9c293eb65a
commit
09eb977c06
|
@ -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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue