From 7c85df522cda83a61057c40924f6413b2df848d7 Mon Sep 17 00:00:00 2001 From: Marco Collovati Date: Thu, 13 Aug 2026 11:16:45 +0000 Subject: [PATCH] fix: avoid NPE when canceling a JavaScript invocation of a closed UI (#25095) Invocations owned by an invisible component are retained in the UI's queue and get a detach listener registered for them. Registering that listener installs a handler on the invocation itself, and the handler stays attached to the invocation for the rest of its lifetime, since there is no way to unsubscribe it. A component that keeps the PendingJavaScriptResult and cancels it after being reused in another UI therefore runs the handler installed by the closed UI, which dereferences its cleared session. Return early when the UI no longer has a session, as its invocation queue has already been released by then. Also release the retained invocations when the UI is closed, so that the queue and the detach listener registrations on the state nodes do not outlive the UI when a detach listener fails and prevents the ones after it from running. Fixes #25092 --- .../flow/component/internal/UIInternals.java | 29 +++++ .../component/internal/UIInternalsTest.java | 123 ++++++++++++++++++ 2 files changed, 152 insertions(+) diff --git a/flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java b/flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java index 75ceed02028..4070407cf67 100644 --- a/flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java +++ b/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 dumpPendingJavaScriptInvocations() { return readyToSend; } + /** + * Discards the JavaScript invocations still queued for the related UI and + * unregisters the detach listeners tracking them. + *

+ * 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) { diff --git a/flow-server/src/test/java/com/vaadin/flow/component/internal/UIInternalsTest.java b/flow-server/src/test/java/com/vaadin/flow/component/internal/UIInternalsTest.java index 50499afe849..3a1c5d34373 100644 --- a/flow-server/src/test/java/com/vaadin/flow/component/internal/UIInternalsTest.java +++ b/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));