RFR: 8025834: NPE in Parallel Scavenge with -XX:+CheckUnhandledOops

[Coleen Phillimore]

Stefan is right.  We should be saving the result of this:

   // Oop that keeps the metadata for this class from being unloaded
   // in places where the metadata is stored in other places, like nmethods
   oop klass_holder() const {
     return is_anonymous() ? java_mirror() : class_loader();
   }

This was implemented before anonymous classes, where we used to use the 
class loader to keep any metadata that we store alive.  We did this 
preemptively in this case.  If the class loader cannot be reclaimed 
while these jvmtiBreakpoint classes are alive (ie. there is always a 
mirror or instance of the class around), we probably don't need this 
code at all.   But I don't know if this is proveable.   Potentially the 
stack cases of this, it might be, just not the allocated case?

thanks,
Coleen

On 10/18/2013 04:29 AM, Stefan Karlsson wrote:
> On 2013-10-16 18:09, Erik Helin wrote:
>> Hi all,
>>
>> this patch fixes an issue where an oop in JvmtiBreakpoint,
>> JvmtiBreakpoint::_class_loader, was found by the unhandled oop detector.
>>
>> Instead of registering the oop as an unhandled oop, which would have
>> worked, I decided to wrap the oop in a handle as long as it is on the
>> stack.
>>
>> A JvmtiBreakpoint is created on the stack by the two methods
>> JvmtiEnv::SetBreakpoint and JvmtiEnv::ClearBreakpoint. This
>> JvmtiBreakpoint is only created to carry the Method*, jlocation and oop
>> to a VM operation, VM_ChangeBreakpoints. VM_ChangeBreakpoints will, when
>> at a safepoint, allocate a new JvmtiBreakpoint on the native heap, copy
>> the values from the stack allocated JvmtiBreakpoint and then 
>> place/clear the
>> newly alloacted JvmtiBreakpoint in
>> JvmtiCurrentBreakpoints::_jvmti_breakpoints.
>>
>> I have updated to the code to check that the public constructor is only
>> used to allocate JvmtiBreakpoints on the stack (to warn a future
>> programmer if he/she decides to allocate one on the heap). The
>> class_loader oop is now wrapped in a Handle for stack allocated
>> JvmtiBreakpoints.
>>
>> Due to the stack allocated JvmtiBreakpoint having the oop in a handle,
>> the oops_do method of VM_ChangeBreakpoints can be removed. This also
>> makes the oop in the handle safe for use after the VM_ChangeBreakpoint
>> operation is finished.
>>
>> The unhandled oop in the JvmtiBreakpoint allocated on the heap will be
>> visited by the GC via jvmtiExport::oops_do ->
>> JvmtiCurrentBreakpoints::oops_do -> JvmtiBreakpoints::oops_do ->
>> GrowableCache::oops_do -> JvmtiBreakpoint::oops_do, since it is being
>> added to.
>>
>> I've also removed some dead code to simplify the change:
>> - GrowableCache::insert
>> - JvmtiBreakpoint::copy
>> - JvmtiBreakpoint::lessThan
>> - GrowableElement::lessThan
>>
>> Finally, I also formatted JvmtiEnv::ClearBreakpoint and
>> Jvmti::SetBreakpoint exactly the same to highlight that they share all
>> code except one line. Unfortunately, I was not able to remove this code
>> duplication in a good way.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~ehelin/8025834/webrev.00/
>
> Not a review.
>
> I think there's a pre-existing bug in this code. Registering the class 
> loader as a way to keep the Klass alive will not work for breakpoints 
> in Anonymous Classes (JSR292). Anonymous classes have to register the 
> mirror. See the code in the ClassLoaderData::is_alive:
>
> bool ClassLoaderData::is_alive(BoolObjectClosure* is_alive_closure) 
> const {
>   bool alive =
>     is_anonymous() ?
> is_alive_closure->do_object_b(_klasses->java_mirror()) :
>        class_loader() == NULL || 
> is_alive_closure->do_object_b(class_loader());
>   assert(!alive || claimed(), "must be claimed");
>   return alive;
> }
>
> We probably want to add a function ClassLoaderData::keep_alive_oop() 
> which returns the class loader for normal classes and the mirror for 
> the anonymous classes, and use that in this and similar situations.
>
> StefanK
>
>>
>> Testing:
>> - JPRT
>> - The four tests that were failing are now passing
>>
>> Thanks,
>> Erik
>