fix: prevent NPE in ElementEffect detach listener on re-attach (#25005) (CP: 25.2) (#25013)

This PR cherry-picks changes from the original PR #25005 to branch 25.2.
---
#### Original PR description
> ElementEffect registered a new detach listener on every attach, stored
it in the single detachRegistration field, and relied on each detach
listener removing itself. When an element is re-attached without a
detach event firing in between - StateNode.removeFromTree(false), as
used by UIInternals.moveToNewUI for `@PreserveOnRefresh` - a second
detach listener was registered while the first was still present. The
next real detach ran both: the first nulled detachRegistration, the
second dereferenced null and threw a NullPointerException.
> 
> Register the detach listener through a helper that removes any
previous registration before adding a new one, and null-guard the
removal inside the listener so at most one detach listener is ever
active.
> 
> Fixes #24973

Co-authored-by: Marco Collovati <marco@vaadin.com>

Author: vaadin-bot

flow-server/src/main/java/com/vaadin/flow/dom/ElementEffect.java

@@ -87,11 +87,7 @@ public ElementEffect(Element owner, ContextualEffectAction effectFunction) {
             // execution context.
             enableEffect(owner);
 
-            detachRegistration = owner.addDetachListener(detach -> {
-                disableEffect();
-                detachRegistration.remove();
-                detachRegistration = null;
-            });
+            registerDetachListener();
         } else {
             // Element is not yet attached: run a probe immediately so that
             // structural errors (e.g. MissingSignalUsageException) are reported
@@ -106,11 +102,33 @@ public ElementEffect(Element owner, ContextualEffectAction effectFunction) {
         attachRegistration = owner.addAttachListener(attach -> {
             enableEffect(attach.getSource());
 
-            detachRegistration = owner.addDetachListener(detach -> {
-                disableEffect();
+            registerDetachListener();
+        });
+    }
+
+    /**
+     * Registers the detach listener that disables the effect when the owner is
+     * detached.
+     * <p>
+     * A detach listener from a previous attach may still be registered if the
+     * element was re-attached without a detach event firing in between - e.g.
+     * {@code StateNode.removeFromTree(false)} as used by
+     * {@code UIInternals.moveToNewUI} for {@code @PreserveOnRefresh}. Such a
+     * stale listener is removed first so that the effect never accumulates
+     * multiple detach listeners sharing the single {@link #detachRegistration}
+     * field, which would otherwise cause a {@link NullPointerException} when
+     * the second listener dereferences the already-nulled registration.
+     */
+    private void registerDetachListener() {
+        if (detachRegistration != null) {
+            detachRegistration.remove();
+        }
+        detachRegistration = owner.addDetachListener(detach -> {
+            disableEffect();
+            if (detachRegistration != null) {
                 detachRegistration.remove();
                 detachRegistration = null;
-            });
+            }
         });
     }
 

flow-server/src/test/java/com/vaadin/flow/dom/ElementEffectTest.java

@@ -50,6 +50,7 @@
 import com.vaadin.flow.signals.shared.SharedValueSignal;
 import com.vaadin.tests.util.MockUI;
 
+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;
@@ -630,6 +631,49 @@ void effect_componentAttachedAndDetached_effectEnabledAndDisabled() {
         assertEquals(4, count.get(), "Effect should not be run after remove");
     }
 
+    @Test
+    void effect_reattachedViaMoveToNewUI_detachDoesNotThrow() {
+        // Reproduces #24973: UIInternals.moveToNewUI (used for
+        // @PreserveOnRefresh) re-attaches an element via
+        // StateNode.removeFromTree(false) followed by appendChild. This fires
+        // the attach listener again without a detach event in between.
+        // ElementEffect must not accumulate multiple detach listeners sharing
+        // the single detachRegistration field, otherwise the next real detach
+        // throws a NullPointerException.
+        CurrentInstance.clearAll();
+        TestComponent component = new TestComponent();
+        ValueSignal<String> signal = new ValueSignal<>("initial");
+        AtomicInteger count = new AtomicInteger();
+        Signal.effect(component, () -> {
+            signal.get();
+            count.incrementAndGet();
+        });
+
+        MockUI ui = new MockUI();
+        ui.add(component);
+
+        // Simulate UIInternals.moveToNewUI: reset the node without firing
+        // detach listeners, then re-attach the element to a new UI.
+        MockUI newUi = new MockUI();
+        component.getElement().getNode().removeFromTree(false);
+        newUi.getElement().appendChild(component.getElement());
+
+        // An ordinary detach must not throw. Before the fix this raised a
+        // NullPointerException from a second, stale ElementEffect detach
+        // listener dereferencing the already-nulled registration.
+        assertDoesNotThrow(() -> component.getElement().removeFromParent());
+
+        // The effect keeps working after the move: it is disabled while
+        // detached and re-enabled (and re-run because the signal changed) on a
+        // fresh attach.
+        signal.set("while detached");
+        int countAfterDetach = count.get();
+        newUi.getElement().appendChild(component.getElement());
+        signal.set("after reattach");
+        assertTrue(count.get() > countAfterDetach,
+                "Effect should still run after re-attach following a move");
+    }
+
     @Test
     void effect_reattachWithoutChanges_effectNotReRun() {
         CurrentInstance.clearAll();