> 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. ------------- Added commits: - c8d04111: Fix test based on review comments Changes: - all: https://git.openjdk.java.net/jfx/pull/113/files - new: https://git.openjdk.java.net/jfx/pull/113/files/1097ace6..c8d04111 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/113/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/113/webrev.01-02 Stats: 119 lines in 1 file changed: 22 ins; 85 del; 12 mod Patch: https://git.openjdk.java.net/jfx/pull/113.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/113/head:pull/113 PR: https://git.openjdk.java.net/jfx/pull/113