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

[Calvin Cheung]

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