6713352: Deadlock in JFileChooser with synchronized custom FileSystemView

Reviewed-by: malenkov, peterz

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

@@ -230,51 +230,46 @@ public class BasicDirectoryModel extends AbstractListModel implements PropertyCh
         }
 
         public void run0() {
-            DoChangeContents doChangeContents = ShellFolder.getInvoker().invoke(new Callable<DoChangeContents>() {
-                public DoChangeContents call() throws Exception {
-                    FileSystemView fileSystem = filechooser.getFileSystemView();
+            FileSystemView fileSystem = filechooser.getFileSystemView();
 
-                    File[] list = fileSystem.getFiles(currentDirectory, filechooser.isFileHidingEnabled());
+            File[] list = fileSystem.getFiles(currentDirectory, filechooser.isFileHidingEnabled());
 
-                    Vector<File> acceptsList = new Vector<File>();
+            if (isInterrupted()) {
+                return;
+            }
 
-                    if (isInterrupted()) {
-                        return null;
-                    }
+            final Vector<File> newFileCache = new Vector<File>();
+            Vector<File> newFiles = new Vector<File>();
 
-                    // run through the file list, add directories and selectable files to fileCache
-                    for (File file : list) {
-                        if (filechooser.accept(file)) {
-                            acceptsList.addElement(file);
-                        }
+            // 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) {
+                if (filechooser.accept(file)) {
+                    boolean isTraversable = filechooser.isTraversable(file);
+
+                    if (isTraversable) {
+                        newFileCache.addElement(file);
+                    } else if (filechooser.isFileSelectionEnabled()) {
+                        newFiles.addElement(file);
                     }
 
                     if (isInterrupted()) {
-                        return null;
+                        return;
                     }
+                }
+            }
 
-                    // First sort alphabetically by filename
-                    sort(acceptsList);
-
-                    Vector<File> newDirectories = new Vector<File>(50);
-                    Vector<File> newFiles = new Vector<File>();
-                    // run through list grabbing directories in chunks of ten
-                    for (int i = 0; i < acceptsList.size(); i++) {
-                        File f = acceptsList.elementAt(i);
-                        boolean isTraversable = filechooser.isTraversable(f);
-                        if (isTraversable) {
-                            newDirectories.addElement(f);
-                        } else if (!isTraversable && filechooser.isFileSelectionEnabled()) {
-                            newFiles.addElement(f);
-                        }
-                        if (isInterrupted()) {
-                            return null;
-                        }
-                    }
+            // First sort alphabetically by filename
+            sort(newFileCache);
+            sort(newFiles);
 
-                    Vector<File> newFileCache = new Vector<File>(newDirectories);
-                    newFileCache.addAll(newFiles);
+            newFileCache.addAll(newFiles);
 
+            // To avoid loads of synchronizations with Invoker and improve performance we
+            // execute the whole block on the COM thread
+            DoChangeContents doChangeContents = ShellFolder.getInvoker().invoke(new Callable<DoChangeContents>() {
+                public DoChangeContents call() throws Exception {
                     int newSize = newFileCache.size();
                     int oldSize = fileCache.size();
 

src/share/classes/sun/awt/shell/ShellFolder.java

@@ -274,45 +274,61 @@ public abstract class ShellFolder extends File {
 
     // Override File methods
 
-    public static void sort(List<? extends File> files) {
+    public static void sort(final List<? extends File> files) {
         if (files == null || files.size() <= 1) {
             return;
         }
 
-        // Check that we can use the ShellFolder.sortChildren() method:
-        //   1. All files have the same non-null parent
-        //   2. All files is ShellFolders
-        File commonParent = null;
-
-        for (File file : files) {
-            File parent = file.getParentFile();
+        // To avoid loads of synchronizations with Invoker and improve performance we
+        // synchronize the whole code of the sort method once
+        getInvoker().invoke(new Callable<Void>() {
+            public Void call() throws Exception {
+                // Check that we can use the ShellFolder.sortChildren() method:
+                //   1. All files have the same non-null parent
+                //   2. All files is ShellFolders
+                File commonParent = null;
+
+                for (File file : files) {
+                    File parent = file.getParentFile();
+
+                    if (parent == null || !(file instanceof ShellFolder)) {
+                        commonParent = null;
+
+                        break;
+                    }
+
+                    if (commonParent == null) {
+                        commonParent = parent;
+                    } else {
+                        if (commonParent != parent && !commonParent.equals(parent)) {
+                            commonParent = null;
+
+                            break;
+                        }
+                    }
+                }
 
-            if (parent == null || !(file instanceof ShellFolder)) {
-                commonParent = null;
+                if (commonParent instanceof ShellFolder) {
+                    ((ShellFolder) commonParent).sortChildren(files);
+                } else {
+                    Collections.sort(files, FILE_COMPARATOR);
+                }
 
-                break;
+                return null;
             }
+        });
+    }
 
-            if (commonParent == null) {
-                commonParent = parent;
-            } else {
-                if (commonParent != parent && !commonParent.equals(parent)) {
-                    commonParent = null;
+    public void sortChildren(final List<? extends File> files) {
+        // To avoid loads of synchronizations with Invoker and improve performance we
+        // synchronize the whole code of the sort method once
+        getInvoker().invoke(new Callable<Void>() {
+            public Void call() throws Exception {
+                Collections.sort(files, FILE_COMPARATOR);
 
-                    break;
-                }
+                return null;
             }
-        }
-
-        if (commonParent instanceof ShellFolder) {
-            ((ShellFolder) commonParent).sortChildren(files);
-        } else {
-            Collections.sort(files, FILE_COMPARATOR);
-        }
-    }
-
-    public void sortChildren(List<? extends File> files) {
-        Collections.sort(files, FILE_COMPARATOR);
+        });
     }
 
     public boolean isAbsolute() {

src/windows/classes/sun/awt/shell/Win32ShellFolder2.java

@@ -527,7 +527,7 @@ final class Win32ShellFolder2 extends ShellFolder {
     /**
      * @return Whether this is a file system shell folder
      */
-    public synchronized boolean isFileSystem() {
+    public boolean isFileSystem() {
         if (cachedIsFileSystem == null) {
             cachedIsFileSystem = hasAttribute(ATTRIB_FILESYSTEM);
         }
@@ -543,8 +543,8 @@ final class Win32ShellFolder2 extends ShellFolder {
             public Boolean call() throws Exception {
                 // Caching at this point doesn't seem to be cost efficient
                 return (getAttributes0(getParentIShellFolder(),
-                        getRelativePIDL(), attribute)
-                        & attribute) != 0;
+                    getRelativePIDL(), attribute)
+                    & attribute) != 0;
             }
         });
     }
@@ -761,7 +761,7 @@ final class Win32ShellFolder2 extends ShellFolder {
     /**
      * @return Whether this shell folder is a link
      */
-    public synchronized boolean isLink() {
+    public boolean isLink() {
         if (cachedIsLink == null) {
             cachedIsLink = hasAttribute(ATTRIB_LINK);
         }
@@ -1160,8 +1160,16 @@ final class Win32ShellFolder2 extends ShellFolder {
     private static native int compareIDsByColumn(long pParentIShellFolder, long pidl1, long pidl2, int columnIdx);
 
 
-    public void sortChildren(List<? extends File> files) {
-        Collections.sort(files, new ColumnComparator(getIShellFolder(), 0));
+    public void sortChildren(final List<? extends File> files) {
+        // To avoid loads of synchronizations with Invoker and improve performance we
+        // synchronize the whole code of the sort method once
+        getInvoker().invoke(new Callable<Void>() {
+            public Void call() throws Exception {
+                Collections.sort(files, new ColumnComparator(getIShellFolder(), 0));
+
+                return null;
+            }
+        });
     }
 
     private static class ColumnComparator implements Comparator<File> {

src/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java

@@ -478,21 +478,22 @@ public class Win32ShellFolderManager2 extends ShellFolderManager {
 
         public <T> T invoke(Callable<T> task) {
             try {
-                T result;
                 if (Thread.currentThread() == comThread) {
                     // if it's already called from the COM
                     // thread, we don't need to delegate the task
-                    result = task.call();
+                    return task.call();
                 } else {
-                    Future<T> future = submit(task);
-                    try {
-                        result = future.get();
-                    } catch (InterruptedException e) {
-                        result = null;
-                        future.cancel(true);
+                    while (true) {
+                        Future<T> future = submit(task);
+
+                        try {
+                            return future.get();
+                        } catch (InterruptedException e) {
+                            // Repeat the attempt
+                            future.cancel(true);
+                        }
                     }
                 }
-                return result;
             } catch (Exception e) {
                 Throwable cause = (e instanceof ExecutionException) ? e.getCause() : e;
                 if (cause instanceof RuntimeException) {

test/javax/swing/JFileChooser/6713352/bug6713352.java

+/*
+ * Copyright 2009 Sun Microsystems, Inc.  All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
+ * CA 95054 USA or visit www.sun.com if you need additional information or
+ * have any questions.
+ */
+
+/* @test
+   @bug 6713352
+   @summary Deadlock in JFileChooser with synchronized custom FileSystemView
+   @author Pavel Porvatov
+   @run main bug6713352
+*/
+
+import sun.awt.shell.ShellFolder;
+
+import javax.swing.*;
+import javax.swing.filechooser.FileSystemView;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+
+public class bug6713352 {
+    public static void main(String[] args) throws Exception {
+        SwingUtilities.invokeAndWait(new Runnable() {
+            public void run() {
+                MyFileSystemView systemView = new MyFileSystemView();
+
+                synchronized (systemView) { // Get SystemView lock
+                    new JFileChooser(systemView);
+
+                    // Wait a little bit. BasicDirectoryModel will lock Invoker and stop on
+                    // the bug6713352.MyFileSystemView.getFiles() method
+                    try {
+                        Thread.sleep(5000);
+                    } catch (InterruptedException e) {
+                        throw new RuntimeException(e);
+                    }
+
+                    try {
+                        System.out.println("Try to get Invokers lock");
+
+                        ShellFolder.getShellFolder(new File("c:/")).listFiles(true);
+                    } catch (FileNotFoundException e) {
+                        throw new RuntimeException(e);
+                    }
+                }
+
+                // To avoid RejectedExecutionException in BasicDirectoryModel wait a second
+                try {
+                    Thread.sleep(1000);
+                } catch (InterruptedException e) {
+                    throw new RuntimeException(e);
+                }
+            }
+        });
+    }
+
+    private static class MyFileSystemView extends FileSystemView {
+
+        public File createNewFolder(File containingDir) throws IOException {
+            return null;
+        }
+
+        public File[] getFiles(File dir, boolean useFileHiding) {
+            System.out.println("getFiles start");
+
+            File[] result;
+
+            synchronized (this) {
+                result = super.getFiles(dir, useFileHiding);
+            }
+
+            System.out.println("getFiles finished");
+
+            return result;
+        }
+
+        public synchronized Boolean isTraversable(File f) {
+            return super.isTraversable(f);
+        }
+    }
+}