From a882ee595b4a9a388c40ee49d50748b21fd9ec6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Kubitz?= Date: Tue, 18 Jun 2024 16:09:06 +0200 Subject: [PATCH] org.eclipse.jdt.internal.core.Buffer: threadsafe Some field accesses had not been synchronized which may have caused NPEs in access from different thread. https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/736 --- .../org/eclipse/jdt/internal/core/Buffer.java | 98 ++++++++++++------- .../jdt/internal/core/JavaModelManager.java | 4 +- 2 files changed, 63 insertions(+), 39 deletions(-) diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/Buffer.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/Buffer.java index 2bf95bc2264..bd86be5da17 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/Buffer.java +++ b/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 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 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 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 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 Buffer 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 diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java index 9819c904b6c..02845a52b4a 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/JavaModelManager.java +++ b/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;