8290331 Binding value left null when immediately revalidated in invalidation listener

[hjohn]

I introduced a bug with the fluent bindings PR which affects all ObjectBindings.

This is the code that fails:

    SimpleObjectProperty<Boolean> condition = new SimpleObjectProperty<>(true);
    ObservableValue<String> binding = condition.map(Object::toString);

    binding.addListener(o -> { binding.getValue(); });

    condition.set(false);

    assertEquals(false, binding.getValue());  // returns null (!)

This PR fixes this problem and adds a test case to cover it.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8290331: Binding value left null when immediately revalidated in invalidation listener

Reviewers

• Michael Strauß (@mstr2 - Author)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/829/head:pull/829
$ git checkout pull/829

Update a local copy of the PR:
$ git checkout pull/829
$ git pull https://git.openjdk.org/jfx pull/829/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 829

View PR using the GUI difftool:
$ git pr show -t 829

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/829.diff

[bridgekeeper]

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 01: Full - Incremental (34856569)
• 00: Full (4c8453af)

[kevinrushforth]

Good catch. And yes, this needs to go into JavaFX 19.

/reviewers 2

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[mstr2]

Looks good to me.

[nlisker]

/issue add JDK-8206449

[openjdk]

@nlisker Only the author (@hjohn) is allowed to issue the /issue command.

[hjohn]

8206449

Ah, nice, looks like that one can be closed then

[nlisker]

@kevinrushforth Any idea why /issue add is only available to the author? If I can go to JBS and close it as a duplicate, it makes sense that I could mark it as such here. Is there some conceptual point I'm missing?

[kevinrushforth]

Any idea why /issue add is only available to the author?

This is deliberate. Commands like /issue and /summary are under the purview of the Author of the PR, since they affect the commit message associated with this PR.

In any case, it is not correct to add JDK-8206449 as an additional issue fixed by this PR. Rather, JDK-8206449 is a duplicate of the previously integrated JDK-8274771.

[nlisker]

Alright, I closed JDK-8206449 as duplicated of JDK-8274771.

[kevinrushforth]

Looks good.

[openjdk]

@hjohn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290331: Binding value left null when immediately revalidated in invalidation listener

Reviewed-by: mstrauss, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 9 new commits pushed to the jfx19 branch:

• b8302f6: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed
• eb8e9ef: 8289606: CustomSecurityManagerTest fails on Mac M1
• b9a1ec5: 8288137: The set of available printers is not updated without application restart
• 0132ac8: 8284676: TreeTableView loses sort ordering when applied on empty table
• 8d5d3ca: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses
• 437c078: 8289255: update Eclipse .classpath and other configuration files
• 13f3e1f: 8289395: Fix warnings: Varargs methods should only override or be overridden by other varargs methods
• c7d3fd9: 8283402: Update to gcc 11.2 on Linux
• cbb53b2: 8289952: Visual Studio libs msvcp140_1.dll and msvcp140_2.dll missing from build

Please see this link for an up-to-date comparison between the source branch of this pull request and the jfx19 branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[hjohn]

/integrate

[openjdk]

@hjohn
Your change (at version 3485656) is now ready to be sponsored by a Committer.

[kevinrushforth]

/sponsor

[openjdk]

Going to push as commit 4959f1b.
Since your change was applied there have been 9 commits pushed to the jfx19 branch:

• b8302f6: 8277785: ListView scrollTo jumps to wrong location when CellHeight is changed
• eb8e9ef: 8289606: CustomSecurityManagerTest fails on Mac M1
• b9a1ec5: 8288137: The set of available printers is not updated without application restart
• 0132ac8: 8284676: TreeTableView loses sort ordering when applied on empty table
• 8d5d3ca: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses
• 437c078: 8289255: update Eclipse .classpath and other configuration files
• 13f3e1f: 8289395: Fix warnings: Varargs methods should only override or be overridden by other varargs methods
• c7d3fd9: 8283402: Update to gcc 11.2 on Linux
• cbb53b2: 8289952: Visual Studio libs msvcp140_1.dll and msvcp140_2.dll missing from build

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinrushforth @hjohn Pushed as commit 4959f1b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Mailing list message from John Hendrikx on openjfx-dev:

I think the issue in JDK-8206449 might not be entirely resolved.

Even though `value` is now cleared in `ObjectBinding`, I noticed that
`ExpressionHelper` also maintains the current value when there are
change listeners registered. Unfortunately, it forgets to clear this
when all change listeners are removed but there are still sufficient
invalidation listeners registered for it to need the Generic
ExpressionHelper implementation.

I think an overhaul of ExpressionHelper might be warranted:

- Doesn't clear current value when it should
- The locking done makes a new copy of the listener lists each time if
there are multiple changes to these lists during an event emission (this
can happen easily when many listener stubs have gone out of scope)
- Change event emission should probably not be dependent on the order of
listeners, see https://bugs.openjdk.org/browse/JDK-8290310
- Further optimizations are possible:
?? - Better remove performance -- although there is no JDK collection
class that has fast remove(T) while allowing duplicates, it isn't hard
to construct a specialized class that has fast `addLast`, fast
`remove(T)` and fast iteration
?? - Single invalidation listener could just use the helper field
directly (instanceof check can determine if it is an invalidation
listener or a helper instance); this can save quite some memory as the
fast majority of properties/bindings may never have more than 1 listener
?? - Single change maybe as well (requires current value to be passed
into #fireValueChangedEvent, which is how most other ExpressionHelpers,
except the base ExpressionHelper,? are done) -- note that many, if not
all, users of ExpressionHelper often also have a copy of the current
value maintained separately...
?? - Small listener lists could use a single array (using instanceof to
distinguish between invalidation/change)

I also find that there is a test for ExpressionHelper, testing a ton of
edge cases, but I think this class shouldn't be tested at all.? Instead,
a test should be there to test the behavior of classes implementing
Observable and ObservableValue, regardless of whether they use
ExpressionHelper internally. Such a test would be reusable, regardless
of how these classes manage their listeners, or if some decide to not
use `ExpressionHelper`.

A good JUnit test should I think be constructed first, validating the
current behavior based on classes implementing Observable and
ObservableValue; these should be thorough enough to cover most (if not
all) of the edge cases now tested in ExpressionHelperTest.

With any luck, the test can be written in such a way that it can be
parameterized on the type of class implementing ObservableValue,
allowing a single test to test behavior of all *Property classes and all
*Binding classes.

--John

On 15/07/2022 16:09, Nir Lisker wrote: