[9] RFR(S): 8054402: "klass->is_loader_alive(_is_alive)) failed: must be alive" for anonymous classes

[Tobias Hartmann]

Vladimir, Coleen, thanks for the review!

Best,
Tobias

On 13.08.2014 18:22, Coleen Phillimore wrote:
>
> This looks really good.
> Thank you!
> Coleen
>
> On 8/13/14, 5:18 AM, Tobias Hartmann wrote:
>> Vladimir, Coleen, thanks for the review.
>>
>> Please find comments inline.
>>
>> On 12.08.2014 16:11, Coleen Phillimore wrote:
>>> There is some dead code (if statement) in dictionary::do_unloading() 
>>> when you remove this flag that you can remove also.
>>
>> Thanks for pointing that out. As you suggested, I also moved the 
>> index into the loop statement.
>>
>>> Also, about the test.  I thought AnonymousClass was something we 
>>> were going to discontinue support for (or was it a different 
>>> AnonymousClass)?
>>
>> I'm now directly using the corresponding method of the Unsafe API 
>> instead of the AnonymousClassLoader.
>>
>>> On 8/12/14, 9:49 AM, Vladimir Kozlov wrote:
>>>> Should you also modify Dictionary::do_unloading() since it is the 
>>>> only place where it is called and the result is not used anymore?
>>
>> Yes, I missed that. Done.
>>
>>>> I don't think you need IgnoreUnrecognizedVMOptions flag in the test.
>>
>> I thought we need it for the Whitebox API but that's not the case. I 
>> removed it.
>>
>> New webrev: http://cr.openjdk.java.net/~thartmann/8054402/webrev.01/
>>
>> Thanks,
>> Tobias
>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 8/12/14 2:20 AM, Tobias Hartmann wrote:
>>>>> Hi,
>>>>>
>>>>> please review the following patch that fixes JDK-8054402.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8054402
>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8054402/webrev.00/
>>>>>
>>>>> == Problem ==
>>>>> The test [1] rarely fails with the assert 
>>>>> 'klass->is_loader_alive(_is_alive)) failed: must be alive' in
>>>>> 'CheckClass::check_class' (nmethod.cpp) because a IC in a compiled 
>>>>> method [2] references an unloaded anonymous Klass*
>>>>> [3] (i.e., the Klass has an unloaded mirror class [4]). Usually, 
>>>>> ICs are cleaned in 'nmethod::do_unloading' by
>>>>> 'clean_ic_if_metadata_is_dead' if they reference stale metadata. 
>>>>> In this case, the check for stale metadata is not
>>>>> performed because 'unloading_occurred' is set to false and 
>>>>> therefore no class unloading should have happened.
>>>>> The error is in 'SystemDictionary::do_unloading'. First, 
>>>>> 'ClassLoaderDataGraph::do_unloading ' is invoked to remove the
>>>>> class loader data of unloaded classes. Next, 
>>>>> 'Dictionary::do_unloading' is called to remove the system dictionary
>>>>> entries of dead classes. 'unloading_occurred' is only set to true 
>>>>> if dead class loaders were found _and_ entries were
>>>>> removed from the system dictionary. The problem is that anonymous 
>>>>> classes are not in the system dictionary. In the rare
>>>>> case where only anonymous classes were unloaded, 
>>>>> 'unloading_occurred' is set to false even if (anonymous) class
>>>>> unloading occurred.
>>>>>
>>>>> == Solution ==
>>>>> The variable 'unloading_occurred' is always set to true if dead 
>>>>> class loaders were found.
>>>>> I was able to write a jtreg test that deterministically triggers 
>>>>> the bug.
>>>>>
>>>>> == Testing ==
>>>>> - New jtreg test
>>>>> - Failing test [1]
>>>>> - JPRT
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>>
>>>>> [1] 
>>>>> java/util/stream/test/org/openjdk/tests/java/util/stream/InfiniteStreamWithLimitOpTest.java
>>>>> [2] java.util.stream.Streams$RangeIntSpliterator::forEachRemaining
>>>>> [3] 
>>>>> java.util.stream.StreamSpliterators$IntWrappingSpliterator$$Lambda$170/25231275
>>>>> [4] 
>>>>> java/util/stream/StreamSpliterators$IntWrappingSpliterator$$Lambda$170 
>>>>>
>>>
>>
>