fix: avoid NPE when canceling a JavaScript invocation of a closed UI (#25095) (CP: 25.1)

flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java

@@ -460,6 +460,7 @@ public void setSession(VaadinSession session) {
                     getLogger().warn("Error detaching closed UI {} ",
                             ui.getUIId(), e);
                 }
+                releasePendingJavaScriptInvocations();
                 // Disable push when the UI is detached. Otherwise the
                 // push connection and possibly VaadinSession will live on.
                 ui.getPushConfiguration().setPushMode(PushMode.DISABLED);
@@ -644,6 +645,27 @@ public List<PendingJavaScriptInvocation> dumpPendingJavaScriptInvocations() {
         return readyToSend;
     }
 
+    /**
+     * Discards the JavaScript invocations still queued for the related UI and
+     * unregisters the detach listeners tracking them.
+     * <p>
+     * Detaching the UI normally runs those detach listeners, which release the
+     * invocations they track. A listener that throws prevents the remaining
+     * ones on the same node from running, so the invocations are released here
+     * as well. This is called while the session is still available, since
+     * releasing an invocation requires the session lock.
+     */
+    private void releasePendingJavaScriptInvocations() {
+        session.checkHasLock();
+        // Copied because releasing an invocation unregisters its listener,
+        // which removes it from the map
+        List.copyOf(pendingJsInvocationDetachListeners.values())
+                .forEach(PendingJavaScriptInvocationDetachListener::execute);
+        // Invocations added after the last dump have no detach listener yet,
+        // and a closed UI can no longer send them
+        pendingJsInvocations.clear();
+    }
+
     @SuppressWarnings({ "rawtypes", "unchecked" })
     private void registerDetachListenerForPendingInvocation(
             PendingJavaScriptInvocation invocation) {
@@ -681,6 +703,13 @@ public void execute() {
 
         private void removePendingInvocation(
                 PendingJavaScriptInvocation invocation) {
+            if (session == null) {
+                // The UI has been closed, so its invocation queue has already
+                // been released. The handler this runs from stays attached to
+                // the invocation, so a component reusing the invocation in
+                // another UI can still reach this point.
+                return;
+            }
             session.checkHasLock();
             UIInternals.this.pendingJsInvocations.remove(invocation);
             if (invocationList.isEmpty() && registration != null) {

flow-server/src/test/java/com/vaadin/flow/component/internal/UIInternalsTest.java

@@ -15,10 +15,13 @@
  */
 package com.vaadin.flow.component.internal;
 
+import java.lang.reflect.Field;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
@@ -35,6 +38,7 @@
 import com.vaadin.flow.component.PushConfiguration;
 import com.vaadin.flow.component.Tag;
 import com.vaadin.flow.component.UI;
+import com.vaadin.flow.component.page.PendingJavaScriptResult;
 import com.vaadin.flow.component.page.Push;
 import com.vaadin.flow.dom.Element;
 import com.vaadin.flow.function.DeploymentConfiguration;
@@ -54,6 +58,7 @@
 import com.vaadin.tests.util.MockDeploymentConfiguration;
 
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -453,6 +458,124 @@ void dumpPendingJavaScriptInvocations_multipleInvocation_detachListenerRegistere
         assertEquals(0, internals.getPendingJavaScriptInvocations().count());
     }
 
+    @Test
+    void closedUI_retainedInvocationCanceled_noNullPointerException() {
+        UI closedUI = new UI();
+        UIInternals closedInternals = closedUI.getInternals();
+        closedInternals
+                .setSession(new AlwaysLockedVaadinSession(vaadinService));
+
+        Element element = new Element("div");
+        element.setVisible(false);
+        closedUI.getElement().appendChild(element);
+        PendingJavaScriptResult pending = element.executeJs("this.foo = $0",
+                "bar");
+
+        // The invocation is owned by an invisible component, so it is retained
+        // in the queue and gets a detach listener registered for it
+        closedInternals.getStateTree().runExecutionsBeforeClientResponse();
+        closedInternals.dumpPendingJavaScriptInvocations();
+
+        closedInternals.setSession(null);
+
+        // The component may be reused in another UI and cancel the invocation
+        // it still references, which triggers the handler registered by the
+        // closed UI
+        assertDoesNotThrow(pending::cancelExecution,
+                "Canceling an invocation retained by a closed UI should not fail");
+    }
+
+    @Test
+    void closedUI_detachListenerNotRun_pendingInvocationsCleanedUp()
+            throws Exception {
+        UI closedUI = new UI();
+        UIInternals closedInternals = closedUI.getInternals();
+        closedInternals
+                .setSession(new AlwaysLockedVaadinSession(vaadinService));
+
+        Element element = new Element("div");
+        element.setVisible(false);
+        closedUI.getElement().appendChild(element);
+
+        // Registered before the detach listener that dumping adds, and
+        // StateNode.fireDetachListeners has no per-listener guard, so this
+        // prevents the framework listener from running when the UI is closed
+        element.getNode().addDetachListener(() -> {
+            throw new IllegalStateException("detach listener failure");
+        });
+        element.executeJs("this.foo = $0", "bar");
+
+        closedInternals.getStateTree().runExecutionsBeforeClientResponse();
+        closedInternals.dumpPendingJavaScriptInvocations();
+
+        assertEquals(1, pendingInvocations(closedInternals).size(),
+                "Invocation of invisible component should be retained");
+        assertEquals(1, detachListeners(closedInternals).size(),
+                "Detach listener should be registered for retained invocation");
+
+        closedInternals.setSession(null);
+
+        assertEquals(0, pendingInvocations(closedInternals).size(),
+                "Closing the UI should discard retained invocations");
+        assertEquals(0, detachListeners(closedInternals).size(),
+                "Closing the UI should unregister the invocation detach listeners");
+    }
+
+    @Test
+    void elementRemovedFromTree_uiClosed_reusedInAnotherUI_invocationReleased()
+            throws Exception {
+        UI firstUI = new UI();
+        UIInternals firstInternals = firstUI.getInternals();
+        firstInternals.setSession(new AlwaysLockedVaadinSession(vaadinService));
+
+        Element element = new Element("div");
+        element.setVisible(false);
+        firstUI.getElement().appendChild(element);
+        PendingJavaScriptResult pending = element.executeJs("this.foo = $0",
+                "bar");
+
+        firstInternals.getStateTree().runExecutionsBeforeClientResponse();
+        firstInternals.dumpPendingJavaScriptInvocations();
+
+        // Detaches the element before resetting its node, which releases the
+        // invocation from the queue of the first UI. The handler registered for
+        // it stays attached to the invocation itself.
+        element.removeFromTree(false);
+
+        assertEquals(0, pendingInvocations(firstInternals).size(),
+                "Detaching the element should release the invocation");
+
+        firstInternals.setSession(null);
+
+        // The element is reused in a new UI and cancels the invocation it still
+        // references while attaching
+        UI secondUI = new UI();
+        secondUI.getInternals()
+                .setSession(new AlwaysLockedVaadinSession(vaadinService));
+        secondUI.getElement().appendChild(element);
+
+        assertDoesNotThrow(pending::cancelExecution,
+                "Canceling an invocation retained by a closed UI should not fail");
+    }
+
+    private static Collection<?> pendingInvocations(UIInternals internals)
+            throws Exception {
+        return (Collection<?>) readField(internals, "pendingJsInvocations");
+    }
+
+    private static Map<?, ?> detachListeners(UIInternals internals)
+            throws Exception {
+        return (Map<?, ?>) readField(internals,
+                "pendingJsInvocationDetachListeners");
+    }
+
+    private static Object readField(UIInternals internals, String name)
+            throws Exception {
+        Field field = UIInternals.class.getDeclaredField(name);
+        field.setAccessible(true);
+        return field.get(internals);
+    }
+
     @Test
     void isDirty_noPendingJsInvocation_returnsFalse() {
         StateNode node1 = Mockito.spy(new StateNode(ElementData.class));