RFR (XS): 8023004: JSR 292: java.lang.RuntimeException: Original target method was called.

[Christian Thalinger]

On Oct 18, 2013, at 1:25 AM, serguei.spitsyn at oracle.com wrote:

> Vladimir,
> 
> 
> Just want to share some details with you.
> 
> 
> On 10/17/13 5:46 AM, Vladimir Ivanov wrote:
>> Serguei, Chris, Igor, thanks for looking at this fix.
>> 
>> I need to look more thoroughly at this code.
>> I assumed that member name is initialized only once, but I was wrong - asserting empty slot in MethodHandles::init_method_MemberName fails during initialization of JSR292 core classes.
> 
> John or Chris, please, correct me if I'm wrong...
> But, I think, John had a plan in mind to utilize the MNT for uniqueness of member names.
> It was about all member names, not only method member names.

Correct.  It is worrying that the assert fires.  We should get to the bottom of this and fix it properly.

> 
> 
> Thanks,
> Serguei
> 
>> 
>> Best regards,
>> Vladimir Ivanov
>> 
>> On 10/17/13 1:39 PM, serguei.spitsyn at oracle.com wrote:
>>> On 10/16/13 1:28 PM, Christian Thalinger wrote:
>>>> On Oct 14, 2013, at 12:15 PM, Vladimir Ivanov
>>>> <vladimir.x.ivanov at oracle.com> wrote:
>>>> 
>>>>> http://cr.openjdk.java.net/~vlivanov/8023004/webrev.00/
>>>>> 7 lines changed: 5 ins; 1 del; 1 mod
>>>>> 
>>>>> Assertion code in JDK fetches info from VM using MHN.getMemberVMInfo:
>>>>> assert(m.vminfoIsConsistent());
>>>>> 
>>>>> After that check, class redefinition is broken - it's still possible
>>>>> to invoke old version of a method.
>>>>> 
>>>>> Analysis from Serguei:
>>>>> "The problem is here:
>>>>> oop MethodHandles::init_method_MemberName(Handle mname, CallInfo&
>>>>> info) {
>>>>>  . . . . . . . .
>>>>> m->method_holder()->add_member_name(m->method_idnum(), mname);
>>>>> 
>>>>>  return mname();
>>>>> }
>>>>> 
>>>>> The call to add_member_name() resets the member name at the index
>>>>> m->method_idnum(). After that one of the two member names with the
>>>>> same method_idnum() is out of the MNT and is not updated when the
>>>>> method is redefined."
>>>> As I understand this the problem is calling
>>>> MethodHandles::init_method_MemberName which installs a new MemberName
>>>> instance in the MemberNameTable.  Correct?
>>>> 
>>>> If that's the case then there is a potential race here.
>>>> 
>>>> +     x = m->method_holder()->get_member_name(m->method_idnum());
>>>> +     if (x == NULL) {
>>>> +       Handle mname2 = MethodHandles::new_MemberName(CHECK_NULL);
>>>>         x = MethodHandles::init_method_MemberName(mname2, info);
>>>>       }
>>>> 
>>>> Racing threads can get back null when calling get_member_name and then
>>>> install different MemberName objects in the table.
>>> 
>>> Good catch, Chris.
>>> I do not see a way to fix it easily without some refactoring.
>>> 
>>> It is probably better to add to the InstanceKlass a helper function that
>>> will do
>>> this dance with a required synchronization.
>>> But the following line needs to be taken out of the
>>> init_method_MemberName() :
>>> m->method_holder()->add_member_name(m->method_idnum(), mname);
>>> 
>>> 
>>> Thanks,
>>> Serguei
>>> 
>>>> 
>>>>> The fix is to check whether there's already a member name associated
>>>>> with the before creating a new one.
>>>>> 
>>>>> Testing: failing test.
>>>>> 
>>>>> Contributed-by: sspitsyn, vlivanov
>>>>> 
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>> 
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8023004
>>> 
>