On Fri, 14 Feb 2020 14:23:45 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. > > The pull request has been updated with 1 additional commit. modules/javafx.base/src/test/java/test/javafx/beans/property/ObjectPropertyLeakTest.java line 73: > 72: : "Original properties should be GCed"; > 73: assertEquals(origMsg, origExpected, origCount); > 74: As long as I'm, modifying the test, I'll also move the above block, which is repeated twice, into a separate method. ------------- PR: https://git.openjdk.java.net/jfx/pull/113