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

[Shafi Ahmad]

Hi,

Please find updated webrev. 

http://cr.openjdk.java.net/~shshahma/8161144/webrev.02/

Regards,
Shafi 

> -----Original Message-----
> From: Coleen Phillimore
> Sent: Monday, August 01, 2016 6:01 PM
> To: David Holmes; Shafi Ahmad; 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*)
> 
> 
> 
> 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
> >>>>>
>