On Fri, 12 Feb 2021 23:40:51 GMT, Brian Burkhalter <b...@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. Thank you Brian for the review and help in testing the change. ------------- PR: https://git.openjdk.java.net/jdk/pull/2326
