RFR: 8038468: java/lang/instrument/ParallelTransformerLoader.sh fails with ClassCircularityError

[Yumin Qi]

David,  Thanks for the comment. See embedded.


On 10/13/2014 7:30 PM, David Holmes wrote:
> Hi Yumin,
>
> jdk9-dev is not the best place for code review requests. 
> serviceability-dev would be better for this test.
>
> On 14/10/2014 8:58 AM, Yumin Qi wrote:
>> bug: https://bugs.openjdk.java.net/browse/JDK-8038468
>> webrev:*http://cr.openjdk.java.net/~minqi/8038468/webrev00/
>>
>> the bug marked as confidential so post the webrev internally.
>
> Not any more :)
>
Thanks. I changed to non security related bug. Usually when test failed, 
a confidential bug is filed. I would like to create bug open if the test 
is in open part.
>> Problem: The test case tries to load a class from the same jar via agent
>> in the middle of loading another class from the jar via same class
>> loader in same thread. The call happens in transform which is a rare
>> case --- in middle of loading class, loading another class. The result
>> is a CircularityError. When first class is in loading, in vm we put
>> JarLoader$2 on place holder table, then we start the defineClass, which
>> calls transform, begins loading the second class so go along the same
>> routine for loading JarLoader$2 first, found it already in placeholder
>> table. A CircularityError is thrown.
>> Fix: The test case should not call loading class with same class loader
>> in same thread from same jar in 'transform' method. I modify it loading
>> with system class loader and we expect see ClassNotFoundException.
>> Detail see bug comments.
>
> It is not clear to me that the test is incorrect. It is also unclear 
> why such an old test is now failing - we must have changed something.
> And it's unclear whether what the test does with your change is 
> actually testing what the test wanted to test.
>
> It seems to me that the actual problem in the test is the reference to 
> the "main" thread ie:
>
>  if (!tName.equals("main"))
>
> The test knows not to do the loading in the main thread, but has 
> overlooked the fact that the main thread, upon the end of main() 
> becomes the DestroyJavaVM thread - and it is that thread which 
> encounters the ClassCircularityError:
>
> Starting test with 1000 iterations
> Thread 'DestroyJavaVM' has called transform()
>
> So perhaps the right fix is to expand the above to:
>
>  if (!tName.equals("main") && !tName.equals("DestroyJavaVM"))
>
> ? I admit I'm having trouble seeing the full picture in this test.
>
It is not DestroyJavaVM thread cause CircularityError. It is TestThread 
cause CircularityError.
In TestThread (DestroyJavaVM may cause same I think, but not seen in debug):

     forName("TestClass2", true, classLoader);  <---- the loader is 
customer loader which is obtained from agent code.
          -->...... transform(...)
              -->defineClass(...)
                    -->...... call into vm, we need to load JarLoader$2 
since JarLoader$1 used
                        ->resolve_instance_class_or_null
                                // here we create PlaceTableEntry for 
JarLoader$2, put into place holder table
                            -->......
                                --->forName("TestClass3", true, 
classLoader);
                                    -->... transform(...)
                                        -->defineClass(...)
                                            -->...... call into vm 
again. Now JarLoader$2 is not loaded, but it is in placeholder table, so 
throw_circularity_error set and throw.
                       .......
      With custom loader, agent's transform will be called, then it 
loads TestClass3, repeat the same steps as loading TestClass2. The 
problem is JarLoader$2 has not been loaded yet but in place holder table 
(this is for checking CircularityError), then begins loading TestClass3, 
this is a recursive and embedded case.  The non-failed case also saw 
CircularityError thrown, but somehow the test case did not fail.  Design 
like this will cause call transform in transform which is the reason 
CircularityError thrown.

    I have no idea about the original desin of the test case, but think 
it should do this.

>
> Looking at your change, don't leave commented out lines in the code:
>  115                         // ClassLoader loader = 
> ParallelTransformerLoaderAgent.getClassLoader();
>  118                                 //Class.forName("TestClass" + 
> index, true, loader);
>
Will remove

Thanks
Yumin
> Thanks,
> David
>
>> Thanks
>> Yumin *