Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8253977: More memory leaks in client-libs on macOS #514

Closed
wants to merge 1 commit into from

Conversation

mrserb
Copy link
Member

@mrserb mrserb commented Oct 5, 2020

While working on the JDK-8240709 I have found that the test created for JDK-8134947 (currently executed on macOS only) can fail due to memory leaks(more often if change it to be stricter). See my comments inline.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8253977: More memory leaks in client-libs on macOS

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/514/head:pull/514
$ git checkout pull/514

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 5, 2020

👋 Welcome back serb! 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
Copy link

openjdk bot commented Oct 5, 2020

@mrserb The following labels will be automatically applied to this pull request:

  • 2d
  • awt
  • swing

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

@openjdk openjdk bot added 2d client-libs-dev@openjdk.org swing client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org labels Oct 5, 2020
@mrserb
Copy link
Member Author

mrserb commented Oct 5, 2020

/label delete 2d

@openjdk
Copy link

openjdk bot commented Oct 5, 2020

@mrserb The label delete is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

@mrserb
Copy link
Member Author

mrserb commented Oct 5, 2020

/label remove 2d

@openjdk openjdk bot removed the 2d client-libs-dev@openjdk.org label Oct 5, 2020
@openjdk
Copy link

openjdk bot commented Oct 5, 2020

@mrserb
The 2d label was successfully removed.

@@ -224,7 +224,7 @@ protected void uninstallListeners(final AbstractButton b) {
b.removeAncestorListener(listener);
}
uninstallHierListener(b);
AquaUtilControlSize.addSizePropertyListener(b);
AquaUtilControlSize.removeSizePropertyListener(b);
Copy link
Member Author

@mrserb mrserb Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tiny leak existed from the initial Mac OS X Port Project.

JTextField tf = ((JSpinner.DefaultEditor) editor).getTextField();
if (tf != null) {
tf.removeFocusListener(getNextButtonHandler());
tf.removeFocusListener(getPreviousButtonHandler());
Copy link
Member Author

@mrserb mrserb Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We started to add these listeners in JDK-8238824, but forgot to remove it.

@Override
public void setBackground(final Color c) {
super.setBackground(c);
updateOpaque();
Copy link
Member Author

@mrserb mrserb Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was added in the JDK-8033786, when it was implemented it was possible to call this method from the Window.setBackground() and from the Window.getPeer().setBackground().

Since the Window.setBackground() is always followed by the peer.setOpaque()->updateOpaque(), and now it is not possible to use the peer directly this method became redundant.

Deleted it in this fix since it generates a bunch of memory garbage.

test(chooser.getPropertyChangeListeners());
test(chooser.getAncestorListeners());
test(chooser.getVetoableChangeListeners());
private static void checkListenersCount(Component comp) {
Copy link
Member Author

@mrserb mrserb Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a test improvement, now we will check the listeners recursively for each component inside FileChooser.

* @run main/timeout=300/othervm -Xmx12m -XX:+HeapDumpOnOutOfMemoryError UnninstallUIMemoryLeaks
* @bug 8134947 8253977
* @library /test/lib
* @run main/timeout=450/othervm UnninstallUIMemoryLeaks
*/
public final class UnninstallUIMemoryLeaks {
Copy link
Member Author

@mrserb mrserb Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this test stricter:

  • The execution time is increased to 350 seconds per L&F.
  • To minimize the overall time execution the test now forks a subtest for each L&F and run it in parallel
  • Now the test recursively checks the number of listeners in all components
  • The "-mx12m" option is changed to "-mx9m"

@mrserb mrserb marked this pull request as ready for review Oct 5, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 5, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 5, 2020

Webrevs

Copy link
Member

@azuev-java azuev-java left a comment

Looks good.

@openjdk
Copy link

openjdk bot commented Oct 5, 2020

@mrserb 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 more details.

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

8253977: More memory leaks in client-libs on macOS

Reviewed-by: kizune

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

  • 4fe68f5: 8253426: jpackage is unable to generate working EXE for add-launcher configurations
  • c9d0407: 8253794: TestAbortVMOnSafepointTimeout never timeouts
  • f2f77f7: 8253761: Wrong URI syntax printed by jar --describe-module
  • b29e108: 8253944: Certain method references to VarHandle methods should fail
  • 88d75c9: 8156071: List.of: reduce array copying during creation
  • ea27a54: 8224509: Incorrect alignment in CDS related allocation code on 32-bit platforms
  • 4d29116: 8253433: Remove -XX:+Debugging product option
  • 81dae70: 8253948: Memory leak in ImageFileReader
  • 65cab55: 8253971: ZGC: Flush mark stacks after processing concurrent roots
  • 19219a9: 8253960: Memory leak in Java_java_lang_ClassLoader_defineClass0()
  • ... and 2 more: https://git.openjdk.java.net/jdk/compare/9604ee82690f89320614b37bfef4178abc869777...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 openjdk bot added the ready Pull request is ready to be integrated label Oct 5, 2020
@mrserb
Copy link
Member Author

mrserb commented Oct 7, 2020

/integrate

@openjdk openjdk bot closed this Oct 7, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 7, 2020
@openjdk
Copy link

openjdk bot commented Oct 7, 2020

@mrserb Since your change was applied there have been 29 commits pushed to the master branch:

  • 2a0389a: 8254095: remove jdk.test.lib.Utils::distro() method
  • ac772cd: 8253750: use build-stable default seed for Utils.RANDOM_GENERATOR
  • 6712f8c: 8254084: Remove TemplateTable::pd_initialize
  • 82fe023: 8254061: Missing space in flag description
  • bd50ccd: 8253735: Cleanup SearchIndexItem API
  • 54b340b: 8254081: java/security/cert/PolicyNode/GetPolicyQualifiers.java fails due to an expired certificate
  • 51fdb4c: 8254075: Shenandoah: Remove ShenandoahCodeRootsStyle diagnostic flag and related test
  • 77921b9: 8254080: fix for JDK-8204256 causes jlink test failures
  • 57493c1: 8253694: Remove Thread::muxAcquire() from ThreadCrashProtection()
  • d2b1dc6: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/9604ee82690f89320614b37bfef4178abc869777...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3973073.

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

@mrserb mrserb deleted the JDK-8253977 branch Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awt client-libs-dev@openjdk.org integrated Pull request has been integrated swing client-libs-dev@openjdk.org
2 participants