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

[Stefan Karlsson]

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