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

[David Holmes]

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

Thanks,
David

> Please review at:
>
>      http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/
>
> Retesting in progress.
>
> Thanks,
> Lois
>
>>
>> Thanks,
>> Lois
>>
>>>
>>> thanks,
>>> StefanK
>>>
>>>
>>>>
>>>> -Alan
>>>
>>
>