RFR 8022887: Assertion hit while using class and redefining it with RedefineClasses s,imultaneously

[Coleen Phillimore]

Hi,  The new webrev is larger now.  I found code where Method* can be 
leaked because methodHandles are not freed, and have rewritten 
JVM_GetClassDeclaredMethods and JVM_GetClassDeclaredConstructors to be 
redefinition safe.

http://cr.openjdk.java.net/~coleenp/8022887_2/ 
<http://cr.openjdk.java.net/%7Ecoleenp/8022887_2/>

Tested with internal vm.quick.testlist and mlvm tests.

Thanks,
Coleen


On 9/5/2013 2:20 PM, serguei.spitsyn at oracle.com wrote:
> Coleen,
>
> This is great finding, and also a nice catch by Dan.
> Waiting for a new webrev from you.
>
> Thanks,
> Serguei
>
> On 9/5/13 9:35 AM, Coleen Phillimore wrote:
>>
>> Dan,
>> Thank you for looking at this so quickly.   You are right, we are not 
>> only getting public methods, whose number cannot change right now 
>> with redefine classes.
>> I have to rework this change.
>> Thanks,
>> Coleen
>>
>> On 9/5/2013 12:23 PM, Daniel D. Daugherty wrote:
>>> On 9/5/13 9:33 AM, Coleen Phillimore wrote:
>>>> Summary: Need to refetch the methods array from InstanceKlass after 
>>>> safepoint.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8022887/
>>>
>>> The "frames" links are broken in this webrev. I had to
>>> write down the changed line numbers for jvm.cpp and then
>>> use the "new" link to see the context of the changes.
>>>
>>> src/share/vm/oops/instanceKlass.cpp
>>>     Nice catch. The old code could return an 'm' value that
>>>     referred to a method that wasn't a match. Ouch.
>>>
>>
>> yes, it was a bit of a red herring for a while.
>>
>>> src/share/vm/prims/jvm.cpp
>>>     Nice catch of the use of potentially stale method array, but I
>>>     think there might be more issues here.
>>>
>>>     In JVM_GetClassDeclaredMethods:
>>>
>>>     line 1865: ++num_methods;
>>>
>>> <snip>
>>>
>>>     line 1871: objArrayOop r = 
>>> oopFactory::new_objArray(SystemDictionary::reflect_Method_klass(), 
>>> num_methods, CHECK_NULL);
>>>
>>> <snip>
>>>
>>>     line 1876: methods = k->methods();
>>>     line 1877: methods_length = methods->length();
>>>
>>> <snip>
>>>
>>>     line 1885: result->obj_at_put(out_idx, m);
>>>
>>> <snip>
>>>
>>>     line 1890:  assert(out_idx == num_methods, "just checking");
>>>
>>>         So num_methods is computed before the new_objArray() call that
>>>         can result in a safepoint which can permit a RedefineClasses()
>>>         operation to complete. You refresh methods and methods_length,
>>>         but num_methods still has its pre-RedefineClasses value and
>>>         the size of the result array is also at the pre-RedefineClasses
>>>         size. Isn't it possible that we could overflow the result array
>>>         here? And maybe fire that assert() on line 1890.
>>>
>>>
>>>     In JVM_GetClassDeclaredConstructors(), similar concerns for these
>>>     lines:
>>>
>>>     line 1922: ++num_constructors;
>>>
>>> <snip>
>>>
>>>     line 1928: objArrayOop r = 
>>> oopFactory::new_objArray(SystemDictionary::reflect_Constructor_klass(), 
>>> num_constructors, CHECK_NULL);
>>>
>>> <snip>
>>>
>>>     line 1942: result->obj_at_put(out_idx, m);
>>>
>>> <snip>
>>>
>>>     line 1947: assert(out_idx == num_constructors, "just checking");
>>>
>>>
>>>     Yes, this RedefineClasses() stuff is a serious pain in the butt
>>>     because it can change your assumed invariants in the middle of
>>>     your function.
>>>
>>> Dan
>>>
>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8022887
>>>>
>>>> Tested with the test cases in the bug, and with internal SQE tests 
>>>> (nsk.quick.testlist).
>>>>
>>>> thanks,
>>>> Coleen
>>>
>>
>