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

[David Holmes]

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

---

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