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

[Shafi Ahmad]

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.

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