On Fri, 14 Feb 2020 14:23:45 GMT, Kevin Rushforth <[email protected]> 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.
>
> The pull request has been updated with 1 additional commit.
modules/javafx.base/src/test/java/test/javafx/beans/property/ObjectPropertyLeakTest.java
line 111:
> 110: origRefs.add(new WeakReference<>(origProp));
> 111: wrappedRefs.add(new WeakReference<>(wrappedProp));
> 112: }
I suggest extracting this section into its own method since it's being used in
all other tests.
-------------
PR: https://git.openjdk.java.net/jfx/pull/113
