8362289: [macOS] Remove finalize method in JRSUIControls.java #26331

prrace · 2025-07-15T19:57:56Z

Eliminate a finalize() method in the Swing macOS implementation.

I tested that the dispose method is being called by running this small test in combination with some 'println' statements in the source code (now removed)

import javax.swing.JTabbedPane;
import javax.swing.plaf.TabbedPaneUI;

public class CreateTabbedPaneUIStressTest {

   public static void main(String[] args) {
       for (int i=0; i<1000000000; i++) {
           JTabbedPane jtp = new JTabbedPane();
           TabbedPaneUI tpui = jtp.getUI();
       }
   }
}

I also monitored the process size using 'top'.
And when I commented out the native call that did the free and re-tested I saw process size grow.

Turning the above into a useful regression test doesn't seem possible to me.
Limiting Java heap is pointless (and I did use -Xmx20m) since it is a native resource that is to be freed and failure to dispose won't show up as a problem without taking down the machine.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8362289: [macOS] Remove finalize method in JRSUIControls.java (Bug - P4)

Reviewers

Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26331/head:pull/26331
$ git checkout pull/26331

Update a local copy of the PR:
$ git checkout pull/26331
$ git pull https://git.openjdk.org/jdk.git pull/26331/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26331

View PR using the GUI difftool:
$ git pr show -t 26331

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26331.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-07-15T19:59:14Z

👋 Welcome back prr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-07-15T20:00:26Z

@prrace This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8362289: [macOS] Remove finalize method in JRSUIControls.java

Reviewed-by: serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 18 new commits pushed to the master branch:

04c0b13: 8362565: ProblemList jdk/jfr/event/io/TestIOTopFrame.java
cab5159: 8361424: Eliminate Log methods mandatoryWarning() and mandatoryNote()
bd55d7a: 8362203: assert(state == nullptr || state->get_thread_oop() != nullptr) failed: incomplete state
... and 15 more: https://git.openjdk.org/jdk/compare/20b5f097773043068ce732e0fa30c2726273e40a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-07-15T20:01:19Z

@prrace The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

prrace · 2025-07-15T20:06:33Z

src/java.desktop/macosx/classes/apple/laf/JRSUIControl.java

