Thanks for the time spent ;-) On 5 January 2017 at 08:08, Ambarish Rapte <ambarish.ra...@oracle.com> wrote:
> Hi David, > +1 > Looks good to me. > > Regards, > Ambarish > > -----Original Message----- > From: Phil Race > Sent: Thursday, January 05, 2017 12:39 AM > To: David CARLIER; awt-dev@openjdk.java.net > Subject: Re: <AWT Dev> RFR JDK-8171836: Memory leak in > java.desktop/unix/native/common/awt/fontpath.c > > This looks correct. > > +1 > > I can commit this if we get a 2nd reviewer to approve. > > -phil. > > On 01/04/2017 11:05 AM, David CARLIER wrote: > > Hi here an updated version. Thanks. regards. > > > > > >>>>>>>> Whilst the loop termination condition of "< totalDirCount" is > >>>>>>>> fine in the code from which you have copied it, it is not > >>>>>>>> correct here, since you are still in the loop which populates the > array. > >>>>>>>> The likely resulting crash will be a lot worse than the leak .. > >>>>>>>> > >>>>>>>> Also I am confused about who is the OCA contributor of this > >>>>>>>> fix, and who is the sponsor (JDK 9 committer) for this fix. > >>>>>>>> > >>>>>>>> And, BTW, this should really have been reviewed on 2d-dev. > >>>>>>>> > >>>>>>>> > >>>>>>>> -phil. > >>>>>>>> > >>>>>>>> > >>>>>>>> On 01/03/2017 12:13 PM, David CARLIER wrote: > >>>>>>>>> Hi all, > >>>>>>>>> > >>>>>>>>> this is a new version. > >>>>>>>>> > >>>>>>>>> Cheers, > >>>>>>>>> > >>>>>>>>> On 22 December 2016 at 05:35, Ambarish > >>>>>>>>> Rapte<ambarish.ra...@oracle.com> > >>>>>>>>> wrote: > >>>>>>>>>> Hi Abhijit, > >>>>>>>>>> > >>>>>>>>>> There can be references added to > >>>>>>>>>> newFontPath[] in previous iterations of loop at Line 298: > >>>>>>>>>> newFontPath[nPaths++] = onePath; > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> So these references should also be > >>>>>>>>>> freed, something similar to code at line 308: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> for ( index = origNumPaths; index< > >>>>>>>>>> totalDirCount; > >>>>>>>>>> index++ ) > >>>>>>>>>> { > >>>>>>>>>> > >>>>>>>>>> free( newFontPath[index] > >>>>>>>>>> ); > >>>>>>>>>> > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> As many references added to newFontPath should be freed > >>>>>>>>>> accordingly. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Regards, > >>>>>>>>>> > >>>>>>>>>> Ambarish > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> From: Abhijit Roy > >>>>>>>>>> Sent: Thursday, December 22, 2016 12:08 AM > >>>>>>>>>> To: Vadim Pakhnushev; awt-dev@openjdk.java.net > >>>>>>>>>> Subject: Re:<AWT Dev> RFR JDK-8171836: Memory leak in > >>>>>>>>>> java.desktop/unix/native/common/awt/fontpath.c > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Hi Vadim, > >>>>>>>>>> > >>>>>>>>>> Yes. I did a mistake here. Please find the correct webrev below. > >>>>>>>>>> > >>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rpatil/8171836/webrev.01/ > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Thanks > >>>>>>>>>> Abhijit > >>>>>>>>>> > >>>>>>>>>> On 12/21/2016 7:47 PM, Vadim Pakhnushev wrote: > >>>>>>>>>> > >>>>>>>>>> Abhijit, > >>>>>>>>>> I think there's some misunderstanding here. > >>>>>>>>>> The pointer you are trying to free is NULL already: > >>>>>>>>>> > >>>>>>>>>> if ( newFontPath == NULL ) { > >>>>>>>>>> free ( ( void *) appendDirList ); > >>>>>>>>>> + free((void*) newFontPath); > >>>>>>>>>> > >>>>>>>>>> Thanks, > >>>>>>>>>> Vadim > >>>>>>>>>> > >>>>>>>>>> On 21.12.2016 16:02, Abhijit Roy wrote: > >>>>>>>>>> > >>>>>>>>>> Hi all, > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Please review the fix for the bug below: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8171836 > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Description: Memory leak in > >>>>>>>>>> java.desktop/unix/native/common/awt/fontpath.c > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rpatil/8171836/webrev.00/ > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> To prevent memory leak issue, I have released the newFontPath > >>>>>>>>>> in java.desktop/unix/native/common/awt/fontpath. > >>>>>>>>>> > >>>>>>>>>> Moving forward it for review. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Regards, > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Abhijit > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >