RFR (S): JDK-8152949: Jigsaw crash when Klass in _fixup_module_field_list is unloaded

[Lois Foltan]

On 4/19/2016 4:24 PM, Coleen Phillimore wrote:
>
> Hi, this is getting long.
>
> On 4/19/16 3:48 PM, Lois Foltan wrote:
>>
>> On 4/18/2016 10:11 PM, David Holmes wrote:
>>> Hi Lois,
>>>
>>> On 19/04/2016 6:25 AM, Lois Foltan wrote:
>>>>
>>>> On 4/18/2016 7:31 AM, Lois Foltan wrote:
>>>>>
>>>>> On 4/18/2016 3:06 AM, Stefan Karlsson wrote:
>>>>>> On 2016-04-15 21:45, Alan Bateman wrote:
>>>>>>>
>>>>>>> On 15/04/2016 18:02, Lois Foltan wrote:
>>>>>>>>
>>>>>>>> Hi Stefan,
>>>>>>>>
>>>>>>>> In start up before module system initialization in complete I
>>>>>>>> believe the VM is single threaded, so the increment/decrement
>>>>>>>> reference counts do not need to be atomic.  Adding it is a
>>>>>>>> defensive move in case the reference count is ever used passed
>>>>>>>> start up in the future.  It kind of does seem a bit excessive,
>>>>>>>> sounds like you agree?
>>>>>>> There will be a number of threads running before the base module is
>>>>>>> defined to the VM. As things stand the the java threads at this
>>>>>>> point will be the Common-Cleaner, Finalizer, Reference Handler and
>>>>>>> Signal Handler.
>>>>>>
>>>>>> So, are you saying that we need the atomics?
>>>>>>
>>>>>> The java_lang_Class::create_mirror function isn't multi-thread safe,
>>>>>> and must already be guarded by a lock (SystemDictionary_lock 
>>>>>> AFAICT).
>>>>>> The increment in Unsafe_DefineAnonymousClass0, will only be done
>>>>>> once, for the single InstanceKlass instance in the CLD. And all 
>>>>>> reads
>>>>>> of _keep_alive from the GC are done during safepoints.
>>>>> The anonymous class is inserted in the fixup mirror and fixup module
>>>>> lists during java_lang_Class::create_mirror() before it is made 
>>>>> public
>>>>> or "published" as loaded.  So the two instances where the reference
>>>>> count is incremented, Unsafe_DefineAnonymousClass0 and in
>>>>> java_lang_Class::create_mirror(), are guarded by a lock as well as 
>>>>> the
>>>>> decrement in Unsafe_DefineAnonymousClass0.  No other thread has 
>>>>> access
>>>>> to the class during this time, as it is being loaded.
>>>>>>
>>>>>> How does ModuleEntryTable::patch_javabase_entries guard against
>>>>>> concurrent inserts into the _fixup_module_field_list list?
>>>>> That leaves the decrement in
>>>>> ModuleEntryTable::patch_javabase_entries() as possibly unguarded. 
>>>>> This
>>>>> only occurs when the VM is called to define the module java.base.  I
>>>>> believe this should be okay but will double check.
>>>>
>>>> One small change in modules.cpp/define_javabase_module() to ensure 
>>>> that
>>>> only one definition attempt of java.base will occur and thus only one
>>>> call to ModuleEntryTable::patch_javabase_entries().  If a situation
>>>> arises where java.base is trying to be multiply defined, according to
>>>> the expected error conditions for JVM_DefineModule(), an
>>>> IllegalArgumentException should be thrown.
>>>>
>>>> I have also added a comment in classfile/classLoaderData.hpp 
>>>> explaining
>>>> why _keep_alive does need to be defined volatile or atomic.
>>>
>>> Can you add assertions to check that _keep_alive is only modified 
>>> under the protection of the lock (with a special case perhaps for 
>>> the unguarded java.base case) ?
>>
>> Hi David,
>>
>> Thanks for the review.  I misspoke when I indicated that the two 
>> increments and the one decrement of the reference counter that occur 
>> during a call to the Unsafe_DefineAnonymous0() method were guarded 
>> under a lock.  However, due to the way anonymous classes are created 
>> only a single non-GC thread will have access to the _keep_alive field 
>> during this time.  And as Stefan indicates above, all reads of 
>> _keep_alive from the GC are done during safepoints.  Each anonymous 
>> class, when defined, has a dedicated ClassLoaderData created for it.  
>> No other class shares the anonymous class' name or CLD.  Due to this 
>> uniqueness, no other thread has knowledge about this anonymous class 
>> while it is being defined.  It is only upon return from 
>> Unsafe_DefineAnonymous0(), that the anonymous class exists and other 
>> threads, at that point, can potentially access it.
>>
>
> Ah interesting.  Currently, this is true and why this is safe.  If we 
> change the JVM to have *some* anonymous classes share CLD with their 
> host_class because the lifetimes are the same, then we'll have to use 
> atomic operations.
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/src/share/vm/classfile/classLoaderData.cpp.udiff.html 
>
>
> Can you put one comment directly above the inc_keep_alive() and 
> dec_keep_alive() functions to this effect here, just so we remember?

Hi Coleen,

I will add the comment before I commit.  I might expand a bit on your 
last sentence a bit.

>
> // Anonymous classes have their own ClassLoaderData that is marked to 
> keep alive while the class is being parsed, and
> // if the class appears on the module fixup list.
> // If anonymous classes are changed to share with host_class, this 
> refcount needs to be changed to use atomic operations.
>
> *+ void ClassLoaderData::inc_keep_alive() {*
> *+ assert(_keep_alive >= 0, "Invalid keep alive count");*
> *+ _keep_alive++;*
> *+ }*
> *+ *
> *+ void ClassLoaderData::dec_keep_alive() {*
> *+ assert(_keep_alive > 0, "Invalid keep alive count");*
> *+ _keep_alive--;*
> *+ }*
> *+ *
>
> More below.
>
>> Thanks,
>> Lois
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> Please review at:
>>>>
>>>>      http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/src/share/vm/classfile/modules.cpp.frames.html 
>
>
> I'm not sure how this relates to the bug.

This change ensures that java.base will not be multiply defined and thus 
only one call to ModuleEntryTable::patch_javabase_entries() will occur.  
A decrement of the reference count happens when an anonymous class is on 
the fixup module list post patching it with java.base

>
> Otherwise, the change looks good.

Thanks again!

>
> Thanks,
> Coleen
>
>
>>>>
>>>> Retesting in progress.
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>>
>>>>> Thanks,
>>>>> Lois
>>>>>
>>>>>>
>>>>>> thanks,
>>>>>> StefanK
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> -Alan
>>>>>>
>>>>>
>>>>
>>
>