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

Jump to bottom

8260401: StackOverflowError on open WindowsPreferences #2326

Closed
wants to merge 1 commit into from
Closed

8260401: StackOverflowError on open WindowsPreferences #2326

wants to merge 1 commit into from

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Jan 30, 2021 •
edited by openjdk bot

Can I please get a review for this change which proposes to fix the issue reported in https://bugs.openjdk.java.net/browse/JDK-8260401?

As noted in that issue, when the constructor of java.util.prefs.WindowsPreferences runs into an error while dealing with the Windows registry, it logs a warning message. The message construction calls rootNativeHandle() (on the same instance of WindowsPreferences that's being constructed) which then ultimately ends up calling the constructor of WindowsPreferences which then again runs into the Windows registry error and attempts to log a message and this whole cycle repeats indefinitely leading to that StackOverFlowError.

Based on my limited knowledge of the code in that constructor and analyzing that code a bit, it's my understanding (and also the input provided by the reporter of the bug) that the log message should actually be using the rootNativeHandle parameter that is passed to this constructor instead of invoking the rootNativeHandle() method. The commit in this PR does this change to use the passed rootNativeHandle in the log message.

Furthermore, there's another constructor in this class which does a similar thing and potentially can run into the same error as this one. I've changed that constructor too, to avoid the call to rootNativeHandle() method in that log message. Reading the log message that's being generated there, it's my understanding that this change shouldn't cause any inaccuracy in what's being logged and in fact, I think it's now more precise about which handle returned the error code.

Finally, this log message creation, in both the constructors, also calls windowsAbsolutePath() and byteArrayToString() methods. The byteArrayToString() is a static method and a call to it doesn't lead back to the constructor again. The windowsAbsolutePath() is a instance method and although it does use a instance variable absolutePath, that instance variable is already initialized appropriately by the time this windowsAbsolutePath() gets called in the log message. Also, the windowsAbsolutePath() call doesn't lead back to the constructor of the WindowsPreferences so it doesn't pose the same issue as a call to rootNativeHandle().

Given the nature of this issue, I haven't added any jtreg test for this change.

Progress

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

Issue

  • JDK-8260401: StackOverflowError on open WindowsPreferences

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 30, 2021

👋 Welcome back jpai! 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 openjdk bot added the rfr Pull request is ready for review label Jan 30, 2021
@openjdk
Copy link

openjdk bot commented Jan 30, 2021

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

  • core-libs

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.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 30, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 30, 2021

Webrevs

@bplb
Copy link
Member

bplb commented Feb 1, 2021

The code change looks all right. It would be better if there were a test, but apparently this might depend on the user who runs the test not having registry access rights.

@jaikiran
Copy link
Member Author

jaikiran commented Feb 2, 2021

Hello Brian,

Thank you for the review.

It would be better if there were a test, but apparently this might depend on the user who runs the test not having registry access rights.

That's correct. Looking at the code, it looks to me that this will require very specific setup of the Windows system to be able to trigger the error.

The code change looks all right.

Should I go ahead and integrate this?

@jaikiran
Copy link
Member Author

jaikiran commented Feb 2, 2021

The code change looks all right.

Should I go ahead and integrate this?

Actually, I didn't notice that this PR wasn't marked as reviewed. I'll wait for the review(s) then.

@bplb
Copy link
Member

bplb commented Feb 2, 2021

I'd let it sit for a bit in case others want to comment.

@jaikiran
Copy link
Member Author

Ping. Anymore reviews/suggestions from anyone?

@bplb
Copy link
Member

bplb commented Feb 12, 2021

Did you run this through the usual CI tests in all tiers?

@jaikiran
Copy link
Member Author

Did you run this through the usual CI tests in all tiers?

Hello Brian,

Do you mean other than the ones that have been automatically run and passed in the GitHub actions against this PR? I don't have a Windows box, but if there's some specific tests you want me to run locally, please do let me know and I'll run them.

@bplb
Copy link
Member

bplb commented Feb 12, 2021 via email

bplb
bplb approved these changes Feb 12, 2021
View reviewed changes
Copy link
Member

@bplb bplb left a comment

Choose a reason for hiding this comment

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

I think you can go ahead and integrate this now.

@openjdk
Copy link

openjdk bot commented Feb 12, 2021

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

8260401: StackOverflowError on open WindowsPreferences

Reviewed-by: bpb

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

  • 735757f: 8261661: gc/stress/TestReclaimStringsLeaksMemory.java fails because Reserved memory size is too big
  • e29c560: 8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes
  • dc46aa8: 8261534: Test sun/security/pkcs11/KeyAgreement/IllegalPackageAccess.java fails on platforms where no nsslib artifacts are defined
  • f0d9829: 8261533: Java_sun_font_CFont_getCascadeList leaks memory according to Xcode
  • 06170b7: 8261662: Rename compute_loader_lock_object
  • 3dc6f52: 8261160: Add a deserialization JFR event
  • a305743: 8261660: AArch64: Race condition in stub code generation for LSE Atomics
  • 28163a9: 8261652: Remove some dead comments from os_bsd_x86
  • 6675775: 8253702: BigSur version number reported as 10.16, should be 11.nn
  • 33fcd32: 8261659: JDK-8261027 causes a Tier1 validate-source failure
  • ... and 192 more: https://git.openjdk.java.net/jdk/compare/fcfe6478f945fc049eb8bee10d74f6350e87d71a...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 Feb 12, 2021
@jaikiran
Copy link
Member Author

Thank you Brian for the review and help in testing the change.

@jaikiran
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Feb 13, 2021
@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 Feb 13, 2021
@openjdk
Copy link

openjdk bot commented Feb 13, 2021

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

  • 735757f: 8261661: gc/stress/TestReclaimStringsLeaksMemory.java fails because Reserved memory size is too big
  • e29c560: 8247514: Improve clhsdb 'findpc' ability to determine what an address points to by improving PointerFinder and PointerLocation classes
  • dc46aa8: 8261534: Test sun/security/pkcs11/KeyAgreement/IllegalPackageAccess.java fails on platforms where no nsslib artifacts are defined
  • f0d9829: 8261533: Java_sun_font_CFont_getCascadeList leaks memory according to Xcode
  • 06170b7: 8261662: Rename compute_loader_lock_object
  • 3dc6f52: 8261160: Add a deserialization JFR event
  • a305743: 8261660: AArch64: Race condition in stub code generation for LSE Atomics
  • 28163a9: 8261652: Remove some dead comments from os_bsd_x86
  • 6675775: 8253702: BigSur version number reported as 10.16, should be 11.nn
  • 33fcd32: 8261659: JDK-8261027 causes a Tier1 validate-source failure
  • ... and 192 more: https://git.openjdk.java.net/jdk/compare/fcfe6478f945fc049eb8bee10d74f6350e87d71a...master

Your commit was automatically rebased without conflicts.

Pushed as commit 849390a.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bplb bplb bplb approved these changes

Assignees
No one assigned
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
Milestone
No milestone
2 participants
@jaikiran @bplb