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

[Per Liden]

Hi,

On 2016-04-15 20:26, Dean Long wrote:
> On 4/15/2016 10:02 AM, Lois Foltan wrote:
>>
>> On 4/15/2016 9:11 AM, Stefan Karlsson wrote:
>>> Hi Lois,
>>>
>>> On 2016-04-15 14:50, Lois Foltan wrote:
>>>>
>>>> On 4/14/2016 4:23 PM, Dean Long wrote:
>>>>> Do the inc_keep_alive()  and dec_keep_alive() updates need to be
>>>>> atomic by any chance?
>>>>
>>>> Thanks Dean for the review and good point.  I will make that change
>>>> and send out an updated webrev.
>>>
>>> When would we ever race on the _keep_alive variable? Or is this more
>>> of a defensive change to safeguard against future changes?
>>
>> 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?
>>
>
> It does seems excessive if we are single threaded.  So calling
> Universe::is_module_initialized() should always return false in those
> functions?  If so, then you could add an assert.

I think it's excessive and potentially confusing for someone reading the 
code, since it kind of signals that we would be in a multi-threaded 
context when we're not. I think adding an assert would be more clear.

cheers,
Per

>
> dl
>
>> Thanks,
>> Lois
>>
>>>
>>> Thanks,
>>> StefanK
>>>
>>>> Lois
>>>>
>>>>>
>>>>> dl
>>>>>
>>>>> On 4/14/2016 12:29 PM, Lois Foltan wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the following fix:
>>>>>>
>>>>>> Webrev:
>>>>>>       http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949/
>>>>>>
>>>>>> Bug: Jigsaw crash when Klass in _fixup_module_field_list is unloaded
>>>>>>       https://bugs.openjdk.java.net/browse/JDK-8152949
>>>>>>
>>>>>> Summary:
>>>>>> Prior to java.base being defined to the VM by the module system
>>>>>> initialization, classes loaded must be saved on a fixup list in
>>>>>> order to later have their java.lang.Class' module field patched
>>>>>> with java.base's java.lang.reflect.Module object once java.base is
>>>>>> defined. Before module system initialization is complete, all
>>>>>> classes loaded must have java.base as their defining module and be
>>>>>> loaded by the boot loader.   It was erroneously assumed that all
>>>>>> classes placed on the module fixup list therefore would not die
>>>>>> before java.base was defined.  This assumption did not hold for
>>>>>> anonymous classes which have a shorter lifetime than the boot
>>>>>> loader.  Test cases run with a small heap, -Xmx2m, would cause GC
>>>>>> to unload the anonymous classes on the fixup list, later causing
>>>>>> issues when an attempt was made to patch these classes with
>>>>>> java.base's java.lang.reflect.Module object. Thank you to Per
>>>>>> Liden and Stefan Karlsson for contributing this fix for the
>>>>>> runtime team.
>>>>>>
>>>>>> Test:
>>>>>> - java/lang, java/util, java/io, all Hotspot jtreg tests, Hotspot
>>>>>> colocated tests & noncolo.quick.testlist
>>>>>> - several iterations of ConcurrentLinkedQueue/RemoveLeak.java
>>>>>> which exhibited the problem
>>>>>
>>>>
>>>
>>
>