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 I took a closer look at the change, and I think it is fairly harmless (famous last words :) ). The `currentValue` is a field that is sometimes updated when the listener lists changes. Specifically, it is `null`ed (so it isn't referring to something that could be GC'd) when all change listeners are removed. If that happens while a notification is in progress, then instead of the clearly intended value, `null` is sent because every listener callback uses the field's value directly. Capturing the value at the start of the loop solves this. ------------- Marked as reviewed by jhendrikx (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/2200#pullrequestreview-4705508783
