fix: avoid NPE when canceling a JavaScript invocation of a closed UI

[mcollovati]

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

[github-actions]

Test Results

 1 447 files  ±0   1 447 suites  ±0   1h 43m 51s ⏱️ - 1m 9s
10 345 tests +3  10 278 ✅ +3  67 💤 ±0  0 ❌ ±0 
10 799 runs  +3  10 731 ✅ +3  68 💤 ±0  0 ❌ ±0 

Results for commit 7c22bf1. ± Comparison against base commit ffc3563.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[mcollovati]

Needs manual pick to 24.10 and 24.9 because of JUnit 4

[mshabarov]

@claude review this pull request

[claude]

Claude finished @mshabarov's task in 1m 46s —— View job

---

PR Review: avoid NPE when canceling a JavaScript invocation of a closed UI

• Read UIInternals.java change
• Read new tests in UIInternalsTest.java
• Analyze correctness, ordering, thread-safety, and lifecycle
• Post review

---

Summary

The fix is correct and well-targeted. It addresses the NPE in two complementary ways, both consistent with the existing lifecycle design:

1. removePendingInvocation returns early when session == null — this is the actual NPE guard for the "component reused in another UI cancels a retained invocation" path.
2. releasePendingJavaScriptInvocations() proactively releases retained invocations and unregisters their detach listeners while the session is still available during UI close.

The test coverage is thorough and reproduces the reported failure modes precisely (retained-invocation cancel, throwing detach listener, and element reuse across UIs).

Correctness verification

Ordering in setSession(null) is right. releasePendingJavaScriptInvocations() runs inside the if (session == null) (parameter) branch, before this.session = session sets the field to null (UIInternals.java:530). So session.checkHasLock() and the per-invocation release still operate against the live session — matching the Javadoc ("called while the session is still available"). ✅

No double-processing on normal close. Detaching the root node (UIInternals.java:519) already fires the node detach listeners, which run execute() and unregister themselves from pendingJsInvocationDetachListeners. By the time releasePendingJavaScriptInvocations() runs, the map is empty, so List.copyOf(...) iterates nothing and pendingJsInvocations.clear() is a cheap no-op. The new method only does real work in the failure case (a listener threw before the framework listener ran). ✅

ConcurrentModification is handled. execute() → removePendingInvocation → registration.remove() mutates pendingJsInvocationDetachListeners, so copying the values first (List.copyOf(...)) before iterating is necessary and correct. ✅

Post-close callback is safe. After release, execute() clears invocationList and nulls the registration. A later completion/cancel of the still-referenced invocation calls onInvocationCompleted → removePendingInvocation, which now hits the session == null early return. No NPE, no stale mutation. ✅

Observation (non-blocking): potential memory leak of the closed UIInternals

This isn't introduced by the PR — the PR actually improves the situation — but it's worth documenting as a known trade-off / follow-up:

PendingJavaScriptInvocationDetachListener is a non-static inner class, so it holds an implicit reference to UIInternals.this. The then callback registered in registerDetachListenerForPendingInvocation (UIInternals.java:760-762) captures that listener and is retained by the invocation for its whole lifetime, with no way to unsubscribe (as the PR description notes).

Consequence: if a component keeps a long-lived PendingJavaScriptResult and is reused across UIs, the closed UIInternals (and transitively its StateTree) stays reachable from the invocation until the invocation itself is released. The session == null guard prevents the crash but doesn't break this reference chain.

A fully clean solution would need an unsubscribe mechanism on PendingJavaScriptInvocation.then(...), which is a larger change. For this bug-fix PR the current approach is a reasonable and safe choice — just flagging it so it's a conscious decision. Explore unsubscribe follow-up →

Minor nits

• The early-return in removePendingInvocation reads the volatile session field twice (null check, then checkHasLock()). In practice both run under a session lock so there's no real TOCTOU, but a single local read would be marginally cleaner. Not worth changing.
• Consider whether getPendingJavaScriptInvocations() / other session.checkHasLock() call sites could also be reached post-close via a retained invocation; from the traced paths they cannot, so no action needed — just confirming the guard is placed at the one reachable site.

Tests

The three added tests are the essential ones and map cleanly to the two code changes:

• closedUI_retainedInvocationCanceled_noNullPointerException — the direct NPE regression.
• closedUI_detachListenerNotRun_pendingInvocationsCleanedUp — validates releasePendingJavaScriptInvocations() cleanup via reflection on both internal collections.
• elementRemovedFromTree_uiClosed_reusedInAnotherUI_invocationReleased — the element-reuse-across-UIs path.

Good use of assertDoesNotThrow and direct state assertions rather than just null checks, per the repo's testing guidance.

Verdict

Looks good to merge. The core fix is correct, minimal, and safe; ordering and concurrency are handled; tests are on point. The only thing I'd suggest tracking is the retained-reference/memory-leak observation above as a possible follow-up. (Note the author already flagged manual picks to 24.10 / 24.9 due to JUnit 4.)
· branch issues/25092-pending_invocation_cancel_issue