+        }
+
+        public synchronized void dispose() {
+            if (cfDictionaryPtr == 0) {


although the constructor throws an exception if it can't initialize this and also the native method checks for 0 too,
I decided to leave this existing check out of an abundance of caution.
It ensures that the method is safe to be called twice.

mlbridge · 2025-07-15T20:09:55Z

Webrevs

mrserb · 2025-07-15T20:24:36Z

src/java.desktop/macosx/classes/apple/laf/JRSUIControl.java

@@ -109,16 +113,29 @@ public JRSUIControl(final boolean flipped){
        flipped = other.flipped;
        cfDictionaryPtr = getCFDictionary(flipped);
        if (cfDictionaryPtr == 0) throw new RuntimeException("Unable to create native representation");


Just a side topic to discuss: In the past I tried to look into this pattern and figure out is it possible for the GC to start cleaning the object after we allocate the native resource but before we register it with Disposer.addRecord. In such a case, the cleanup might never be executed.

Perhaps we should consider adding a reachabilityFence at the end of Disposer.addRecord, for both the DisposerRecord and the referent, to ensure they remain reachable. Similarly how it was done for cleaner:
https://hg.openjdk.org/jdk9/jdk9/jdk/rev/1f33e517236e
Or may be we can just use cleaner.

there is even a bug for Cleaner that the reachabilityFence should be moved to the end of register:
https://bugs.openjdk.org/browse/JDK-8291867
If there are no objectiosn I'll add it to the Disposer.addRecord in a separate bug.

I think that would be OK, meaning probably harmless. But I don't know if this case actually requires it.
Where I've seen a problem like this a Java object is used at an earlier point in the method and not again and so becomes eligible to be freed, but some native resource it held is needed later but is already freed along with its no longer references holder.

Here, if this object (the JRSUIControl) becomes unreachable during construction, and so the referent and disposer are also unreachable outside this object, they are still going to be live until we reach code in the constructor which uses them and since the referent and the disposer are passed into addRecord they'll be live there until stored.

But these cases are subtle, maybe I'm missing something. Can you expand on the scenario ?
Or maybe there's some other patterns where we use Disposer that it is needed ?

I think I understand why reachabilityFence is used here. That code first registers the object in the cleanup queue and then continues executing the rest of the constructor. As far as I understand, the object could be collected between the registration and the end of the constructor so the cleaner will be called on a partially constructed object.

If that assumption is correct then in the change above the code:
nativeMap = new HashMap<Key, DoubleValue>();
changes = new HashMap<Key, DoubleValue>();
might never be executed and the native disposer will be called before, not an issue in this particular case but good to know.
Or I missing something.

I think that's right. At first I thought you were referring to the constructor of the thing that wants to use the cleaner, but you mean the constructor of the Cleanable itself may be still under construction when the referent is collected and installing the reachabilityFence at the end of the constructor prevents that.

I tried to compare how Cleaner and Disposer are used. I found that in most cases Cleaner uses this as the object to track. But Disposer often uses a separate object like disposerReferent or some kind of anchor. This patch is one example. In some places we even replaced this with a different object. You can see that in the following RFR:
https://bugs.openjdk.org/browse/JDK-8030099
https://mail.openjdk.org/pipermail/swing-dev/2015-October/005053.html

I think I found the reason: https://bugs.openjdk.org/browse/JDK-8071507

In older versions of the JDK the Disposer had to use phantom references. Phantom references allow native cleanup to happen after the object is 100% no longer reachable. But this had a problem, the object stayed in memory until the reference was added to the queue and the code called clear.

To avoid keeping large AWT or Java2D objects in memory for too long the code used small helper objects as referents instead of using the real object.

Now the problem described in JDK-8071507 is fixed. So maybe we no longer need to use small disposer referents. And you can check that by the test from the description of this PR.

I know this is possible, but I'd prefer to keep this pattern. This way It is crystal clear that nothing to do with reference queues etc are delaying the GC from immediately reclaiming the memory.
Honestly, it probably matters not so much for JRSUIControls, and even less so for CGraphicsEnvironment, but may matter for some other cases.

A later follow-up fix could go change a whole bunch of these cases at once and we'd then have a good reference (pun intended) as to whether any issues showed up.

sounds good

src/java.desktop/macosx/classes/apple/laf/JRSUIControl.java

openjdk · 2025-07-16T21:11:46Z

@prrace this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout jrsuicontrols
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

src/java.desktop/macosx/classes/apple/laf/JRSUIControl.java

bchristi-git

I would recommend adding a reachabiltyFence() to Disposer.addRecord(), as @mrserb suggests.

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()

mrserb · 2025-07-18T03:34:49Z

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 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.

bchristi-git · 2025-07-18T18:51:39Z

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.

Sounds good. True, the issue already existed with the finalizer.

prrace · 2025-07-22T05:03:23Z

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 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.

prrace · 2025-07-22T05:06:03Z

I would recommend adding a reachabiltyFence() to Disposer.addRecord(), as @mrserb suggests.

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.

prrace · 2025-07-22T19:56:52Z

I would recommend adding a reachabiltyFence() to Disposer.addRecord(), as @mrserb suggests.
...

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.

I have at least filed the [enhancement] bug on this : https://bugs.openjdk.org/browse/JDK-8363886

8362289

f72b109

openjdk bot changed the title ~~8362289~~ 8362289: [macOS] Remove finalize method in JRSUIControls.java Jul 15, 2025

openjdk bot added the client [email protected] label Jul 15, 2025

8362289

130fdc5

openjdk bot added the rfr Pull request is ready for review label Jul 15, 2025

prrace commented Jul 15, 2025

View reviewed changes

mrserb reviewed Jul 15, 2025

View reviewed changes

src/java.desktop/macosx/classes/apple/laf/JRSUIControl.java Show resolved Hide resolved

8362289

2c56173

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jul 16, 2025

Merge branch 'master' into jrsuicontrols

21ad3a1

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Jul 16, 2025

mrserb reviewed Jul 16, 2025

View reviewed changes

src/java.desktop/macosx/classes/apple/laf/JRSUIControl.java Outdated Show resolved Hide resolved

JRSUIControl.java

3b4556c

bchristi-git suggested changes Jul 17, 2025

View reviewed changes

mrserb approved these changes Jul 18, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 18, 2025

8362289: [macOS] Remove finalize method in JRSUIControls.java #26331

Are you sure you want to change the base?

8362289: [macOS] Remove finalize method in JRSUIControls.java #26331

Uh oh!

Conversation

prrace commented Jul 15, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 15, 2025

Uh oh!

openjdk bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openjdk bot commented Jul 16, 2025

Uh oh!

Uh oh!

bchristi-git left a comment

Choose a reason for hiding this comment

Uh oh!

mrserb commented Jul 18, 2025

Uh oh!

bchristi-git commented Jul 18, 2025

Uh oh!

prrace commented Jul 22, 2025

Uh oh!

prrace commented Jul 22, 2025

Uh oh!

prrace commented Jul 22, 2025

Uh oh!

Uh oh!

prrace commented Jul 15, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 15, 2025 •

edited

Loading

mlbridge bot commented Jul 15, 2025 •

edited

Loading