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