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

[David Holmes]

On 1/08/2016 4:48 PM, Shafi Ahmad wrote:
> Hi David,
>
> Thanks for the review.
>
>> -----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.
>
> I replied to her mail, this is not reproducible in jdk9. I have updated the bug with 9-na label.
>
>>
>> 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?
>
> Yes, you are right.  I assume the array  b->_methods will not have duplicate entry.
>
>>
>> Nit: need space after i in i<
>
> I will send updated webrev.

You did not respond to the rest of my comments.

David
------

> Regards,
> Shafi
>
>>
>> ---
>>
>> 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
>>
>> ---
>>
>>  >>> 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:
>>
>> 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!
>>
>> 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
>>>>