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

[Harold Seigel]

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
>>>
>