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

[hjohn]

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

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change must be properly reviewed

Issue

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

Download

$ git fetch https://git.openjdk.java.net/jfx pull/198/head:pull/198 $ git checkout pull/198

[bridgekeeper[bot]]

:wave: 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[bot]]

Webrevs

• 01: Full - Incremental (011017b7)
• 00: Full (e3095db3662b008c17fae3dcb625f1d791273eef)

[kevinrushforth]

/reviewers 2

[openjdk[bot]]

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

[kevinrushforth]

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]

@arapte can you also review this?

[nlisker]

I will review this too anyway.

[kevinrushforth]

I will review this too anyway.

Thank you. That will be helpful.

[nlisker]

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]

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

[hjohn]

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]

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]

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]

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

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]

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]

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]

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]

    /**
     * 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]

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.

[openjdk[bot]]

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

• ca29cc6122010e4b94778cc658efd4fdddc8af67: 8298728: Cells in VirtualFlow jump after resizing
• e866a6c35c9040115dd62c6c04fd4b3d4d0e1324: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
• 0dbc4484c96859c6106bb4c62bff261c7e901aed: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
• 94fb7ede2dcde306a631d1feb0107c96a8221452: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
• 012fa16faccaf9b6a1f56cd3b5450ab200bdec9c: 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]

/integrate

[openjdk[bot]]

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

[mstr2]

/sponsor

[openjdk[bot]]

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

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

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@mstr2 @hjohn Pushed as commit bca1bfc5e3b04293c13417fd923d99864cdd9147.

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

[openjdk[bot]]

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