Skip to content

Commit

Permalink
QuestionFragment.ClassificationInProgress: Do some defensive copying.
Browse files Browse the repository at this point in the history
Return copies of lists and copy lists in constructors.
This is to avoid concurrent changes when, for instance, the answers
List changes while we are iterating over it. It's not clear when that
might happen, but we call getAnswers() from AsyncTask(), so it seems
possible. There is at least one report of a ConcurrentModificationException
so this might help:
#15
  • Loading branch information
murraycu committed Jan 16, 2015
1 parent fcc4e59 commit b938717
Showing 1 changed file with 38 additions and 5 deletions.
43 changes: 38 additions & 5 deletions app/src/main/java/com/murrayc/galaxyzoo/app/QuestionFragment.java
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ static interface Callbacks {
* way, but this lets us avoid having half-complete classifications
* in the content provider.
*/
public static class ClassificationInProgress implements Parcelable {
public static final class ClassificationInProgress implements Parcelable {
public static final Parcelable.Creator<ClassificationInProgress> CREATOR
= new Parcelable.Creator<ClassificationInProgress>() {
public ClassificationInProgress createFromParcel(final Parcel in) {
Expand Down Expand Up @@ -940,8 +940,18 @@ public void writeToParcel(final Parcel dest, final int flags) {
dest.writeInt(favorite ? 1 : 0);
}

/** Returns a deep copy of the list of answers,
* to avoid any chance of concurrent use.
*
* @return
*/
List<QuestionAnswer> getAnswers() {
return answers;
final List<QuestionAnswer> result = new ArrayList<>();
for(final QuestionAnswer answer : answers) {
result.add(new QuestionAnswer(answer));
}

return new ArrayList<>(answers);
}

boolean isFavorite() {
Expand Down Expand Up @@ -974,17 +984,35 @@ public QuestionAnswer[] newArray(final int size) {
private final List<String> checkboxIds;

public QuestionAnswer(final String questionId, final String answerId, final List<String> checkboxIds) {
//Strings are immutable so we don't need to copy them:
this.questionId = questionId;
this.answerId = answerId;
this.checkboxIds = checkboxIds;

this.checkboxIds = deepCopyCheckBoxIds(checkboxIds);
}

private List<String> deepCopyCheckBoxIds(final List<String> strList) {
final List<String> result = new ArrayList<>();

//Strings are immutable so we don't need to copy them:
result.addAll(strList);

return result;
}

private QuestionAnswer(final Parcel in) {
//Keep this in sync with writeToParcel().
this.questionId = in.readString();
this.answerId = in.readString();

this.checkboxIds = in.createStringArrayList();
this.checkboxIds = deepCopyCheckBoxIds(in.createStringArrayList());
}

public QuestionAnswer(final QuestionAnswer answer) {
this.questionId = new String(answer.getQuestionId());
this.answerId = new String(answer.getAnswerId());

this.checkboxIds = deepCopyCheckBoxIds(answer.checkboxIds);
}

@Override
Expand Down Expand Up @@ -1025,8 +1053,13 @@ public String getAnswerId() {
return answerId;
}

/** Returns a deep copy of the list of checkbox IDs,
* to avoid any chance of concurrent use.
*
* @return
*/
public List<String> getCheckboxIds() {
return checkboxIds;
return deepCopyCheckBoxIds(checkboxIds);
}

@Override
Expand Down

0 comments on commit b938717

Please sign in to comment.