8243115: Spurious invalidations due to bug in IntegerBinding and other classes #198

hjohn · 2020-04-27T11:43:28Z

This fixes a bug where the first call to unbind would clear the internal invalidation listener used, resulting in subsequent unbind calls to be no-ops, unless bind was called again first.

I had to rewrite the parameterized test slightly as Parameterized will only call the parameters method once, and my new test modifies the internal state of the bindings used as parameters (by doing some unbind calls) which was making other tests fail.

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-8243115: Spurious invalidations due to bug in IntegerBinding and other classes

Reviewers

Ambarish Rapte (@arapte - Reviewer)
Michael Strauß (@mstr2 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 198

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

Using diff file

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

This fixes a bug where the first call to unbind would clear the internal invalidation listener used, resulting in subsequent unbind calls to be no-ops, unless bind was called again first.

bridgekeeper · 2020-04-27T11:44:37Z

👋 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.

mlbridge · 2020-04-27T11:48:24Z

Webrevs

01: Full - Incremental (011017b7)
00: Full (e3095db)

kevinrushforth · 2020-04-27T13:13:04Z

/reviewers 2

openjdk · 2020-04-27T13:13:20Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

kevinrushforth · 2020-04-27T13:13:44Z

The title of this PR should match exactly the title of the JBS bug. So:

8243115: Spurious invalidations due to bug in IntegerBinding and other classes

kevinrushforth · 2020-04-27T13:19:49Z

@arapte can you also review this?

nlisker · 2020-04-27T23:32:04Z

I will review this too anyway.

kevinrushforth · 2020-04-28T00:00:28Z

I will review this too anyway.

Thank you. That will be helpful.

nlisker · 2020-05-11T22:27:27Z

As I started my review I noticed that unbind does not null-check its argument dependencies like bind does and it can lead to NPEs. If it is out of scope for this PR to fix this, a new issue should be filed.

nlisker · 2020-05-12T15:56:32Z

The fix looks correct and the tests pass. I just wonder why the change to reflection-based construction with bindingMockClassConstructor?

hjohn · 2020-05-12T17:47:20Z

The fix looks correct and the tests pass. I just wonder why the change to reflection-based construction with bindingMockClassConstructor?

The Parameterized test constructs some standard Binding objects to run multiple tests with. This works fine if those objects are immutable (or aren't modified during the tests). My new test however does modify them, and other tests would fail with such modified objects (as unbind was called on some). So I rewrote this a little bit to construct fresh objects for each test, and for that I used some reflection magic to avoid having to write a specific test for each of the 8 binding types.

hjohn · 2020-05-12T17:58:09Z

As I started my review I noticed that unbind does not null-check its argument dependencies like bind does and it can lead to NPEs. If it is out of scope for this PR to fix this, a new issue should be filed.

I'm fine with doing a fix, but I need to know which one. Avoiding NPE's and silently doing nothing is IMHO not very desirable as this will give the user of the API no feedback that something went wrong.

So I would prefer to fix this by documenting that these cases will result in a NPE.

The bind method has a similar issue -- it doesn't check its array elements for null, and will throw a NPE when attempting to add a listener to null. Again, I would just document the NPE so what is clearly a mistake doesn't go unnoticed.

nlisker · 2020-05-12T22:41:14Z

I'm fine with doing a fix, but I need to know which one. Avoiding NPE's and silently doing nothing is IMHO not very desirable as this will give the user of the API no feedback that something went wrong.

So I would prefer to fix this by documenting that these cases will result in a NPE.

The bind method has a similar issue -- it doesn't check its array elements for null, and will throw a NPE when attempting to add a listener to null. Again, I would just document the NPE so what is clearly a mistake doesn't go unnoticed.

I think that checking the array elements doesn't help much because by the time you can check them they will already be used, and that will throw an NPE implicitly.

bind is no-op for null or 0-length arrays, but should have probably throw an NPE on the null case. The 0-length check saves creating the observer, so I think it makes sense. unbind should probably only throw an NPE on null arrays, but that happens implicitly anyway, so maybe there is no change needed unless it's for clarity because we can add a custom error message.

hjohn · 2022-10-18T06:14:19Z

@kevinrushforth I was going through my open JDK tickets, and saw that this slipped through the cracks.

This ticket would need another reviewer still, @arapte could you take a look?

kevinrushforth · 2022-10-18T12:55:36Z

Yes, it did fall through the cracks. I wasn't sure it was still relevant, but since it is, I'll put it on my queue.

Perhaps either @arapte or @nlisker can be the second reviewer.

arapte · 2023-01-03T09:46:51Z

Looks good to me. Tested on Windows10 and verified that not setting observer to null does not lead to any leak.
Please merge with latest master to trigger a GitHub build and test.

Under a different bug, should we implement the dispose() method? Track all observables in a Weak list and remove observer from them in dispose()

hjohn · 2023-01-03T10:12:47Z

I'm fine with doing a fix, but I need to know which one. Avoiding NPE's and silently doing nothing is IMHO not very desirable as this will give the user of the API no feedback that something went wrong.
So I would prefer to fix this by documenting that these cases will result in a NPE.
The bind method has a similar issue -- it doesn't check its array elements for null, and will throw a NPE when attempting to add a listener to null. Again, I would just document the NPE so what is clearly a mistake doesn't go unnoticed.

I think that checking the array elements doesn't help much because by the time you can check them they will already be used, and that will throw an NPE implicitly.

(I must have missed this comment) @nlisker

What I meant here was to document this behavior, not to add a null check. So for bind:

    /**
     * Start observing the dependencies for changes. If the value of one of the
     * dependencies changes, the binding is marked as invalid.
     *
     * @param dependencies
     *            the dependencies to observe
+    * @throws NullPointerException when dependencies is null, or any of its elements was null
     */
    protected final void bind(Observable... dependencies) {
-       if ((dependencies != null) && (dependencies.length > 0)) {
+       if (dependencies.length > 0) {
            if (observer == null) {
                observer = new BindingHelperObserver(this);
            }
            for (final Observable dep : dependencies) {
                dep.addListener(observer);
            }
        }
    }

And if you want to be even more specific, we could add that when a NPE is thrown, the result is undefined (as some dependencies may have been added already). I don't think we want to specify that case.

bind is no-op for null or 0-length arrays, but should have probably throw an NPE on the null case. The 0-length check saves creating the observer, so I think it makes sense. unbind should probably only throw an NPE on null arrays, but that happens implicitly anyway, so maybe there is no change needed unless it's for clarity because we can add a custom error message.

I don't think we should throw a specific exception (or add a message), only documentation. IMHO, passing null anywhere in any form, is always incorrect and doesn't require further explanation unless explicitly allowed in the documentation.

hjohn · 2023-01-03T10:34:11Z

Looks good to me. Tested on Windows10 and verified that not setting observer to null does not lead to any leak. Please merge with latest master to trigger a GitHub build and test.

Thanks, I've merged in master.

Under a different bug, should we implement the dispose() method? Track all observables in a Weak list and remove observer from them in dispose()

Perhaps, currently only more specific implementations (provided mainly by Bindings) do this.

I think it was left up to the subclass to decide whether this would be worth it in order to keep bindings as light weight as possible. dispose certainly makes no promises in that regard (it basically makes no promises at all after reading the docs).

arapte · 2023-01-06T09:34:18Z

    /**
     * Start observing the dependencies for changes. If the value of one of the
     * dependencies changes, the binding is marked as invalid.
     *
     * @param dependencies
     *            the dependencies to observe
+    * @throws NullPointerException when dependencies is null, or any of its elements was null
     */
    protected final void bind(Observable... dependencies) {
-       if ((dependencies != null) && (dependencies.length > 0)) {
+       if (dependencies.length > 0) {
            if (observer == null) {
                observer = new BindingHelperObserver(this);
            }
            for (final Observable dep : dependencies) {
                dep.addListener(observer);
            }
        }
    }

I would not recommend to make this change as it may break any existing app, even though the app is at wrong to pass null Or atleast not to make this change as part of this PR, we can discuss it separately under a new bug.

hjohn · 2023-01-06T09:55:01Z

I would not recommend to make this change as it may break any existing app, even though the app is at wrong to pass null Or atleast not to make this change as part of this PR, we can discuss it separately under a new bug.

No, definitely not part of this fix, I was just replying to Nir.

arapte

LGTM

mstr2

The patch looks good, and I can confirm that it fixes the bug.

openjdk · 2023-01-09T05:42:29Z

@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:

8243115: Spurious invalidations due to bug in IntegerBinding and other classes

Reviewed-by: arapte, mstrauss

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 5 new commits pushed to the master branch:

ca29cc6: 8298728: Cells in VirtualFlow jump after resizing
e866a6c: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
0dbc448: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
94fb7ed: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
012fa16: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing

Please see this link for an up-to-date comparison between the source branch of this pull request and the master 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 (@arapte, @mstr2) 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 · 2023-01-09T08:12:51Z

/integrate

openjdk · 2023-01-09T08:26:06Z

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

mstr2 · 2023-01-09T14:29:56Z

/sponsor

openjdk · 2023-01-09T14:48:08Z

Going to push as commit bca1bfc.
Since your change was applied there have been 5 commits pushed to the master branch:

ca29cc6: 8298728: Cells in VirtualFlow jump after resizing
e866a6c: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
0dbc448: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
94fb7ed: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
012fa16: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-09T14:49:15Z

@mstr2 @hjohn Pushed as commit bca1bfc.

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

openjdk · 2023-01-09T14:50:41Z

@mstr2 The command sponsor can only be used in open pull requests.

8243115: Unregister bindings when unbind called multiple times

e3095db

This fixes a bug where the first call to unbind would clear the internal invalidation listener used, resulting in subsequent unbind calls to be no-ops, unless bind was called again first.

openjdk bot added the rfr Ready for review label Apr 27, 2020

kevinrushforth self-requested a review April 27, 2020 13:19

kevinrushforth assigned hjohn Apr 27, 2020

hjohn changed the title ~~8243115: Unregister bindings when unbind called multiple times~~ 8243115: Spurious invalidations due to bug in IntegerBinding and other classes Apr 27, 2020

kevinrushforth requested a review from arapte December 20, 2022 21:51

Merge branch 'master' of https://git.openjdk.org/jfx into feature/unbind

011017b

arapte approved these changes Jan 9, 2023

View reviewed changes

mstr2 approved these changes Jan 9, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 9, 2023

openjdk bot added the sponsor Ready to sponsor label Jan 9, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 9, 2023

openjdk bot closed this Jan 9, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Jan 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8243115: Spurious invalidations due to bug in IntegerBinding and other classes #198

8243115: Spurious invalidations due to bug in IntegerBinding and other classes #198

hjohn commented Apr 27, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 27, 2020

mlbridge bot commented Apr 27, 2020 •

edited

Loading

kevinrushforth commented Apr 27, 2020

openjdk bot commented Apr 27, 2020

kevinrushforth commented Apr 27, 2020

kevinrushforth commented Apr 27, 2020

nlisker commented Apr 27, 2020

kevinrushforth commented Apr 28, 2020

nlisker commented May 11, 2020

nlisker commented May 12, 2020

hjohn commented May 12, 2020

hjohn commented May 12, 2020

nlisker commented May 12, 2020

hjohn commented Oct 18, 2022

kevinrushforth commented Oct 18, 2022

arapte commented Jan 3, 2023

hjohn commented Jan 3, 2023 •

edited

Loading

hjohn commented Jan 3, 2023

arapte commented Jan 6, 2023

hjohn commented Jan 6, 2023

arapte left a comment

mstr2 left a comment

openjdk bot commented Jan 9, 2023

hjohn commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

mstr2 commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

8243115: Spurious invalidations due to bug in IntegerBinding and other classes #198

8243115: Spurious invalidations due to bug in IntegerBinding and other classes #198

Conversation

hjohn commented Apr 27, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 27, 2020

mlbridge bot commented Apr 27, 2020 • edited Loading

Webrevs

kevinrushforth commented Apr 27, 2020

openjdk bot commented Apr 27, 2020

kevinrushforth commented Apr 27, 2020

kevinrushforth commented Apr 27, 2020

nlisker commented Apr 27, 2020

kevinrushforth commented Apr 28, 2020

nlisker commented May 11, 2020

nlisker commented May 12, 2020

hjohn commented May 12, 2020

hjohn commented May 12, 2020

nlisker commented May 12, 2020

hjohn commented Oct 18, 2022

kevinrushforth commented Oct 18, 2022

arapte commented Jan 3, 2023

hjohn commented Jan 3, 2023 • edited Loading

hjohn commented Jan 3, 2023

arapte commented Jan 6, 2023

hjohn commented Jan 6, 2023

arapte left a comment

Choose a reason for hiding this comment

mstr2 left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 9, 2023

hjohn commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

mstr2 commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

hjohn commented Apr 27, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 27, 2020 •

edited

Loading

hjohn commented Jan 3, 2023 •

edited

Loading