Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Fri, 12 Feb 2021 23:40:51 GMT, Brian Burkhalter wrote: >> 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. > > I think you can go ahead and integrate this now. Thank you Brian for the review and help in testing the change. - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Fri, 12 Feb 2021 03:34:54 GMT, Jaikiran Pai wrote: >> Did you run this through the usual CI tests in all tiers? > >> 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. > On Feb 11, 2021, at 7:35 PM, Jaikiran wrote: > > 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. > I ran this through our internal CI tests and saw no problems. - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Sat, 30 Jan 2021 14:35:50 GMT, Jaikiran Pai wrote: > 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. I think you can go ahead and integrate this now. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Fri, 12 Feb 2021 03:21:04 GMT, Brian Burkhalter wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Fri, 12 Feb 2021 03:16:02 GMT, Jaikiran Pai wrote: >> I'd let it sit for a bit in case others want to comment. > > Ping. Anymore reviews/suggestions from anyone? Did you run this through the usual CI tests in all tiers? - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Tue, 2 Feb 2021 02:41:10 GMT, Brian Burkhalter wrote: >>> > 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. > > I'd let it sit for a bit in case others want to comment. Ping. Anymore reviews/suggestions from anyone? - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Tue, 2 Feb 2021 02:31:09 GMT, Jaikiran Pai wrote: >> 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? > >> > 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. I'd let it sit for a bit in case others want to comment. - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Tue, 2 Feb 2021 02:25:04 GMT, Jaikiran Pai wrote: > > 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. - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Mon, 1 Feb 2021 19:21:23 GMT, Brian Burkhalter wrote: >> 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. > > 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. 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? - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Sat, 30 Jan 2021 14:35:50 GMT, Jaikiran Pai wrote: > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2326
