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 [8290310: ChangeListener events are incorrect or misleading when a > nested change occurs #1081](https://github.com/openjdk/jfx/pull/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? `ExpressionHelper` won't disappear entirely yet with #1081 -- it is still used by the Bean adapters and (surprisingly) in `TextInputControl`. I could include a full removal in #1081 -- but as that PR is already tough to review, I think that's best done as a follow up. So, with that in mind, I think we can include this "more correct" solution, and it will disappear automatically with the complete removal of `ExpressionHelper`. If #1081 is indeed in early in JFX 28, then I think it is likely `ExpressionHelper` can be fully removed before 28 is released. ------------- PR Comment: https://git.openjdk.org/jfx/pull/2200#issuecomment-4982121014
