Hi Sergey, The changes look fine.
Thanks, Krishna -----Original Message----- From: Sergey Bylokhov Sent: Tuesday, June 26, 2018 11:12 AM To: Krishna Addepalli <[email protected]>; [email protected] Subject: Re: <AWT Dev> [11] Review Request: 8189604 possible hang in sun.awt.shell.Win32ShellFolder2$KnownFolderDefinition::<clinit> Hi, Krishna. On 25/06/2018 04:52, Krishna Addepalli wrote: > The fix looks fine. However, I have couple of comments: > 1. The variable "result" in > Java_sun_awt_shell_Win32ShellFolder2_loadKnownFolders at line 1401 is not > initialized. Although it is initialized in the following if condition, I'm > not sure when it return false, and lead to a return of garbage value to Java. Yes it should be initialized: http://cr.openjdk.java.net/~serb/8189604/webrev.01 > > 2. Could you clarify a bit about the testcase - how it triggers the > hang/crash? The test tries to load all classes using 'Class.forName(name, true, null)" which will initialize the classes(will call static initialization). > > Thanks, > Krishna > > -----Original Message----- > From: Sergey Bylokhov > Sent: Monday, June 25, 2018 2:21 PM > To: [email protected] > Subject: <AWT Dev> [11] Review Request: 8189604 possible hang in > sun.awt.shell.Win32ShellFolder2$KnownFolderDefinition::<clinit> > > Hello. > Please review the fix for jdk11. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8189604 > Webrev: http://cr.openjdk.java.net/~serb/8189604/webrev.00 > > We have a cyclic dependency in > Win32ShellFolder2$KnownFolderDefinition, > which can cause a deadlock if this class is initialized on "non-COM" thread. > > 1. - Static initialization of Win32ShellFolder2$KnownFolderDefinition > 2. -- Win32ShellFolder2.getLibraries(); 3. --- invoke > Win32ShellFolder2.loadKnownFolders() on "COM-thread" and wait 4. ---- > in the native call > env->FindClass("sun/awt/shell/Win32ShellFolder2$KnownFolderDefinition" > env->); > 5. ---- deadlock. because initialization of KnownFolderDefinition was blocked > at step 3. > > In the fix this dependency is broken, so the new class KnownLibraries is not > depends from its own initialization. > > -- Best regards, Sergey.
