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

[Daniel D. Daugherty]

Just one thing below...


On 7/22/15 7:25 PM, Coleen Phillimore wrote:
>
> 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.

Ahhh... That deserves a comment, but...
I ran into a case before where I needed:

    definition objectA;
    definition objectB;

    <other code>
    definition ret_value = something;

    return ret_value
    <objectB destructor>
    <objectA destructor>

but I couldn't find anything in C++ that guaranteed that the
destructors are run in the opposite order of definition.

What I ended up doing was this:

    definition objectA;
    definition ret_value;
    { // make sure objectB is destructed before objectA
      definition objectB;

      <other code>
      ret_value = something;
    } <objectB destructor>

    return ret_value
    <objectA destructor>

You might have better luck finding such a C++ guarantee,
but if not...

Dan

>>
>>     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
>>
>