On Fri, 18 Jul 2025 03:34:49 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:

> > Synchronized methods might be OK without, but the following methods are not 
> > synchronized, and access cfDictionaryPtr:
> 
> Unfortunately this is actually true, in awt we even have a special class 
> [CFRetainedResource](https://github.com/openjdk/jdk/blob/04c0b130f09c093797895cc928fe020d7e584cb9/src/java.desktop/macosx/classes/sun/lwawt/macosx/CFRetainedResource.java#L36)
>  which maintains a locks around the native pointer and prevents its usage 
> after de-allocation(intentional or via finalize). It was implemented as part 
> of JDK-8164143.
> 
> I can migrate the current API of DIsposer to the something similar to 
> CFRetainedResource as a follow up bug. The current one did not introduce the 
> new issues, since implementation via finalize has the same bug.

What does that mean ?

CFRetainedResource is extended by a lot of classes and it is hard to figure out 
how correctly it is used.
I would take a big step back and look before extending what it does.
And it also uses finalize which is why I looked and found what a mess it is.

> I would recommend adding a `reachabiltyFence()` to `Disposer.addRecord()`, as 
> @mrserb 
> [suggests](https://github.com/openjdk/jdk/pull/26331/files#r2208586134).
> 
> Unexpected unreachability is a subtle problem with GC-assisted APIs - 
> finalizers, `Cleaner`, and even direct use of `References` & 
> `ReferenceQueues`. In particular, the VM can decide than object is 
> unreachable even when a method on that object is still running.
> 
> For this reason, our standard recommendation is that any instance method that 
> accesses the cleanable state (in this case, `cfDictionaryPtr`) should be 
> enclosed with a `try{...}finally{ reachabilityFence(); }` block.
> 
> Synchronized methods _might_ be OK without, but the following methods are not 
> synchronized, and access `cfDictionaryPtr`:
> 
> * `getHitForPoint()`
> * `getPartBounds()`
> * `getScrollBarOffsetChange()`
> * `sync()`

I do not see any scenario in which this is needed.
I will evaluate if we need to  file a bug to use reachabiltyFence in the 
Disposer creation since that is a one-time contained thing but  I have no 
reason to suppose it is needed in this class.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/26331#issuecomment-3100996792
PR Comment: https://git.openjdk.org/jdk/pull/26331#issuecomment-3101010453

Reply via email to