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));