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

[Shafi Ahmad]

Hi,

Pease find updated webrev. I have incorporated both the comments by David. 

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

Regards,
Shafi

> -----Original Message-----
> From: Shafi Ahmad
> Sent: Monday, August 01, 2016 12:18 PM
> To: David Holmes; 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 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
> > >>