RFR(XS): 8209736: runtime/RedefineTests/ModifyAnonymous.java fails with NullPointerException when running in CDS mode

[Jiangli Zhou]

Hi Calvin,

Thanks for making the updates. It would be useful to add a comment 
explaining the extra log is intended for assisting debugging when null 
is detected in the allLoadedClasses array in rare cases. No new webrev 
is needed for me.

Thanks,

Jiangli


On 9/4/18 11:53 AM, Calvin Cheung wrote:
> Hi Jiangli,
>
> Here's an updated webrev with your suggestions:
> - logging the loaded class names into a file;
> - throws a RuntimeException upon encountering a null clazz.
>
> webrev: http://cr.openjdk.java.net/~ccheung/8209736/webrev.01/
>
> thanks,
> Calvin
>
> On 8/31/18, 3:47 PM, Jiangli Zhou wrote:
>>
>>> On Aug 31, 2018, at 2:54 PM, Calvin 
>>> Cheung<calvin.cheung at oracle.com>  wrote:
>>>
>>>
>>>
>>>> On 8/31/18, 1:52 PM, Jiangli Zhou wrote:
>>>> Hi Calvin,
>>>>
>>>>
>>>>> On 8/31/18 1:33 PM, Calvin Cheung wrote:
>>>>> Hi Jiangli,
>>>>>
>>>>>> On 8/31/18, 1:10 PM, Jiangli Zhou wrote:
>>>>>> Hi Calvin,
>>>>>>
>>>>>> How about use a separate loop (before the main test loop in 
>>>>>> runTest()) to print out the class names in the allLoadedClasses 
>>>>>> array?
>>>>> 109     static void runTest() {
>>>>> 110         while (!done) {
>>>>> 111             Class[] allLoadedClasses = 
>>>>> inst.getAllLoadedClasses();
>>>>> 112             int len = allLoadedClasses.length;
>>>>> 113             for (int idx = 0; idx<  len; idx++) {
>>>>>
>>>>> Did you mean print out all the class names after line 111? There 
>>>>> would be too much output since there's a while loop enclosing it.
>>>>> Or did you mean something else?
>>>> Yes. If the failure does re-manifest again, we can compare the 
>>>> output with a normal run. That would give some useful information. 
>>>> How big is the allLoadedClasses?
>>> On my local linux machine, it only loops through the while loop 
>>> twice and I saw more than 1800 class names in the output.
>>>> Or, maybe we could print the classes if null is detected.
>>> This is a better option.
>> Probably dumping to a file.
>>
>>>>>> If null is discovered, throw a RuntimeException(). That helps to 
>>>>>> provide more debugging information and also will not silent the 
>>>>>> issue that causes the NPE if it does' resurface again.
>>>>> I don't think throwing a RuntimeException() gives anymore info 
>>>>> than the existing NPE.
>>>>> The idea of this change is that after a null clazz is encountered, 
>>>>> let the test continue to run. It may fail in a different way which 
>>>>> may give us more clues.
>>>> The NPE might be caused by a rare race condition, which might not 
>>>> affect the redefineClass process and the test could pass with the 
>>>> change. We should avoid such case.
>>> Maybe but we don't know for sure. The main purpose of the test is to 
>>> make sure an anonymous class cannot be retransformed or redefined. 
>>> So I don't think we're masking any problem if a null class is 
>>> encountered and the test passed.
>> The issue is with getAllLoadedClasses. The proposed change would make 
>> the issue not being detected. I think we need to understand what’s 
>> the expected behavior of the API.
>>
>> Thanks,
>> Jiangli
>>> thanks,
>>> Calvin
>>>> Thanks,
>>>> Jiangli
>>>>> thanks,
>>>>> Calvin
>>>>>> Thanks,
>>>>>>
>>>>>> Jiangli
>>>>>>
>>>>>>
>>>>>>> On 8/31/18 11:33 AM, Calvin Cheung wrote:
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8209736
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8209736/webrev.00/
>>>>>>>
>>>>>>> The test failure is no longer reproducible; it happened only 
>>>>>>> once in tier3 and once in tier6 testing.
>>>>>>> This simple change is just adding a null check on the variable 
>>>>>>> clazz and prints the array size and index if it is null.
>>>>>>>
>>>>>>> Testing: hs-tier{1,2,3} with default CDS archive in JDK.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin