changeset 7486:982a483303a5

RT-37821 ConcurrentModificationException if an attempt is made to remove an accelerator whilst an accelerator callback is being processed
author Martin Sladecek <martin.sladecek@oracle.com>
date Tue, 15 Jul 2014 13:02:07 +0200
parents 321b64885bc3
children 2507b139676a
files modules/graphics/src/main/java/com/sun/javafx/scene/KeyboardShortcutsHandler.java modules/graphics/src/test/java/javafx/scene/AcceleratorsTest.java
diffstat 2 files changed, 196 insertions(+), 19 deletions(-) [+]
line wrap: on
line diff
--- a/modules/graphics/src/main/java/com/sun/javafx/scene/KeyboardShortcutsHandler.java	Tue Jul 15 09:29:54 2014 +0200
+++ b/modules/graphics/src/main/java/com/sun/javafx/scene/KeyboardShortcutsHandler.java	Tue Jul 15 13:02:07 2014 +0200
@@ -25,19 +25,20 @@
 
 package com.sun.javafx.scene;
 
+import java.util.AbstractMap;
+import java.util.AbstractSet;
 import java.util.ArrayList;
+import java.util.ConcurrentModificationException;
 import java.util.HashMap;
 import java.util.Iterator;
-import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import javafx.collections.ObservableList;
 import javafx.collections.ObservableMap;
 import javafx.event.Event;
 import javafx.scene.Node;
-import javafx.scene.Scene;
 import javafx.scene.input.KeyCombination;
-import javafx.scene.input.KeyCode;
 import javafx.scene.input.KeyEvent;
 import javafx.scene.input.Mnemonic;
 
@@ -47,14 +48,9 @@
 import com.sun.javafx.event.BasicEventDispatcher;
 import com.sun.javafx.scene.traversal.Direction;
 
