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

[Calvin Cheung]

On 9/4/18, 12:05 PM, Jiangli Zhou wrote:
> 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.
I'll add a comment before pushing the change.

thanks,
Calvin
>
> 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
>