Bug 401391 - ConcurrentModificationException in AnnotationModelI20200319-1800

Use a ConcurrentHashMap for the AnnotationModel attachments to avoid
a concurrent modification when the AnnotationIterator is created

Change-Id: I3ceeebea8cd7c894fe70adae2378db59c2ac1f1b
Signed-off-by: Sebastian Zarnekow <sebastian.zarnekow@gmail.com>

5 files changed, 172 insertions, 6 deletions

diff --git a/org.eclipse.text.tests/META-INF/MANIFEST.MF b/org.eclipse.text.tests/META-INF/MANIFEST.MF
index 5b946692fd7..69265e59593 100644
--- a/org.eclipse.text.tests/META-INF/MANIFEST.MF
+++ b/org.eclipse.text.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Plugin.name
 Bundle-SymbolicName: org.eclipse.text.tests
-Bundle-Version: 3.12.500.qualifier
+Bundle-Version: 3.12.600.qualifier
 Bundle-Vendor: %Plugin.providerName
 Bundle-Localization: plugin
 Export-Package: 
diff --git a/org.eclipse.text.tests/pom.xml b/org.eclipse.text.tests/pom.xml
index 456f3b6267b..901321daf1b 100644
--- a/org.eclipse.text.tests/pom.xml
+++ b/org.eclipse.text.tests/pom.xml
@@ -19,7 +19,7 @@
   </parent>
   <groupId>org.eclipse.text</groupId>
   <artifactId>org.eclipse.text.tests</artifactId>
-  <version>3.12.500-SNAPSHOT</version>
+  <version>3.12.600-SNAPSHOT</version>
   <packaging>eclipse-test-plugin</packaging>
   <properties>
   	<testSuite>${project.artifactId}</testSuite>
