On Fri, 3 Jul 2026 12:36:44 GMT, Jose Pereda <[email protected]> wrote:
> This PR captures the `currentValue` of the fired value change event in > `ExpressionHelper$Generic::fireValueChangedEvent`, before entering the > notification loop, in order to pass it down to all the change listeners. > > This prevents a potential issue that could happen if all the change listeners > are removed before the notification loop ends: the `currentValue` is > nullified, and the pending listeners would unexpectedly receive null, instead > of the value that was correctly sent to the previous listeners. > > Some unit tests have been included, all of them but one pass before and after > the proposed fix, and the last one fails before the fix and passes after it. > > The scenario that describes this failing test is: > - A property has attached a few InvalidationListeners and a few > ChangeListeners (so the Generic path is followed, and not the SingleChange). > - At some point the property changes to a non-null `newValue`, and all the > listeners get notified, one by one, in the same order as they were > registered. > - If, for any reason, and _while_ the different change listeners are being > notified, all the change listeners are removed, the pending listeners will, > unexpectedly, receive `null`, though the listeners that were notified before > that happens received the expected `newValue`. > > For simplicity, the test simulates that exact moment by removing all the > change listeners when `listenerA` receives `newValue` from the `changed` > notification. Then the `currentValue` gets nullified, and then the pending > listeners in the notification list `curChangeList` (`listenerB` and > `listenerC`, as they were registered after `listenerA`) get `null`. There > could be other triggers, as long as they happen on the same thread, as part > of the same call stack that started `fireValueChangedEvent`. > > With the fix, by capturing the `currentValue` into `newValue` before entering > the loop, then `B` and `C` will still receive `newValue`, regardless any > possible change done to `currentValue` in the process. > > --------- > - [X] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). ## Pull request overview This PR hardens JavaFX’s internal `ExpressionHelper.Generic` change-notification path so that change listeners receive a consistent “new value” even if listeners are removed during the same `fireValueChangedEvent` call stack. **Changes:** - Capture the “new value” once and reuse it for all `ChangeListener.changed(...)` callbacks to prevent late listeners seeing `null` after listener removals. - Add new unit tests covering listener-removal-during-notification scenarios in the Generic helper path. - Update copyright years in touched files. ### Reviewed changes Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments. | File | Description | | ---- | ----------- | | modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java | Adjusts `Generic.fireValueChangedEvent()` to pass a stable value to change listeners. | | modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperTest.java | Adds regression tests for listener removal during notification; minor import/message updates. | --- 💡 <a href="/openjdk/jfx/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot"; class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>. modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java line 387: > 385: // currentValue could be nullified if all the > change listeners are removed. If that happens > 386: // during the notification loop, the pending > listeners should still receive the correct value. > 387: final T newValue = currentValue; The new-value capture currently relies on the `currentValue` field, but `Generic.removeListener(ChangeListener)` clears `currentValue` when the last change listener is removed (ExpressionHelper.java:324). Because invalidation listeners are notified before this block, an invalidation listener can remove the last change listener and clear `currentValue` before it is captured here, so pending change listeners from the snapshot can still receive `null` and the `changed` check can be computed against `null`. Using a local `newValue` that falls back to `observable.getValue()` when `currentValue` has been cleared avoids that edge case without changing the earlier helper-transition logic. modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperTest.java line 816: > 814: final ChangeListener<Object> listenerA = (observable, oldValue, > newValue) -> > 815: ExpressionHelper.removeListener(helper, listenerB); > 816: `ExpressionHelper.removeListener(...)` can return a different helper instance when the helper type changes (e.g., when the last listener of a kind is removed). Ignoring the return value here can leave the `helper` field pointing at a stale instance, making the test fragile and potentially incorrect if the listener counts change. modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperTest.java line 825: > 823: > 824: assertEquals(DATA_1, oldValueRef.get(), "ListenerC should > observer the old value"); > 825: assertEquals(DATA_2, newValueRef.get(), "ListenerC should > observer the new value"); Typo in assertion messages: "should observer" should be "should observe". modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperTest.java line 842: > 840: final ChangeListener<Object> listenerA = (observable, oldValue, > newValue) -> > 841: ExpressionHelper.removeListener(helper, listenerB); > 842: Same issue as above: `ExpressionHelper.removeListener(...)` may return a new helper instance, so the returned value should be assigned back to `helper` to keep subsequent operations consistent. ------------- PR Review: https://git.openjdk.org/jfx/pull/2200#pullrequestreview-4625824512 PR Review Comment: https://git.openjdk.org/jfx/pull/2200#discussion_r3519897375 PR Review Comment: https://git.openjdk.org/jfx/pull/2200#discussion_r3519897410 PR Review Comment: https://git.openjdk.org/jfx/pull/2200#discussion_r3519897446 PR Review Comment: https://git.openjdk.org/jfx/pull/2200#discussion_r3519897431