-import static javafx.scene.input.KeyCode.DOWN;
-import static javafx.scene.input.KeyCode.LEFT;
-import static javafx.scene.input.KeyCode.RIGHT;
-import static javafx.scene.input.KeyCode.TAB;
-import static javafx.scene.input.KeyCode.UP;
-
 public final class KeyboardShortcutsHandler extends BasicEventDispatcher {
     private ObservableMap<KeyCombination, Runnable> accelerators;
+    private CopyOnWriteMap<KeyCombination, Runnable> acceleratorsBackingMap;
     private ObservableMap<KeyCombination, ObservableList<Mnemonic>> mnemonics;
 
     public void addMnemonic(Mnemonic m) {
@@ -94,7 +90,8 @@
 
     public ObservableMap<KeyCombination, Runnable> getAccelerators() {
         if (accelerators == null) {
-            accelerators = new ObservableMapWrapper<KeyCombination, Runnable>(new HashMap<KeyCombination, Runnable>());
+            acceleratorsBackingMap = new CopyOnWriteMap<>();
+            accelerators = new ObservableMapWrapper<>(acceleratorsBackingMap);
         }
         return accelerators;
     }
@@ -333,22 +330,27 @@
     }
 
     private void processAccelerators(KeyEvent event) {
-        if (accelerators != null) {
-            for (Map.Entry<KeyCombination, Runnable>
-                     accelerator: accelerators.entrySet()) {
-                
-                if (accelerator.getKey().match(event)) {
-                    Runnable acceleratorRunnable = accelerator.getValue();
-                    if (acceleratorRunnable != null) {
+        if (acceleratorsBackingMap != null) {
+            acceleratorsBackingMap.lock();
+            try {
+                for (Map.Entry<KeyCombination, Runnable>
+                        accelerator : acceleratorsBackingMap.backingMap.entrySet()) {
+
+                    if (accelerator.getKey().match(event)) {
+                        Runnable acceleratorRunnable = accelerator.getValue();
+                        if (acceleratorRunnable != null) {
                         /*
                         ** for accelerators there can only be one target
                         ** and we don't care whether it's visible or reachable....
                         ** we just run the Runnable.......
                         */
-                        acceleratorRunnable.run();
-                        event.consume();
+                            acceleratorRunnable.run();
+                            event.consume();
+                        }
                     }
                 }
+            } finally {
+                acceleratorsBackingMap.unlock();
             }
         }
     }
@@ -396,4 +398,81 @@
             }
         }
     }
+
+    private static class CopyOnWriteMap<K, V> extends AbstractMap<K, V> {
+
+        private Map<K, V> backingMap = new HashMap<>();
+        private boolean lock;
+
+        public void lock() {
+            lock = true;
+        }
+
+        public void unlock() {
+            lock = false;
+        }
+
+        @Override
+        public V put(K key, V value) {
+            if (lock) {
+                backingMap = new HashMap<>(backingMap);
+                lock = false;
+            }
+            return backingMap.put(key, value);
+        }
+
+        @Override
+        public Set<Entry<K, V>> entrySet() {
+            return new AbstractSet<Entry<K, V>>() {
+                @Override
+                public Iterator<Entry<K, V>> iterator() {
+                    return new Iterator<Entry<K, V>>() {
+
+                        private Iterator<Entry<K, V>> backingIt = backingMap.entrySet().iterator();
+                        private Map<K, V> backingMapAtCreation = backingMap;
+                        private Entry<K, V> lastNext = null;
+
+                        @Override
+                        public boolean hasNext() {
+                            checkCoMod();
+                            return backingIt.hasNext();
+                        }
+
+                        private void checkCoMod() {
+                            if (backingMap != backingMapAtCreation) {
+                                throw new ConcurrentModificationException();
+                            }
+                        }
+
+                        @Override
+                        public Entry<K, V> next() {
+                            checkCoMod();
+                            return lastNext = backingIt.next();
+                        }
+
+                        @Override
+                        public void remove() {
+                            checkCoMod();
+                            if (lastNext == null) {
+                                throw new IllegalStateException();
+                            }
+                            if (lock) {
+                                backingMap = new HashMap<>(backingMap);
+                                backingIt = backingMap.entrySet().iterator();
+                                while (!lastNext.equals(backingIt.next()));
+                                lock = false;
+                            }
+                            backingIt.remove();
+                            lastNext = null;
+                        }
+                    };
+                }
+
+                @Override
+                public int size() {
+                    return backingMap.size();
+                }
+            };
+        }
+    }
 }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/modules/graphics/src/test/java/javafx/scene/AcceleratorsTest.java	Tue Jul 15 13:02:07 2014 +0200
@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2011, 2014, Oracle and/or its affiliates. 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.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * 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 Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package javafx.scene;
+
+import com.sun.javafx.test.MouseEventGenerator;
+import javafx.collections.ObservableMap;
+import javafx.scene.input.KeyCode;
+import javafx.scene.input.KeyCombination;
+import javafx.scene.input.KeyEvent;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.ConcurrentModificationException;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class AcceleratorsTest {
+
+    private Scene scene;
+    private ObservableMap<KeyCombination, Runnable> accelerators;
+
+    @Before
+    public void setUp() {
+        scene = new Scene(new Group());
+        accelerators = scene.getAccelerators();
+    }
+
+
+    @Test
+    public void testAcceleratorExecuted() {
+        AtomicBoolean executed = new AtomicBoolean();
+        accelerators.put(KeyCombination.keyCombination("Alt + A"), () -> executed.set(true));
+        scene.impl_processKeyEvent(new KeyEvent(KeyEvent.KEY_PRESSED, "A", "A", KeyCode.A, false, false, true, false));
+        assertTrue(executed.get());
+    }
+
+    @Test
+    public void testAcceleratorRemovedWhenExecuted() {
+        AtomicBoolean executed = new AtomicBoolean();
+        final KeyCombination altA = KeyCombination.keyCombination("Alt + A");
+        final KeyCombination altB = KeyCombination.keyCombination("Alt + B");
+        accelerators.put(altA, () -> accelerators.remove(altA));
+        accelerators.put(altB, () -> executed.set(true));
+        assertEquals(2, accelerators.size());
+        scene.impl_processKeyEvent(new KeyEvent(KeyEvent.KEY_PRESSED, "A", "A", KeyCode.A, false, false, true, false));
+        assertEquals(1, accelerators.size());
+        assertFalse(executed.get());
+        scene.impl_processKeyEvent(new KeyEvent(KeyEvent.KEY_PRESSED, "B", "B", KeyCode.B, false, false, true, false));
+        assertTrue(executed.get());
+    }
+
+    @Test(expected = ConcurrentModificationException.class)
+    public void testAcceleratorComodification() {
+        final KeyCombination altA = KeyCombination.keyCombination("Alt + A");
+        final KeyCombination altB = KeyCombination.keyCombination("Alt + B");
+        accelerators.put(altA, () -> {
+        });
+        accelerators.put(altB, () -> {
+        });
+
+        final Iterator<Map.Entry<KeyCombination, Runnable>> iterator = accelerators.entrySet().iterator();
+        iterator.next();
+
+        final Iterator<Map.Entry<KeyCombination, Runnable>> iterator1 = accelerators.entrySet().iterator();
+        iterator1.next();
+        iterator1.remove();
+
+        iterator.next();
+    }
+}