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

[John Hendrikx]

On Wed, 11 Jan 2023 13:09:37 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

> > Should I expect x here to be unreferenced? No, that's how properties work, b will keep its reference to x.
> 
> No, x is not dereferenced. But you should expect that a is no longer referencing b, but both a and b are referencing x.

I missed that the test was referencing only the properties, although I suppose the "list like behavior" people might find it surprising to see the previously bound property now suddenly referencing a different `ObservableList`. If that `ObservableList` is supposed to be short lived, then that might be a new kind of memory leak (but nothing that wasn't there before).

The above written as a test, people may find this surprising behavior:

    @Test
    public void testListsDontCrossPolinate() {
        ObservableList<Object> a = FXCollections.observableArrayList("A");
        ObservableList<Object> b = FXCollections.observableArrayList("A", "B", "C");

        SimpleListProperty<Object> propA = new SimpleListProperty<>(a);
        SimpleListProperty<Object> propB = new SimpleListProperty<>(b);

        propA.bind(propB);

        assertEquals(3, propA.get().size());  // Note: if you remove this line, the old code fails badly...

        propA.unbind();

        propA.get().add("A");

        assertEquals(4, propA.get().size());
        assertEquals(4, propB.get().size());
    }

> If you think the PR creates an unwanted behavior, I would appreciate seeing a **unit-test** for it, because it's way easier to discuss on the basis of a test.

Here is one that passed with the old version, which highlights the standard gotcha's of all uses of weak references:

    @Test
    public void testWeakRefDoesntDisappearUnexpectedly() {
        List<Object> changes = new ArrayList<>();

        ObservableList<Object> observableList = FXCollections.observableArrayList();

        SimpleListProperty<Object> simpleListProperty = new SimpleListProperty<>(observableList);
        simpleListProperty.addListener((obs, old, current) -> changes.add(current));

        simpleListProperty = null;

        observableList.add("A");

        assertEquals(1, changes.size());

        System.gc();

        observableList.add("B");

        assertEquals(2, changes.size());
    }

This test highlights the following issues:
- Weak references don't disappear instantly; they may disappear at any time or never, at the digression of your JVM; the first change we do to `observableList` may be picked up or not, it's unpredictable.  In real applications, this exhibits itself as "works on my machine" or "seems to work for a while, but then breaks".
- The second change doesn't get picked up, but it still might.  It depends on the mood of the garbage collector.  This can show up in real applications as phantom changes being triggered by objects that are supposedly no longer in use.

And finally, why would we expect that the outcome is anything but what the test says?  I registered a listener, I didn't unregister it, it's there, it's registering changes made to an observable list and putting those in another list for later use.  Why would this test fail at all?  Explaining what's happening here to JavaFX novices is impossible. We're relying here on a fundamental JVM system, that is largely unspecified in how it behaves exactly (and with good reason), for a very basic but crucial feature.

The way I see it, `ListProperty` should work as follows:

1) When created, it registers nothing to anything; this will allow it to go out of scope (currently it can't, as it registers on `ObservableList` which will point back to it).

2) When any listener is registered to it (or to its `empty` or `size` properties), it registers on the underlying `ObservableList`; when all listeners are gone, it unregisters from the underlying `ObservableList`.  This is what you would want; if I'm listening to something, I don't want this to disappear for any reason.

3) `bind` / `unbind` can stay as they are; when you bind to another property, that other property gets a listener and registers with its underlying `ObservableList`.  When `unbind` gets called, that listener is removed again (and potentially, the other property unregisters from its underlying `ObservableList` if that was the only listener).  Before this would break, as the `ObservableList` would still be pointing to the other list property (as a listener is present) and the now unbound property, which also refers that `ObservableList` now (whether that's good or bad) will then keep an indirect reference to that other property (previously bound property -> observable list -> other property).

No weak listeners are needed anywhere now, and everything will be nice and predictable.

I'll attempt to make a PoC for this.

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

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