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

[David Holmes]

Hi Lois,

Please note I did not actually review this - the locking issue simply 
caught my attention.

Thanks,
David

On 20/04/2016 5:48 AM, 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.
>
> Thanks,
> Lois
>
>>
>> Thanks,
>> David
>>
>>> Please review at:
>>>
>>>      http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/
>>>
>>> Retesting in progress.
>>>
>>> Thanks,
>>> Lois
>>>
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>>
>>>>> thanks,
>>>>> StefanK
>>>>>
>>>>>
>>>>>>
>>>>>> -Alan
>>>>>
>>>>
>>>
>