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

[Lois Foltan]

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.

Please review at:

     http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/

Retesting in progress.

Thanks,
Lois

>
> Thanks,
> Lois
>
>>
>> thanks,
>> StefanK
>>
>>
>>>
>>> -Alan
>>
>