[8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)

[Coleen Phillimore]

On 8/1/16 7:19 AM, David Holmes wrote:
> Hi Shafi,
>
> On 1/08/2016 6:47 PM, Shafi Ahmad wrote:
>> Hi David,
>>
>> Sorry for my half mail.
>>
>>> -----Original Message-----
>>> From: David Holmes
>>> Sent: Monday, August 01, 2016 5:32 AM
>>> To: Shafi Ahmad; Coleen Phillimore; 
>>> hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: [8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash in
>>> Method::checked_resolve_jmethod_id(_jmethodID*)
>>>
>>> Hi Shafi,
>>>
>>> On 30/07/2016 1:10 AM, Shafi Ahmad wrote:
>>>> Hi All,
>>>>
>>>> Could I have 2nd Reviewer's review for this change, please?
>>>
>>> I didn't see you respond to Coleen's query re JDK9. If this is not 
>>> applicable to
>>> JDK9 please add a 9-na label to the bug report.
>>>
>>> Looking at the code:
>>>
>>> +   void clear_method(Method* m) {
>>> +     for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
>>> +       for (int i = 0; i< number_of_methods; i++) {
>>> +         if (b->_methods[i] == m) {
>>> +           b->_methods[i] = NULL;
>>> +         }
>>> +       }
>>> +     }
>>> +     // not found
>>> +   }
>>>
>>> Based on the "not found" comment I assume you intended to do a return
>>> after NULLing out the method?
>>>
>>> Nit: need space after i in i<
>>>
>>> ---
>>>
>>> 1882 void Method::clear_jmethod_id(ClassLoaderData* loader_data) {
>>> 1883   loader_data->jmethod_ids()->clear_method(this);
>>> 1884 }
>>>
>>> Not sure why you felt the need to add a new member function for this
>>> instead of just doing line #1883 directly at line #113
>>
>> Just to make it consistent with the existing method like void 
>> Method::clear_jmethod_ids(ClassLoaderData* loader_data).
>
> I would not add to the API unnecessarily as it just makes the API 
> harder to understand.

I think this API is good rather than telling Method::deallocate_contents 
that there's a jmethod_ids pointer in the class_loader data.   If this 
changes for some reason, it would be good to change it together with the 
other APIs.

Shafi, can you move this function down to below Method::set_on_stack?  
Then it'll be clearer that it belongs with Method::clear_jmethod_ids().

Thanks,
Coleen
>
>>> ---
>>>
>>>  >>> After this change I am seeing Method::is_method_id() is 
>>> getting  >>>
>>> called with NULL and I have done below change to avoid crash >>>  >>> -
>>> assert(m != NULL, "should be called with non-null method"); >>> +  
>>> if (m ==
>>> NULL) {
>>>  >>> +    return false;
>>>  >>> +  }
>>>
>>> This is the only call I can see to is_method_id:
>>
>> Yes, this is the only call.
>>
>>>
>>> 1870 Method* Method::checked_resolve_jmethod_id(jmethodID mid) {
>>> 1871   if (mid == NULL) return NULL;
>>> 1872   if (!Method::is_method_id(mid)) {
>>> 1873     return NULL;
>>> 1874   }
>>>
>>
>>
>>
>>> So I don't see how it can be being passed NULL. If it is then you 
>>> have a
>>> problem!
>>
>> Here actual parameter 'mid"  of  method is_method_id is not null but 
>> this  we are calling another method resolve_jmethod_id(mid) which 
>> returns NULL i.e m becomes null in below code.
>>
>> 1885 bool Method::is_method_id(jmethodID mid) {
>> 1886   Method* m = resolve_jmethod_id(mid);
>> 1887   if (m == NULL) {
>> 1888     return false;
>> 1889   }
>
> Ah I see - sorry. Thanks for clarifying.
>
> David
>
>>
>> Regards,
>> Shafi
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>>> Regards,
>>>> Shafi
>>>>
>>>>> -----Original Message-----
>>>>> From: Coleen Phillimore
>>>>> Sent: Monday, July 25, 2016 5:52 PM
>>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>>> Subject: Re: [8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash
>>>>> in
>>>>> Method::checked_resolve_jmethod_id(_jmethodID*)
>>>>>
>>>>>
>>>>> This looks good.  Was this a backport or is it still broken in 9?
>>>>> thanks,
>>>>> Coleen
>>>>>
>>>>> On 7/25/16 7:53 AM, Shafi Ahmad wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the small code change for bug: "JDK-8161144: Fix for
>>>>>> JDK-
>>>>> 8147451 failed: Crash in
>>>>> Method::checked_resolve_jmethod_id(_jmethodID*)" on jdk8u-dev
>>>>>>
>>>>>> Summary:
>>>>>> Method::deallocate_contents() should clear 'this' from list of
>>>>>> Methods in
>>>>> JNIMethodBlock, similarly to clear_all_methods() does it, when class
>>>>> is unloaded.
>>>>>> After this change I am seeing Method::is_method_id() is getting
>>>>>> called with
>>>>> NULL and I have done below change to avoid crash
>>>>>>
>>>>>> -  assert(m != NULL, "should be called with non-null method");
>>>>>> +  if (m == NULL) {
>>>>>> +    return false;
>>>>>> +  }
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~shshahma/8161144/webrev/
>>>>>> Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8161144
>>>>>>
>>>>>> Test:  Run jprt
>>>>>>
>>>>>> Regards,
>>>>>> Shafi
>>>>>