org.eclipse.jdt.internal.core.Buffer: threadsafe

Some field accesses had not been synchronized which may have caused NPEs
in access from different thread.

eclipse-jdt/eclipse.jdt.ui#736

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/Buffer.java

@@ -16,6 +16,7 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.util.Arrays;
 
 import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IResource;
@@ -34,19 +35,24 @@
  */
 @SuppressWarnings({ "unchecked", "rawtypes" })
 public class Buffer implements IBuffer {
-	protected final IFile file;
-	protected int flags;
-	protected char[] contents;
-	protected ListenerList<IBufferChangedListener> changeListeners;
-	protected final IOpenable owner;
-	protected int gapStart = -1;
-	protected int gapEnd = -1;
+	private final IFile file;
+	/** synchronized by this.lock **/
+	private int flags;
+	/** synchronized by this.lock **/
+	private char[] contents;
+	/** synchronized by Buffer.this **/
+	private ListenerList<IBufferChangedListener> changeListeners;
+	private final IOpenable owner;
+	/** synchronized by this.lock **/
+	private int gapStart = -1;
+	/** synchronized by this.lock **/
+	private int gapEnd = -1;
 
-	protected Object lock = new Object();
+	private final Object lock = new Object();
 
-	protected static final int F_HAS_UNSAVED_CHANGES = 1;
-	protected static final int F_IS_READ_ONLY = 2;
-	protected static final int F_IS_CLOSED = 4;
+	private static final int F_HAS_UNSAVED_CHANGES = 1;
+	private static final int F_IS_READ_ONLY = 2;
+	private static final int F_IS_CLOSED = 4;
 
 /**
  * Creates a new buffer on an underlying resource.
@@ -207,21 +213,27 @@ public IResource getUnderlyingResource() {
  */
 @Override
 public boolean hasUnsavedChanges() {
-	return (this.flags & F_HAS_UNSAVED_CHANGES) != 0;
+	synchronized(this.lock) {
+		return (this.flags & F_HAS_UNSAVED_CHANGES) != 0;
+	}
 }
 /**
  * @see IBuffer
  */
 @Override
 public boolean isClosed() {
-	return (this.flags & F_IS_CLOSED) != 0;
+	synchronized(this.lock) {
+		return (this.flags & F_IS_CLOSED) != 0;
+	}
 }
 /**
  * @see IBuffer
  */
 @Override
 public boolean isReadOnly() {
-	return (this.flags & F_IS_READ_ONLY) != 0;
+	synchronized(this.lock) {
+		return (this.flags & F_IS_READ_ONLY) != 0;
+	}
 }
 /**
  * Moves the gap to location and adjust its size to the
@@ -270,20 +282,28 @@ protected void moveAndResizeGap(int position, int size) {
  * To avoid deadlock, this should not be called in a synchronized block.
  */
 protected void notifyChanged(final BufferChangedEvent event) {
-	ListenerList<IBufferChangedListener> listeners = this.changeListeners;
-	if (listeners != null) {
-		for (IBufferChangedListener listener : listeners) {
-			SafeRunner.run(new ISafeRunnable() {
-				@Override
-				public void handleException(Throwable exception) {
-					Util.log(exception, "Exception occurred in listener of buffer change notification"); //$NON-NLS-1$
-				}
-				@Override
-				public void run() throws Exception {
-					listener.bufferChanged(event);
-				}
-			});
+	IBufferChangedListener[] listeners;
+	synchronized(this) {
+		ListenerList<IBufferChangedListener> cListeners = this.changeListeners;
+		if (cListeners == null) {
+			return;
 		}
+		Object[] l =  cListeners.getListeners();
+		// make a copy before leaving synchronized block to make sure there is no concurrent modification:
+		listeners = Arrays.copyOf(l, l.length, IBufferChangedListener[].class);
+	}
+
+	for (IBufferChangedListener listener : listeners) {
+		SafeRunner.run(new ISafeRunnable() {
+			@Override
+			public void handleException(Throwable exception) {
+				Util.log(exception, "Exception occurred in listener of buffer change notification"); //$NON-NLS-1$
+			}
+			@Override
+			public void run() throws Exception {
+				listener.bufferChanged(event);
+			}
+		});
 	}
 }
 /**
@@ -414,7 +434,9 @@ public void save(IProgressMonitor progress, boolean force) throws JavaModelExcep
 	}
 
 	// the resource no longer has unsaved changes
-	this.flags &= ~ (F_HAS_UNSAVED_CHANGES);
+	synchronized(this.lock) {
+		this.flags &= ~ (F_HAS_UNSAVED_CHANGES);
+	}
 }
 /**
  * @see IBuffer
@@ -423,12 +445,12 @@ public void save(IProgressMonitor progress, boolean force) throws JavaModelExcep
 public void setContents(char[] newContents) {
 	// allow special case for first initialization
 	// after creation by buffer factory
-	if (this.contents == null) {
-		synchronized (this.lock) {
-			this.contents = newContents;
-			this.flags &= ~ (F_HAS_UNSAVED_CHANGES);
+	synchronized (this.lock) {
+		if (this.contents == null) {
+				this.contents = newContents;
+				this.flags &= ~ (F_HAS_UNSAVED_CHANGES);
+			return;
 		}
-		return;
 	}
 
 	if (!isReadOnly()) {
@@ -458,10 +480,12 @@ public void setContents(String newContents) {
  * Sets this <code>Buffer</code> to be read only.
  */
 protected void setReadOnly(boolean readOnly) {
-	if (readOnly) {
-		this.flags |= F_IS_READ_ONLY;
-	} else {
-		this.flags &= ~(F_IS_READ_ONLY);
+	synchronized(this.lock) {
+		if (readOnly) {
+			this.flags |= F_IS_READ_ONLY;
+		} else {
+			this.flags &= ~(F_IS_READ_ONLY);
+		}
 	}
 }
 @Override

org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java

@@ -1624,8 +1624,8 @@ public boolean writeAndCacheClasspath(JavaProject javaProject, final IClasspathE
 
 	public static class PerWorkingCopyInfo implements IProblemRequestor {
 		int useCount = 0;
-		IProblemRequestor problemRequestor;
-		CompilationUnit workingCopy;
+		private final IProblemRequestor problemRequestor;
+		final CompilationUnit workingCopy;
 		public PerWorkingCopyInfo(CompilationUnit workingCopy, IProblemRequestor problemRequestor) {
 			this.workingCopy = workingCopy;
 			this.problemRequestor = problemRequestor;