Make removeFromParent throw if the right session is not locked (#13473)

Change-Id: Id5ef40db07404d7cb41b26768d18e757b8cae2b3

server/src/com/vaadin/server/VaadinService.java

@@ -1635,14 +1635,32 @@ public abstract class VaadinService implements Serializable {
      *             if the current thread holds the lock for another session
      */
     public static void verifyNoOtherSessionLocked(VaadinSession session) {
-        VaadinSession otherSession = VaadinSession.getCurrent();
-        if (otherSession != null && otherSession != session
-                && otherSession.hasLock()) {
+        if (isOtherSessionLocked(session)) {
             throw new IllegalStateException(
                     "Can't access session while another session is locked by the same thread. This restriction is intended to help avoid deadlocks.");
         }
     }
 
+    /**
+     * Checks whether there might be some {@link VaadinSession} other than the
+     * provided one for which the current thread holds a lock. This method might
+     * not detect all cases where some other session is locked, but it should
+     * cover the most typical situations.
+     * 
+     * @since 7.2
+     * @param session
+     *            the session that is expected to be locked
+     * @return <code>true</code> if another session is also locked by the
+     *         current thread; <code>false</code> if no such session was found
+     */
+    public static boolean isOtherSessionLocked(VaadinSession session) {
+        VaadinSession otherSession = VaadinSession.getCurrent();
+        if (otherSession == null || otherSession == session) {
+            return false;
+        }
+        return otherSession.hasLock();
+    }
+
     /**
      * Verifies that the given CSRF token (aka double submit cookie) is valid
      * for the given session. This is used to protect against Cross Site Request

server/src/com/vaadin/ui/AbstractSingleComponentContainer.java

@@ -19,6 +19,8 @@ import java.util.Collections;
 import java.util.Iterator;
 
 import com.vaadin.server.ComponentSizeValidator;
+import com.vaadin.server.VaadinService;
+import com.vaadin.server.VaadinSession;
 
 /**
  * Abstract base class for component containers that have only one child
@@ -150,6 +152,19 @@ public abstract class AbstractSingleComponentContainer extends
     // TODO move utility method elsewhere?
     public static void removeFromParent(Component content)
             throws IllegalArgumentException {
+        // Verify the appropriate session is locked
+        UI parentUI = content.getUI();
+        if (parentUI != null) {
+            VaadinSession parentSession = parentUI.getSession();
+            if (parentSession != null && !parentSession.hasLock()) {
+                String message = "Cannot remove from parent when the session is not locked.";
+                if (VaadinService.isOtherSessionLocked(parentSession)) {
+                    message += " Furthermore, there is another locked session, indicating that the component might be about to be moved from one session to another.";
+                }
+                throw new IllegalStateException(message);
+            }
+        }
+
         HasComponents parent = content.getParent();
         if (parent instanceof ComponentContainer) {
             // If the component already has a parent, try to remove it

server/tests/src/com/vaadin/tests/server/component/abstractsinglecomponentcontainer/TestRemoveFromParentLocking.java

@@ -0,0 +1,125 @@
+/*
+ * Copyright 2000-2013 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.tests.server.component.abstractsinglecomponentcontainer;
+
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.server.VaadinSession;
+import com.vaadin.ui.UI;
+import com.vaadin.ui.VerticalLayout;
+
+public class TestRemoveFromParentLocking {
+
+    private static VerticalLayout createTestComponent() {
+        VaadinSession session = new VaadinSession(null) {
+            private final ReentrantLock lock = new ReentrantLock();
+
+            @Override
+            public Lock getLockInstance() {
+                return lock;
+            }
+        };
+
+        session.getLockInstance().lock();
+
+        UI ui = new UI() {
+            @Override
+            protected void init(VaadinRequest request) {
+            }
+        };
+        ui.setSession(session);
+
+        VerticalLayout layout = new VerticalLayout();
+        ui.setContent(layout);
+
+        session.getLockInstance().unlock();
+        return layout;
+    }
+
+    @Test
+    public void attachNoSessionLocked() {
+        VerticalLayout testComponent = createTestComponent();
+
+        VerticalLayout target = new VerticalLayout();
+
+        try {
+            target.addComponent(testComponent);
+            throw new AssertionError(
+                    "Moving component when not holding its sessions's lock should throw");
+        } catch (IllegalStateException e) {
+            Assert.assertEquals(
+                    "Cannot remove from parent when the session is not locked.",
+                    e.getMessage());
+        }
+    }
+
+    @Test
+    public void attachSessionLocked() {
+        VerticalLayout testComponent = createTestComponent();
+
+        VerticalLayout target = new VerticalLayout();
+
+        testComponent.getUI().getSession().getLockInstance().lock();
+
+        target.addComponent(testComponent);
+        // OK if we get here without any exception
+    }
+
+    @Test
+    public void crossAttachOtherSessionLocked() {
+        VerticalLayout notLockedComponent = createTestComponent();
+
+        VerticalLayout lockedComponent = createTestComponent();
+
+        // Simulate the situation when attaching cross sessions
+        lockedComponent.getUI().getSession().getLockInstance().lock();
+        VaadinSession.setCurrent(lockedComponent.getUI().getSession());
+
+        try {
+            lockedComponent.addComponent(notLockedComponent);
+            throw new AssertionError(
+                    "Moving component when not holding its sessions's lock should throw");
+        } catch (IllegalStateException e) {
+            Assert.assertEquals(
+                    "Cannot remove from parent when the session is not locked."
+                            + " Furthermore, there is another locked session, indicating that the component might be about to be moved from one session to another.",
+                    e.getMessage());
+        }
+    }
+
+    @Test
+    public void crossAttachThisSessionLocked() {
+        VerticalLayout notLockedComponent = createTestComponent();
+
+        VerticalLayout lockedComponent = createTestComponent();
+
+        // Simulate the situation when attaching cross sessions
+        lockedComponent.getUI().getSession().getLockInstance().lock();
+        VaadinSession.setCurrent(lockedComponent.getUI().getSession());
+
+        try {
+            notLockedComponent.addComponent(lockedComponent);
+        } catch (AssertionError e) {
+            // All is fine, don't care about the exact wording in this case
+        }
+    }
+
+}