> 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:
 - 1097ace6: Add final in a few missing places in the test for symmetry

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/113/files
  - new: https://git.openjdk.java.net/jfx/pull/113/files/e90abed6..1097ace6

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/113/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/113/webrev.00-01

  Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 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

Reply via email to