RFR (S) JDK-8007725 - Klass::restore_unshareable_info() triggers assert(k->java_mirror() == NULL)

[Karen Kinnear]

Ioi,

Thank you for fixing this. And thank you so much for asking me to review this - this logic is very tricky.

I had a couple of questions/comments please:

1) klass.cpp line 490, systemDictionary.cpp line 808
I believe that there are other possible PENDING_EXCEPTIONS in addition to OOM we can get here -
could you possibly modify the comments to say something like "An exception, such as OOM, ..."
(e.g. I believe we could get here due to a loader constraint exception)

2) method.cpp 804, klass.cpp 489
Can you possibly explain why you would not see a problem in DumpSharedSpaces?
For remove_unshareable_info I am confused when you would clean up a number of the fields
but leave the class_loader_data?

3. klass.cpp remove_unshareable_info
I'm still trying to figure out exactly how you determined the fields to be cleared.
I get the set that matches restore_unshareable_info.
If the other fields got filled in, then another thread got its hands on this instanceKlass, via
a sibling or subklass - and you don't want to be cleaning this up at all, so I'm wondering
if that would be an assertion instead?

4. Could you possibly remove lines 1162-1166 - I don't think those are pertinent
post-nopermgen.

5. systemDictionary.cpp

I'm wondering if you want to move the cleanup request to a different place.

If the load was successful, but the check_constraints on the initiating loader fails - I think you want to
leave the klass loaded by the defining loader (null) and leave the information intact.
(We allow a defineclass to succeed and future lookups to find the class/defining loader even if
the initiating loader fails due to loader constraints or later checkpackageaccess for instance)

I don't quite understand where the exception is happening that you are cleaning up for.
Where did you see an OOM? Where else could they happen here? What other
exceptions could happen? I think this has to be handled at a tighter level of granularity so
we know exactly when this clean up is needed.

So I'm assuming you are doing a cleanup of the unshareable info because:
find_shared_class succeeded, resolve_super_or_fail on superclasses/interfaces succeeded and
restore_unshareable_info succeeded.
Can we assume load_shared_class succeeded or do we need a check there in case notify_class_loaded
failed? 

For the bootloader today, LOAD_INSTANCE is not done in parallel, so we are not going to run
into the situation in which another thread is loading the same class in the LOAD_INSTANCE sense.
That said, once we add the instanceKlass to the systemdictionary - any other thread could 
find it and use it - so we don't want to clean it up - it may be in use.

In load_instance_class if load_shared_class succeeds, you call find_or_define_instance_class, which
calls define_class.
Inside find_or_define_instance_class and define_instance_class you can have failures 
before you add to the systemdictionary - in which case you want to do this cleanup. If the failures are
after that point - this thread may not get the information back due to a pending exception, but other
threads may use the information - so you don't want to do the cleanup. So the key is - if update_dictionary
succeeded - don't do the cleanup. If it did not, you want the cleanup.

Coleen is planning to put in code to cleanup the instanceKlass - and this should go the same place I
would assume, so you probably want to discuss this closely with her.

thanks,
Karen

On Mar 14, 2013, at 1:49 AM, Ioi Lam wrote:

> Hi, could someone review this? This fixes a crash related to OOM handling.
> 
> Thanks
> - Ioi
> 
> On 03/11/2013 12:47 PM, Ioi Lam wrote:
>> Please review:
>> http://cr.openjdk.java.net/~iklam/8007725/null_mirror_001/
>> 
>> Bug: NPG: Klass::restore_unshareable_info() triggers
>>    assert(k->java_mirror() == NULL)
>> 
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007725
>> or https://jbs.oracle.com/bugs/browse/JDK-8007725
>> 
>>    (The bugs.sun.com page may be unavailable for a couple of days due
>>     to recent change of confidentiality status.)
>> 
>> Summary of fix:
>> 
>>    The assert is at:
>> 
>>    --------
>>    # Internal Error (src/share/vm/classfile/javaClasses.cpp:527)
>>    # assert(k->java_mirror() == NULL) failed: should only assign mirror once
>>    java_lang_Class::create_mirror(KlassHandle)
>>    Klass::restore_unshareable_info()
>>    InstanceKlass::restore_unshareable_info()
>>    instanceKlassHandle SystemDictionary::load_shared_class(instanceKlassHandle,Handle)
>>    --------
>> 
>>    This happens if during class loading, an OOM happened after
>>    Klass::restore_unshareable_info() has called
>>    java_lang_Class::create_mirror(), but before the class loading is completed.
>> 
>>    At this point, the class is not in the system dictionary. However the
>>    Klass structure is partially initialized.
>> 
>>    If the app tries to load the same class again, the assert happens.
>> 
>>    The fix is to undo all work performed by
>>    InstanceKlass::restore_unshareable_info().
>> 
>> Tests executed:
>> 
>>    * JPRT
>>    * JCK (vm, lang, api/signaturetest)
>>    * UTE (vm.quick.testlist, )
>> 
>> Thanks,
>> Ioi
>