On Fri, 3 Jul 2026 12:59:36 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). > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > assign back to helper As a reminder, per the [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai), please do not enable Copilot to post review comments to a PR in any repo the openjdk GitHub organization. You are welcome to use Copilot or other AI tools offline to review pull requests as long as you don't contribute code or comments generated by those tools. ------------- PR Comment: https://git.openjdk.org/jfx/pull/2200#issuecomment-4876654329
