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