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

[Vladimir Kozlov]

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?

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

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

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.