Ensure wrong Action is not executed after detaching a component (#9806)

Due to action key generation on ActionManager a wrong action
may be executed if the component that fired the wanted action
is already detached.

This patch makes action keys globally unique, simplifying the
approach of #8495 but reusing its tests.

Fixes #5864

2 files changed, 51 insertions, 13 deletions

diff --git a/server/src/main/java/com/vaadin/event/ActionManager.java b/server/src/main/java/com/vaadin/event/ActionManager.java
index 1056106b16..fbc5c8411f 100644
--- a/server/src/main/java/com/vaadin/event/ActionManager.java
+++ b/server/src/main/java/com/vaadin/event/ActionManager.java
@@ -19,6 +19,7 @@ import java.util.Arrays;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import com.vaadin.event.Action.Container;
 import com.vaadin.event.Action.Handler;
@@ -166,14 +167,16 @@ public class ActionManager
          * removed but still exist on client side
          */
         if (!actions.isEmpty() || clientHasActions) {
-            actionMapper = new KeyMapper<>();
+            actionMapper = new ActionKeyMapper();
 
             paintTarget.addVariable((VariableOwner) viewer, "action", "");
             paintTarget.startTag("actions");
 
             for (final Action a : actions) {
+
                 paintTarget.startTag("action");
                 final String akey = actionMapper.key(a);
+
                 paintTarget.addAttribute("key", akey);
                 if (a.getCaption() != null) {
                     paintTarget.addAttribute("caption", a.getCaption());
@@ -244,7 +247,6 @@ public class ActionManager
         LinkedHashSet<Action> actions = new LinkedHashSet<>();
         if (ownActions != null) {
             actions.addAll(ownActions);
-
         }
         if (actionHandlers != null) {
             for (Action.Handler h : actionHandlers) {
@@ -256,4 +258,20 @@ public class ActionManager
         }
         return actions;
     }
+
+    /**
+     * Extension of KeyMapper that avoids reusing keys even across different
+     * instances.
+     *
+     * @since 8.1.2
+     */
+    private static class ActionKeyMapper extends KeyMapper<Action> {
+        private static AtomicInteger lastKey = new AtomicInteger(0);
+
+        @Override
+        protected String createKey() {
+            return String.valueOf(lastKey.incrementAndGet());
+        }
+    }
+
 }
diff --git a/server/src/main/java/com/vaadin/server/KeyMapper.java b/server/src/main/java/com/vaadin/server/KeyMapper.java
index f7b878dd27..d20eb7c605 100644
--- a/server/src/main/java/com/vaadin/server/KeyMapper.java
+++ b/server/src/main/java/com/vaadin/server/KeyMapper.java
@@ -43,9 +43,11 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Constructs a new mapper
      *
-     * @param identifierGetter has to return a unique key for every bean, and the returned key has to
-     *                         follow general {@code hashCode()} and {@code equals()} contract,
-     *                         see {@link Object#hashCode()} for details.
+     * @param identifierGetter
+     *            has to return a unique key for every bean, and the returned
+     *            key has to follow general {@code hashCode()} and
+     *            {@code equals()} contract, see {@link Object#hashCode()} for
+     *            details.
      * @since 8.1
      */
     public KeyMapper(ValueProvider<V, Object> identifierGetter) {
@@ -62,7 +64,8 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Gets key for an object.
      *
-     * @param o the object.
+     * @param o
+     *            the object.
      */
     @Override
     public String key(V o) {
@@ -79,13 +82,25 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
         }
 
         // If the object is not yet mapped, map it
-        key = String.valueOf(++lastKey);
+        key = createKey();
         objectIdKeyMap.put(id, key);
         keyObjectMap.put(key, o);
 
         return key;
     }
 
+    /**
+     * Creates a key for a new item.
+     *
+     * This method can be overridden to customize the keys used.
+     *
+     * @return new key
+     * @since 8.1.2
+     */
+    protected String createKey() {
+        return String.valueOf(++lastKey);
+    }
+
     @Override
     public boolean has(V o) {
         return objectIdKeyMap.containsKey(identifierGetter.apply(o));
@@ -94,7 +109,8 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Retrieves object with the key.
      *
-     * @param key the name with the desired value.
+     * @param key
+     *            the name with the desired value.
      * @return the object with the key.
      */
     @Override
@@ -105,11 +121,13 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Removes object from the mapper.
      *
-     * @param removeobj the object to be removed.
+     * @param removeobj
+     *            the object to be removed.
      */
     @Override
     public void remove(V removeobj) {
-        final String key = objectIdKeyMap.remove(identifierGetter.apply(removeobj));
+        final String key = objectIdKeyMap
+                .remove(identifierGetter.apply(removeobj));
         if (key != null) {
             keyObjectMap.remove(key);
         }
@@ -127,9 +145,10 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Checks if the given key is mapped to an object.
      *
-     * @param key the key to check
+     * @param key
+     *            the key to check
      * @return <code>true</code> if the key is currently mapped,
-     * <code>false</code> otherwise
+     *         <code>false</code> otherwise
      * @since 7.7
      */
     public boolean containsKey(String key) {
@@ -151,7 +170,8 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
             this.identifierGetter = identifierGetter;
             objectIdKeyMap.clear();
             for (Map.Entry<String, V> entry : keyObjectMap.entrySet()) {
-                objectIdKeyMap.put(identifierGetter.apply(entry.getValue()),entry.getKey());
+                objectIdKeyMap.put(identifierGetter.apply(entry.getValue()),
+                        entry.getKey());
             }
         }
     }