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

[zgu at redhat.com]

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.

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