From e66788c16563d343f6cccd2807a251ccc6f9b64a Mon Sep 17 00:00:00 2001 From: Alexey Ivanov Date: Thu, 21 Mar 2024 16:03:30 +0000 Subject: [PATCH] 8325179: Race in BasicDirectoryModel.validateFileCache 8238169: BasicDirectoryModel getDirectories and DoChangeContents.run can deadlock Reviewed-by: prr, tr, aturbanov, serb --- .../swing/plaf/basic/BasicDirectoryModel.java | 100 +++++++++--------- 1 file changed, 48 insertions(+), 52 deletions(-) diff --git a/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java b/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java index 7ae6bd727a3..6f0e678dcd6 100644 --- a/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java +++ b/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java @@ -98,10 +98,13 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr * 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(); + } } } @@ -156,14 +159,15 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr 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); + } } /** @@ -276,7 +280,6 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr 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) { @@ -297,22 +300,20 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr } 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 newFileCache = new Vector(); - Vector newFiles = new Vector(); + final Vector newFiles = new Vector(); - // 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) { @@ -339,7 +340,7 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr // 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 runnable = ShellFolder.invoke(new Callable() { public DoChangeContents call() { synchronized (fileCache) { int newSize = newFileCache.size(); @@ -395,7 +396,7 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr } if (!fileCache.equals(newFileCache)) { if (loadThread.isInterrupted()) { - cancelRunnables(); + return null; } return new DoChangeContents(newFileCache, 0, fileCache, 0, fid); } @@ -408,12 +409,6 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr SwingUtilities.invokeLater(runnable); } } - - private void cancelRunnables() { - if (runnable != null) { - runnable.cancel(); - } - } } @@ -522,13 +517,13 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr private final class DoChangeContents implements Runnable { private final List addFiles; private final List 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 addFiles, int addStart, List remFiles, - int remStart, int fid) { + private DoChangeContents(List addFiles, int addStart, + List remFiles, int remStart, + int fid) { this.addFiles = addFiles; this.addStart = addStart; this.remFiles = remFiles; @@ -536,31 +531,32 @@ public class BasicDirectoryModel extends AbstractListModel implements Pr 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(); } } }