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

[John Hendrikx]

On Tue, 12 May 2020 22:41:14 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

> > 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)

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.

-------------

PR: https://git.openjdk.org/jfx/pull/198