review (S) for 6656830: assert((*p)->is_oop(), "expected an oop while scanning weak refs")

[Tom Rodriguez]

On Jun 18, 2010, at 2:32 PM, Vladimir Kozlov wrote:

> Tom,
> 
> I think _jmethod_id could be NULL when unload request happened between publishing nmethod and call to post_compiled_method_load_event() since it is called outside lock so post_compiled_method_load_event() is not called yet.
> And, for the same reason, call method()->jmethod_id() in post_compiled_method_load_event() can have the same original problem you are fixing. Do I make sense or it is rubbish?

The nmethod can't be unloaded at this point because there are strong references to the method being compiled from the compile broker, the CI and the methodHandle in the register_method.

> We can avoid it if we call get_and_cache_jmethod_id() in nmethod::new_nmethod() and nmethod::new_native_nmethod().

I wanted to avoid creating the jmethodID unless JVMTI was active since the semantics of jmethodIDs require that they are never freed, so it would be a leak all the time.

> Also mh is not used anymore in jvmtiCodeBlobEvents.cpp
> 326   methodHandle mh(nm->method());

I've deleted that.

tom

> 
> Vladimir
> 
> Tom Rodriguez wrote:
>> While investigating the sweeper bug I ended up diagnosing this one too.  I already have one review from Dan since he helped me work it out.
>> http://cr.openjdk.java.net/~never/6656830
>> 6656830: assert((*p)->is_oop(),"expected an oop while scanning weak refs")
>> Reviewed-by: dcubed
>> This is a long standing bug with the handling of jmethodIDs by the
>> logic for notifying of code unloading.  The compiled method unload
>> event passes the jmethodID as part of the event so you need to look it
>> up when an nmethod is unloaded because the class it's associated with
>> is being unloaded.  jmethodIDs are loosely coupled to the methodOop so
>> there's an array of them in the instanceKlass and they are searched
>> when the jmethodID for a methodOop is required.  jmethodIDs are really
>> just weak JNI references so when their class is being unloaded they
>> all become null so it's impossible to look up the right jmethodID for
>> an nmethod which is being unloaded.  In the failing test this results
>> in a new jmethodID being allocated in the middle of the GC which can
>> later lead to crashes since root set has been extended so stale oops
>> are being processed in later phases of the GC.  The simplest fix is to
>> cache the jmethodID in the nmethod so that it can be used in the
>> unload notification.
>> I also added logic to complain if new weak references are created
>> while a GC is active.