8325179: Race in BasicDirectoryModel.validateFileCache

8238169: BasicDirectoryModel getDirectories and DoChangeContents.run can deadlock

Reviewed-by: prr, tr, aturbanov, serb

Author: aivanov-jdk

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java

@@ -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();
+            }
         }
     }
 
@@ -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);
+        }
     }
 
     /**
@@ -276,7 +280,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) {
@@ -297,22 +300,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) {
@@ -339,7 +340,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();
@@ -395,7 +396,7 @@ public DoChangeContents call() {
                         }
                         if (!fileCache.equals(newFileCache)) {
                             if (loadThread.isInterrupted()) {
-                                cancelRunnables();
+                                return null;
                             }
                             return new DoChangeContents(newFileCache, 0, fileCache, 0, fid);
                         }
@@ -408,12 +409,6 @@ public DoChangeContents call() {
                 SwingUtilities.invokeLater(runnable);
             }
         }
-
-        private void cancelRunnables() {
-            if (runnable != null) {
-                runnable.cancel();
-            }
-        }
    }
 
 
@@ -522,45 +517,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();
             }
         }
     }