RFR: 8373908: XYChart (ScatteredChart) leaks memory when removing data [v2]

[John Hendrikx]

On Thu, 18 Dec 2025 02:19:03 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Fixes a problem with leaked references in XYChart.  A few things conspire here to make this into a memory leak:
>> 
>> - The weak bindings used by StringBinding leave behind listener "stubs" when GC'd; that's just how they work, it is a "strong" listener that wraps a weak referenced listener.  The strong part remains behind, and is cleaned up when a new listener is added/removed by ExpressionHelper (and if that never happens, those stubs remain there indefinitely).
>> - The fluent bindings (map/flatMap/orElse) use normal listeners, but only when they are observed themselves (lazy listeners)
>> - The "stub" that is left behind counts as being observed, so the fluent bindings don't unsubscribe themselves
>> 
>> The leak has nothing to do with the node or the accessible property, but purely by the StringBinding leaving behind stubs on the flat mapped properties.
>> 
>> The leak is actually because the Series to which the Data object belongs is referencing the Data object.  The flatMaps track the series object so they added a listener to the series object, and they think they are observed indefinitely because of the listener stub.
>> 
>> The easiest solution here is to ensure the Series object is not tracked when not needed; this can be achieved by setting the series to `null` in the ListChangeListener for the Data list.
>
> John Hendrikx has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   Fix memory leak

> Excellent work, the memory leak is fixed, thank you so much @hjohn !
> 
> <img alt="Screenshot 2025-12-18 at 08 36 17" width="706" height="421" src="https://private-user-images.githubusercontent.com/107069028/528205055-f555a51d-c446-4a55-bbbf-469afddb9f68.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NjYwOTYyNjIsIm5iZiI6MTc2NjA5NTk2MiwicGF0aCI6Ii8xMDcwNjkwMjgvNTI4MjA1MDU1LWY1NTVhNTFkLWM0NDYtNGE1NS1iYmJmLTQ2OWFmZGRiOWY2OC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUxMjE4JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MTIxOFQyMjEyNDJaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT03MzcxNTM2MmYwYzg3MDAyZTE5ZmVjMTNmODI4ZmI1YTBiNmUwYWY2YjgxNTdlMWVmYWY5MzZlNTA2ZGJhZjZlJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.2d_UxIX29_PuyyDeziBKZiBvQxklLtPfQKyZHIlWUFc">
> Questions:
> 
> 1. the seriesProperty holds the same object as the series field, can we remove the field?

You added it at some point :) It can be removed, but you'll need use the `get()` method on the property in a lot of places then.  I don't see any harm either way, so let me know what you want.

> 2. could we apply the same treatment to PieChart (perhaps in a followup, unless it's an easy fix)?

I didn't look very far, I looked at the original change and didn't see any use of `flatMap` there.  Is there a problem with it?

> 3. perhaps, as a followup, we ought to add a parameterized test to make sure all other charts don't exhibit the same issue?

Yeah perhaps, or we could take a look at least if they were using a fluent binding + weak binding combination anywhere.  I only looked at the original change, and I saw XYChart and PieChar were affected, but only XYChart was using new bindings.

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

PR Comment: https://git.openjdk.org/jfx/pull/2013#issuecomment-3672479279