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

[coleen.phillimore at oracle.com]

Harold, thank you for the code review.  I will add the bug number to the 
test.

Thanks!
Coleen

On 2/28/19 2:48 PM, Harold Seigel wrote:
> Hi Coleen,
>
> Your change looks good.  You could add 8218266 to the test's @bug, if 
> you want.
>
> Thanks, Harold
>
> On 2/28/2019 11:15 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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
>>>>
>>