diff --git a/org.eclipse.text.tests/src/org/eclipse/text/tests/Bug401391Test.java b/org.eclipse.text.tests/src/org/eclipse/text/tests/Bug401391Test.java
new file mode 100644
index 00000000000..942abb2941f
--- /dev/null
+++ b/org.eclipse.text.tests/src/org/eclipse/text/tests/Bug401391Test.java
@@ -0,0 +1,165 @@
+/*******************************************************************************
+ * Copyright (c) 2020 Sebastian Zarnekow and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     Sebastian Zarnekow - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.text.tests;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+
+import java.lang.reflect.Field;
+import java.util.AbstractMap;
+import java.util.AbstractSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Set;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.eclipse.jface.text.Document;
+import org.eclipse.jface.text.Position;
+import org.eclipse.jface.text.source.Annotation;
+import org.eclipse.jface.text.source.AnnotationModel;
+
+public class Bug401391Test {
+
+	private Document fDocument;
+	private AnnotationModel fAnnotationModel;
+	private AnnotationModel fFirstInnerModel;
+	private AnnotationModel fSecondInnerModel;
+
+	@Before
+	public void setUp() throws Exception {
+		fDocument = new Document("123456789");
+
+		fAnnotationModel = new AnnotationModel();
+		fAnnotationModel.addAnnotation(new Annotation(false), new Position(0, 1));
+
+		fFirstInnerModel = new AnnotationModel();
+		fFirstInnerModel.addAnnotation(new Annotation(false), new Position(1, 2));
+		fAnnotationModel.addAnnotationModel("first", fFirstInnerModel);
+
+		fAnnotationModel.connect(fDocument);
+	}
+
+	/*
+	 * The test simulates a concurrent modification of the attachments by installing
+	 * a trap into the annotation model that will modify the available attachments
+	 * when the attachments are iterated.
+	 */
+	private void installAttachmentTrap() throws Exception {
+		installTrap(fAnnotationModel, "fAttachments", () -> {
+			if (fSecondInnerModel == null) {
+				fSecondInnerModel = new AnnotationModel();
+				fSecondInnerModel.addAnnotation(new Annotation(false), new Position(3, 2));
+				fAnnotationModel.addAnnotationModel("second", fSecondInnerModel);
+			} else {
+				fAnnotationModel.removeAnnotationModel("second");
+			}
+		});
+	}
+
+	private static void installTrap(Object target, String fieldName, Runnable onKeySetIteration) throws Exception {
+		Field fld = target.getClass().getDeclaredField(fieldName);
+		fld.setAccessible(true);
+		@SuppressWarnings("unchecked")
+		Map<Object, Object> original = (Map<Object, Object>) fld.get(target);
+		class TrapMap extends AbstractMap<Object, Object> {
+			@Override
+			public Set<Object> keySet() {
+				Set<Object> delegate = original.keySet();
+
+				return new AbstractSet<Object>() {
+
+					@Override
+					public Iterator<Object> iterator() {
+						Iterator<Object> result = delegate.iterator();
+						onKeySetIteration.run();
+						return result;
+					}
+
+					@Override
+					public int size() {
+						return delegate.size();
+					}
+				};
+			}
+
+			@Override
+			public Set<Entry<Object, Object>> entrySet() {
+				return original.entrySet();
+			}
+
+			@Override
+			public Object put(Object key, Object value) {
+				return original.put(key, value);
+			}
+		}
+		fld.set(target, new TrapMap());
+	}
+
+	@After
+	public void tearDown() {
+		fAnnotationModel.disconnect(fDocument);
+	}
+
+	@Test
+	public void testNoConcurrentModificationOnAddAttachment() throws Exception {
+		installAttachmentTrap();
+		assertNull(fAnnotationModel.getAnnotationModel("second"));
+		Iterator<Annotation> iter = fAnnotationModel.getAnnotationIterator();
+		assertNotNull(fAnnotationModel.getAnnotationModel("second"));
+		assertEquals(3, count(iter));
+	}
+
+	@Test
+	public void testNoConcurrentModificationOnRemoveAttachment() throws Exception {
+		installAttachmentTrap();
+		assertNull(fAnnotationModel.getAnnotationModel("second"));
+		fAnnotationModel.getAnnotationIterator();
+		assertNotNull(fAnnotationModel.getAnnotationModel("second"));
+		Iterator<Annotation> iter = fAnnotationModel.getAnnotationIterator();
+		assertNull(fAnnotationModel.getAnnotationModel("second"));
+		assertEquals(2, count(iter));
+	}
+
+	@Test
+	public void testRemoveAnnotationWhileIterating() throws Exception {
+		Annotation removeWhileIterating = new Annotation(false);
+		fFirstInnerModel.addAnnotation(removeWhileIterating, new Position(5, 2));
+		Iterator<Annotation> iter = fAnnotationModel.getAnnotationIterator();
+		assertTrue(iter.hasNext());
+		iter.next();
+		assertTrue(iter.hasNext());
+		iter.next();
+		fFirstInnerModel.removeAnnotation(removeWhileIterating);
+		assertEquals(2, count(fAnnotationModel.getAnnotationIterator()));
+		assertTrue(iter.hasNext());
+		// Traverses a copy of the model at the time the iterator was created
+		assertSame(removeWhileIterating, iter.next());
+	}
+
+	private int count(Iterator<?> iter) {
+		int result = 0;
+		while (iter.hasNext()) {
+			result++;
+			iter.next();
+		}
+		return result;
+	}
+
+}
diff --git a/org.eclipse.text/META-INF/MANIFEST.MF b/org.eclipse.text/META-INF/MANIFEST.MF
index a7adeb868ce..1a693ade35f 100644
--- a/org.eclipse.text/META-INF/MANIFEST.MF
+++ b/org.eclipse.text/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.text
-Bundle-Version: 3.10.100.qualifier
+Bundle-Version: 3.10.200.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Export-Package: 
diff --git a/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java b/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java
index a12f1935638..d32e0004c69 100644
--- a/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java
+++ b/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2015 IBM Corporation and others.
+ * Copyright (c) 2000, 2020 IBM Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -11,18 +11,19 @@
  * Contributors:
  *     IBM Corporation - initial API and implementation
  *     Anton Leherbauer <anton.leherbauer@windriver.com> - [implementation] AnnotationModel.fModificationStamp leaks annotations - http://bugs.eclipse.org/345715
+ *     Sebastian Zarnekow - [bug 401391] ConcurrentModificationException in AnnotationModel.getAnnotationIterator
  *******************************************************************************/
 package org.eclipse.jface.text.source;
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.IdentityHashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.NoSuchElementException;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.eclipse.core.runtime.Assert;
 
@@ -281,7 +282,7 @@ public class AnnotationModel implements IAnnotationModel, IAnnotationModelExtens
 	 * The model's attachment.
 	 * @since 3.0
 	 */
-	private Map<Object, IAnnotationModel> fAttachments= new HashMap<>();
+	private Map<Object, IAnnotationModel> fAttachments= new ConcurrentHashMap<>();
 	/**
 	 * The annotation model listener on attached sub-models.
 	 * @since 3.0