On Sat, 30 Jan 2021 14:35:50 GMT, Jaikiran Pai <j...@openjdk.org> 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
