Skip to content

Commit

Permalink
8325179: Race in BasicDirectoryModel.validateFileCache
Browse files Browse the repository at this point in the history 
8238169: BasicDirectoryModel getDirectories and DoChangeContents.run can deadlock

Backport-of: e66788c16563d343f6cccd2807a251ccc6f9b64a
  • Loading branch information
@TheRealMDoerr
TheRealMDoerr committed Jun 14, 2024
1 parent f08e5e8 commit 2e4179b
Showing 1 changed file with 48 additions and 52 deletions.
100 changes: 48 additions & 52 deletions src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,13 @@ public void propertyChange(PropertyChangeEvent e) {
* This method is used to interrupt file loading thread.
*/
public void invalidateFileCache() {
if (filesLoader != null) {
filesLoader.loadThread.interrupt();
filesLoader.cancelRunnables();
filesLoader = null;
synchronized (this) {
if (filesLoader != null) {
filesLoader.loadThread.interrupt();
filesLoader = null;
// Increment fetch ID to invalidate pending DoChangeContents
fetchID.incrementAndGet();
}
}
}

Expand Down Expand Up @@ -156,14 +159,15 @@ public void validateFileCache() {
if (currentDirectory == null) {
return;
}
if (filesLoader != null) {
filesLoader.loadThread.interrupt();
filesLoader.cancelRunnables();
}
synchronized (this) {
if (filesLoader != null) {
filesLoader.loadThread.interrupt();
}

int fid = fetchID.incrementAndGet();
setBusy(true, fid);
filesLoader = new FilesLoader(currentDirectory, fid);
int fid = fetchID.incrementAndGet();
setBusy(true, fid);
filesLoader = new FilesLoader(currentDirectory, fid);
}
}

/**
Expand Down Expand Up @@ -270,7 +274,6 @@ private final class FilesLoader implements Runnable {
private final boolean fileSelectionEnabled;
private final int fid;
private final File currentDirectory;
private volatile DoChangeContents runnable;
private final Thread loadThread;

private FilesLoader(File currentDirectory, int fid) {
Expand All @@ -291,22 +294,20 @@ public void run() {
}

private void run0() {
FileSystemView fileSystem = fileSystemView;

if (loadThread.isInterrupted()) {
return;
}

File[] list = fileSystem.getFiles(currentDirectory, useFileHiding);
File[] list = fileSystemView.getFiles(currentDirectory, useFileHiding);

if (loadThread.isInterrupted()) {
return;
}

final Vector<File> newFileCache = new Vector<File>();
Vector<File> newFiles = new Vector<File>();
final Vector<File> newFiles = new Vector<File>();

// run through the file list, add directories and selectable files to fileCache
// Run through the file list, add directories and selectable files to fileCache
// Note that this block must be OUTSIDE of Invoker thread because of
// deadlock possibility with custom synchronized FileSystemView
for (File file : list) {
Expand All @@ -333,7 +334,7 @@ private void run0() {

// To avoid loads of synchronizations with Invoker and improve performance we
// execute the whole block on the COM thread
runnable = ShellFolder.invoke(new Callable<DoChangeContents>() {
DoChangeContents runnable = ShellFolder.invoke(new Callable<DoChangeContents>() {
public DoChangeContents call() {
synchronized (fileCache) {
int newSize = newFileCache.size();
Expand Down Expand Up @@ -389,7 +390,7 @@ public DoChangeContents call() {
}
if (!fileCache.equals(newFileCache)) {
if (loadThread.isInterrupted()) {
cancelRunnables();
return null;
}
return new DoChangeContents(newFileCache, 0, fileCache, 0, fid);
}
Expand All @@ -402,12 +403,6 @@ public DoChangeContents call() {
SwingUtilities.invokeLater(runnable);
}
}

private void cancelRunnables() {
if (runnable != null) {
runnable.cancel();
}
}
}


Expand Down Expand Up @@ -516,45 +511,46 @@ public void run() {
private final class DoChangeContents implements Runnable {
private final List<File> addFiles;
private final List<File> remFiles;
private boolean doFire = true;
private final int fid;
private int addStart = 0;
private int remStart = 0;
private final int addStart;
private final int remStart;

DoChangeContents(List<File> addFiles, int addStart, List<File> remFiles,
int remStart, int fid) {
private DoChangeContents(List<File> addFiles, int addStart,
List<File> remFiles, int remStart,
int fid) {
this.addFiles = addFiles;
this.addStart = addStart;
this.remFiles = remFiles;
this.remStart = remStart;
this.fid = fid;
}

synchronized void cancel() {
doFire = false;
}
@Override
public void run() {
if (fetchID.get() != fid) {
return;
}

public synchronized void run() {
if (fetchID.get() == fid && doFire) {
int remSize = (remFiles == null) ? 0 : remFiles.size();
int addSize = (addFiles == null) ? 0 : addFiles.size();
synchronized(fileCache) {
if (remSize > 0) {
fileCache.removeAll(remFiles);
}
if (addSize > 0) {
fileCache.addAll(addStart, addFiles);
}
files = null;
directories = null;
final int remSize = (remFiles == null) ? 0 : remFiles.size();
final int addSize = (addFiles == null) ? 0 : addFiles.size();
final int cacheSize;
synchronized (fileCache) {
if (remSize > 0) {
fileCache.removeAll(remFiles);
}
if (remSize > 0 && addSize == 0) {
fireIntervalRemoved(BasicDirectoryModel.this, remStart, remStart + remSize - 1);
} else if (addSize > 0 && remSize == 0 && addStart + addSize <= fileCache.size()) {
fireIntervalAdded(BasicDirectoryModel.this, addStart, addStart + addSize - 1);
} else {
fireContentsChanged();
if (addSize > 0) {
fileCache.addAll(addStart, addFiles);
}
files = null;
directories = null;
cacheSize = fileCache.size();
}
if (remSize > 0 && addSize == 0) {
fireIntervalRemoved(BasicDirectoryModel.this, remStart, remStart + remSize - 1);
} else if (addSize > 0 && remSize == 0 && addStart + addSize <= cacheSize) {
fireIntervalAdded(BasicDirectoryModel.this, addStart, addStart + addSize - 1);
} else {
fireContentsChanged();
}
}
}
Expand Down

1 comment on commit 2e4179b

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.