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 > > While I appreciate the effort, I think that a better use of the time spent > > fixing this edge case in ExpressionHelper would have been to review #1081. > > #1081 has a merge conflict and no movement. I'd rather see the platform > moving forward than wait for years for some grand improvement that never > materializes. Having said that, does #1081 fix this particular issue (no way > to check quickly, it seems to have a merge conflict). It has already materialized, the code exists. Framing John’s work as “no movement” is a bit rich when you (and others) have been stalling this PR for three years through non-participation. What a way to treat a community of volunteers... 😞 ------------- PR Comment: https://git.openjdk.org/jfx/pull/2200#issuecomment-4917647596
