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 We have two choices for this PR: 1. Proceed with this PR for jfx27. Even though it is at best an incomplete fix for the problem, it does avoid an NPE in a specific use case 2. Abandon this PR and wait until jfx28 when the underlying problem will be fixed by #1081 Presuming this PR has no ill effects, that is, there are no cases where it misbehaves or has worse behavior than today, then 1 seems a reasonable option. Paraphrasing @mstr2, If we think this is "_more correct_" than what we have today, then we can proceed on the principle "that _more correct_ is better than _less correct_". Either way, we should continue to push on #1081 and get it in early in jfx28 -- some time this month would be great. I note that if we go with option 1, we would likely want to revert it as part of #1081, since it would no longer be necessary. @mstr2 @hjohn What is your recommendation? ------------- PR Comment: https://git.openjdk.org/jfx/pull/2200#issuecomment-4981506678
