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

[David Holmes]

Hi Yumin,

On 15/10/2014 4:40 AM, Yumin Qi wrote:
> 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.

Not according to the bug report:

Starting test with 1000 iterationsThread 'DestroyJavaVM' has called 
transform()
Thread 'DestroyJavaVM' has called transform()
result=1
----------System.err:(14/920)----------
Exception in thread "main" java.lang.ClassCircularityError: 
sun/misc/URLClassPath$JarLoader$2

This shows that "main" got the CCE. Which in itself is confusing given 
we also report "Thread 'DestroyJavaVM' has called transform()" and they 
are in fact the same thread!

David
-----


> 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 *
>