On Thu, 13 Feb 2020 23:10:28 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> This patch removes the `finalize` methods from the `Property` classes in the
> `javafx.base` module.
>
> The `{Boolean,Double,Float,Int,Long}Property` classes each have a pair of
> methods -- `asObject` and `xxxxxProperty` -- that create a `Property<Xxxxx>`
> object from a primitive `XxxxxProperty` object and vice versa. Each of the
> methods bidirectionally binds the original property object to the created
> property object. All of the bidirectional bindings in question use
> `WeakReference`s to refer to the pair of objects being bound to each other,
> so that they won't cause a memory leak.
>
> The `finalize` methods were added in an attempt to cleanup the bidirectional
> bindings when the created object goes away. I say attempt, because it is
> entirely ineffective in doing so. The logic that removes the binding creates
> a new one from the same pair of objects, but fails to match the original
> binding because the referent to the created property object has been garbage
> collected before the `finalize` method is called; the `WeakReference` in the
> binding is cleared and no longer matches. Fortunately, the only impact of
> this ineffective logic is that it can delay the removal of the binding (which
> is a small object that just contains the two weak references) from the
> original property object until it (the original property) is either garbage
> collected or modified (the binding logic already looks for a referent that
> has been GCed and removes the binding).
>
> Given that the `finalize` methods don't do anything useful today, and that
> there is no memory leak in the first place, the proposed fix is to just
> remove the `finalize` methods entirely with no replacement needed. There will
> be no changes in behavior as a result of this.
>
> Existing tests of the methods in question are sufficient to ensure no
> functional regressions, although there is no existing test for leaks, so I
> created new tests to verify that there is no leak. Since there is no existing
> leak, those tests will pass both with and without this fix.
@arapte I request you to be one of the reviewers of this PR.
-------------
PR: https://git.openjdk.java.net/jfx/pull/113
