RFR (S) 8218266: G1 crash in AccessInternal::PostRuntimeDispatch

[coleen.phillimore at oracle.com]

On 2/28/19 10:29 AM, zgu at redhat.com wrote:
> Hi Coleen,
>
> On Tue, 2019-02-26 at 17:25 -0500, coleen.phillimore at oracle.com wrote:
>> Please review: I've added a lock for the DictionaryEntry::pd_set
>> which
>> is a linked list of ProtectionDomainTableEntries.
>>
>> open webrev at
>> http://cr.openjdk.java.net/~coleenp/2019/8218266.02/webrev
> This looks good.
>
> I still prefer to tighten method accesses of Dictionary class, but up
> to you if you want to do it in this patch.

Yes, I agree with you.  I'll make your change in this patch to make 
Dictionary functions private.  I meant to do that.
Thanks!
Coleen
>
> Thanks,
>
> -Zhengyu
>> Ran with modified test and hs tier1-3 and jck tests.
>>
>> Thanks,
>> Coleen
>>
>> On 2/25/19 5:48 PM, coleen.phillimore at oracle.com wrote:
>>>
>>> On 2/25/19 5:22 PM, zgu at redhat.com wrote:
>>>>>> problem. Therefore, I would like to withdraw my review.
>>>>> Okay, I can try to explain it.  All of the dictionary entries
>>>>> point
>>>>> to a
>>>>> linked list of protection domains that were used to load the
>>>>> class.
>>>>> The elements in this list point directly to Hashtable entries
>>>>> in the
>>>>> ProtectionDomainTable.  The protection domains can be GC'd
>>>>> before
>>>>> the
>>>>> dictionary entry is unloaded, so that the elements in the
>>>>> linked
>>>>> list
>>>>> need to be cleaned out.  I chose cleaning to happen right
>>>>> before the
>>>>> protection domain cache table is cleaned up so that the entries
>>>>> are
>>>>> not
>>>>> dangling pointers.  Since the table is cleaned out concurrently
>>>>> now,
>>>>> I
>>>>> have to do the cleanup there so I know it's happened.
>>>>>
>>>> Thanks for educating me.
>>>>
>>>> I were not sure if the crash was due to concurrent access, e.g.
>>>> DictionaryEntry could *leak* outside of Dictionary, then
>>>> ProtectedDomain can be accessed without protection. It looks like
>>>> that
>>>> was not the case, but still should be avoided. How about places
>>>> these
>>>> methods under private? it built.
>>>>
>>>> http://cr.openjdk.java.net/~zgu/JDK-8218266-review/webrev.00/src/
>>>> hotspo
>>>> t/share/classfile/dictionary.hpp.udiff.html
>>>>
>>>>
>>>> I did found an instance of unprotected access -
>>>> DictionaryEntry::contains_protection_domain() may be unprotected
>>>> when
>>>> it walks protection domain list.
>>>>
>>>> I added "assert_locked_or_safepoint(SystemDictionary_lock);"
>>>> inside the
>>>> method:
>>>>
>>>> http://cr.openjdk.java.net/~zgu/JDK-8218266-review/webrev.00/src/
>>>> hotspo
>>>> t/share/classfile/dictionary.cpp.udiff.html
>>>>
>>>> the assertion failed.
>>> Hi Zhengyu,   This is an excellent find!  Since we concurrently
>>> clean
>>> the ProtectionDomainCacheTable, we need to lock access to the
>>> pd_lists
>>> in the Dictionary entries.  We used to be able to read them lock
>>> free.
>>>
>>> I have to rework this to add the appropriate locking.
>>>
>>> thanks!
>>> Coleen
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>>
>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>> Sorry.
>>>>>>
>>>>>> -Zhengyu
>>>>>>
>>>>>>
>>>>>>> Coleen
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> -Zhengyu
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Coleen
>>