RFR: 8277848 Binding and Unbinding to List leads to memory leak [v7]

[John Hendrikx]

On Fri, 19 Aug 2022 10:40:01 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> Making the initial listener of the ListProperty weak fixes the problem.
>> The same is fixed for Set and Map.
>> Due to a smart implementation, this is done without any performance drawback.
>> (The trick is to have an object, which is both the WeakReference and the Changelistener)
>> By implying the same trick to the InvalidationListener, this should even improve the performance of the collection properties.
>
> Florian Kirmaier has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'origjfx/master' into JDK-8277848-list-binding-leak
>  - JDK-8277848
>    Added tests to ensure no premature garbage collection is happening.
>  - JDK-8277848
>    Added 3 more tests to verify that a bug discussed in the PR does not appear.
>  - JDK-8277848
>    Added the 3 requests whitespaces
>  - JDK-8277848
>    Further optimization based code review.
>    This Bugfix should now event improve the performance
>  - JDK-8277848
>    Added missing change
>  - JDK-8277848
>    Fixed memoryleak, when binding and unbinding a ListProperty. The same was fixed for SetProperty and MapProperty.

My question is the following. Isn't this expected behavior?

Let's say I have this program:

     X x = new X();
     ObjectProperty<X> a = new SimpleObjectProperty<>(x);
     ObjectProperty<X> b = new SimpleObjectProperty<>();

     b.bind(a);   // b takes on the value of a (x)
     b.unbind(); // b keeps last value (x)

     a = null;
     x = null;

Should I expect `x` here to be unreferenced? No, that's how properties work, `b` will keep its reference to `x`.

Now replace the above program with `X` being an `ObservableList` and the `ObjectProperty`s being `ListProperty`s.  Should the expected behavior change?

I think there is a far more fundamental issue with how `ListProperty`s behave that's poorly defined and documented.  While investigating, I found the main culprit to the `bind`/`unbind` test (`testBindingLeak`) to be the copy that is made in the `unbind` method:

    @Override
    public void unbind() {
        if (observable != null) {
            value = observable.getValue();   // <<< cause of "memory leak"
            observable.removeListener(listener);
            observable = null;
        }
    }

That line makes total sense from a property perspective; when a property is unbound, it copies the value of its source as the last known value.  Should this perhaps make a deep copy?  Perhaps it should, but these properties are so poorly defined that it is anyone's guess at what it should be doing here.

IMHO there is a conflict between the property like behavior of `ListProperty` and the list like behavior of the `ObservableList` that it wraps.  On the one hand, a list property is a reference to a list, and on the other hand we expect it to behave as only referring to the contents of the list.  The property like behavior that it inherits (perhaps it shouldn't have done that) has been subverted in that adding a change listener will pretend that a change to the list is the same as replacing the entire list object, so it is not quite a property any more.  You can see this in the fact that the "old value" of a change listener is incorrect:

        ObservableList<Object> list = FXCollections.observableArrayList();
        SimpleListProperty<Object> simpleListProperty = new SimpleListProperty<>(list);

        simpleListProperty.addListener((obs, old, current) -> System.out.println("Changed: " + old + " -> " + current));

        list.add("B");
        simpleListProperty.set(FXCollections.observableArrayList("C"));

Prints:

     Changed: [B] -> [B]    // huh? so nothing changed then? but it did...
     Changed: [B] -> [C]

This is frustrating as change listener has very specific semantics that code would like to rely on, but here, again, the old value is just a best effort guess (there are other bugs in this area).

I think that because of these poorly defined semantics, one will keep finding surprises in the behavior of these properties, depending on your perspective (property or list).  I think therefore that the first thing that's need to be done is to clearly define how these observable wrapper properties are supposed to work before fixing what may or may not be bugs in this area.  I fear it may not be possible to have `ListProperty` behave property and list like at the same time, and if so this should be clearly marked in the documentation that `ListProperty` does not obey the general contract of `Property`.

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

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