RFR 8082782: vm crash on StressRedefineWithoutBytecodeCorruption fails with assert(((Metadata*)obj)->is_, valid()) failed: obj is valid

[Coleen Phillimore]

Dan,

Thank you for the quick code review.

On 7/22/15 8:35 PM, Daniel D. Daugherty wrote:
> On 7/22/15 10:20 AM, Coleen Phillimore wrote:
>> Summary: Walk compile task for Method* to not deallocate, store 
>> methods in methodHandle while compile task is being created
>
> Wait... 'while compile task is being created'...
> You're holding these handles while the compile task
> is being removed... Am I confused here?
>

No, you're not confused, you are right, we're getting the compile task 
is being taken off the compile queue.
>
>> I couldn't reproduce this or write a test with the exact timing 
>> needed to get this crash, but this is what the stack trace and code 
>> inspection tells me what happened. Reran all redefinition and 
>> internal tonga 'quick' tests on the change with no regression.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8082782.01/
>
> src/share/vm/compiler/compileBroker.hpp
>     No comments.
>
> src/share/vm/compiler/compileBroker.cpp
>     Is there a reason to have save_method and save_hot_method
>     defined all the way up at lines 671-672 instead of
>     being defined and init'ed on lines 710-711?

The methodHandle destructor has to run after the mutexLocker 
destructor.   The methodHandle destructors take it off the list of 
Methods to mark on stack to not deallocate, but have to run after the 
MutexLocker destructor so it has to be declared first.
>
>     L708:   // Save method pointer across
>         Typo: 'pointer' -> 'pointers'

Fixed.
>
>     So the idea of this fix is that you're holding methodHandles for
>     these method pointers while removing the task because removing
>     the task can go to a safepoint. While we're at that safepoint,
>     a RedefineClasses call can come along and try and access/use the
>     method pointers. 

RedefineClasses will walk the previous versions of the method's holder 
class and determine that nothing is using the old methods, and put them 
on the list to deallocate the next time we unload classes.   The 
methodHandle marks these methods as being used (on_stack) across these 
safepoints so they stick around.

> If we didn't have methodHandles on them, they
>     could be freed by the time RedefineClasses tries to access/use
>     them...
>

They'd be freed by the time the following code uses them (if you 
redefine the class and subsequently do some class unloading GC, not 
terribly likely but can happen in this small window and seems to have 
happened in this crash from the dump).  It's like a naked methodOop 
before permgen elimination, except it doesn't move, it gets deallocated.

The is_valid() debugging function partially detects that something has 
written over this bit of the metadata.  Deallocation writes 0xf5 over 
the stuff it deallocates.

> Do I have this right? Ouch and nice find.
>
thanks!
> src/share/vm/runtime/thread.cpp
>     No comments.
>
>

Thank you!!
Coleen

>> bug link https://bugs.openjdk.java.net/browse/JDK-8082782
>>
>> Thanks,
>> Coleen
>