8213531: Test javax/swing/border/TestTitledBorderLeak.java fails

[prsadhuk]

Test was failing in linux citing java.lang.RuntimeException: Expected Total TitledBorder to be freed : 10 Freed 9
As per the fix done in JDK-8204963 http://hg.openjdk.java.net/jdk/jdk/rev/cd7d2f9154fd
there was no platform specific code done for the fix and logs in TitledBorder.installPropertyChangeListeners shows it was called 10 times for the test but CleanerFactory.cleaner().register action was only called 9 times in linux causing it to fail.

Modified the test to not show the frame which cause the problem to go away and also it can still be used as 8204963 regression test as it still fails without the fix and pass with it. Modified test has passed in all platforms for several iterations.

Also removed the deprecated System.runFinalization

---

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-8213531: Test javax/swing/border/TestTitledBorderLeak.java fails

Reviewers

• @kristylee88 (no known github.com user name / role)
• Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8450/head:pull/8450
$ git checkout pull/8450

Update a local copy of the PR:
$ git checkout pull/8450
$ git pull https://git.openjdk.java.net/jdk pull/8450/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8450

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8450.diff

[bridgekeeper]

👋 Welcome back psadhukhan! 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]

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

[mlbridge]

Webrevs

• 02: Full - Incremental (ae7a4a4b)
• 01: Full - Incremental (8bf50cca)
• 00: Full (ea2c3e11)

[mrserb]

Modified the test to not show the frame which cause the problem to go away

Does it mean that setVisible(true) cause some memory leak since it prevents the window to be disposed?

[prrace]

Modified the test to not show the frame which cause the problem to go away

Does it mean that setVisible(true) cause some memory leak since it prevents the window to be disposed?

Since 9 windows are freed .. I doubt it .. but what's the hold up with #10 ?
I wonder if you need an extra System.gc() cycle ?

[mrserb]

Does it mean that setVisible(true) cause some memory leak since it prevents the window to be disposed?

Since 9 windows are freed .. I doubt it .. but what's the hold up with #10 ? I wonder if you need an extra System.gc() cycle

Then why we delete the "setVisible(true)"? probably we leak the latest visible window?

[prsadhuk]

Does it mean that setVisible(true) cause some memory leak since it prevents the window to be disposed?

Since 9 windows are freed .. I doubt it .. but what's the hold up with #10 ? I wonder if you need an extra System.gc() cycle

Then why we delete the "setVisible(true)"? probably we leak the latest visible window?

I dont think this is related to TitledBorder fix done in JDK-8204963.
Even if we remove TitledBorder code from the test and only keep JFrame, then also I see

java.lang.RuntimeException: Expected Total to be freed : 10 Freed 9

so I think it's a generic JFrame dispose issue with CleanerFactory in linux (last JFrame is not cleaned up by CleanerFactory) which can be worked upon in a separate PR.

As I see, TitledBorder leak is fixed and there's no point having the test problemlisted for linux due to JFrame issue.

[prrace]

Can you confirm we still need the Frame ? I mean when does the used-to-be-leaky listener get installed ?
If it is sufficient to add it to the panel and the panel doesn't need to be added to the frame then the test can be headless.
But we need to be sure that doesn't skip what we want to test.

[prsadhuk]

Can you confirm we still need the Frame ? I mean when does the used-to-be-leaky listener get installed ? If it is sufficient to add it to the panel and the panel doesn't need to be added to the frame then the test can be headless. But we need to be sure that doesn't skip what we want to test.

No, we dont need the frame. The leaky listener gets installed when we call new TitledBorder()

The test is made headless which still fails without the fix and passes with TitledBorder fix and also pass in all CI headless platforms

[openjdk]

@prsadhuk 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:

8213531: Test javax/swing/border/TestTitledBorderLeak.java fails

Reviewed-by: prr

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 530 new commits pushed to the master branch:

• 239ac2a: 8286829: Shenandoah: fix Shenandoah Loom support
• 67ecd30: 8287398: Allow concurrent execution of hotspot docker tests
• 8071b23: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux
• 774928f: 8287625: ProblemList jdk/jshell/HighlightUITest.java on all platforms
• e3791ec: 8287491: compiler/jvmci/errors/TestInvalidDebugInfo.java fails new assert: assert((uint)t < T_CONFLICT + 1) failed: invalid type #
• f8eb7a8: 8287512: continuationEntry.hpp has incomplete definitions
• b2b4ee2: 8287233: Crash in Continuation.enterSpecial: stop: tried to execute native method as non-native
• 168b226: 8282662: Use List.of() factory method to reduce memory consumption
• 48f19e4: 8287453: RenderPerfTest incorrectly measures performance
• 0ef3d85: 8287552: riscv: Fix comment typo in li64
• ... and 520 more: https://git.openjdk.java.net/jdk/compare/16a8ebbf0573b8ee75072f8120fb0d4a584cb51d...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.

[mrserb]

Even if we remove TitledBorder code from the test and only keep JFrame, then also I see

So it means that the last visible but already disposed frame is sitting in the memory/leaked and this test catch that, isn't it?

[prsadhuk]

/integrate

[openjdk]

Going to push as commit 07d2450.
Since your change was applied there have been 536 commits pushed to the master branch:

• 6030c0e: 8287118: Use monospace font for annotation default values
• 72bcf2a: 4511638: Double.toString(double) sometimes produces incorrect results
• 2f19144: 8282024: add EscapeAnalysis statistics under PrintOptoStatistics
• cdb4768: 8287396: LIR_Opr::vreg_number() and data() can return negative number
• 4caf1ef: 8287390: Cleanup Map usage in AuthenticationInfo.requestAuthentication
• 27ad1d5: 8287602: (fs) Avoid redundant HashMap.containsKey call in MimeTypesFileTypeDetector.putIfAbsent
• 239ac2a: 8286829: Shenandoah: fix Shenandoah Loom support
• 67ecd30: 8287398: Allow concurrent execution of hotspot docker tests
• 8071b23: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux
• 774928f: 8287625: ProblemList jdk/jshell/HighlightUITest.java on all platforms
• ... and 526 more: https://git.openjdk.java.net/jdk/compare/16a8ebbf0573b8ee75072f8120fb0d4a584cb51d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@prsadhuk Pushed as commit 07d2450.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[prsadhuk]

Have raised JDK-8287707 for JFrame leak but it is seem playing around with Thread.sleep, the freed count varies from 7-9 so it can be a test issue too.