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;
  }
+ listeners = (IBufferChangedListener[]) cListeners.getListeners();
+ // make a copy before leaving synchronized block to make sure there is no concurrent modification:
+ listeners = Arrays.copyOf(listeners, listeners.length);
+ }
+
+ 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;