[OpenJDK 2D-Dev] [9]: RFR JDK-8023213 [macosx] closed/java/awt/FontClass/NaNTransform.java fails on MacOS X 10.9

[prasanta sadhukhan]

Hi Phil,

I have updated webrev to explicitly set glyph image width/height to be 0 
in case of NaN transform. Could you please review it?
http://cr.openjdk.java.net/~psadhukhan/8023213/webrev.02/

Regards
Prasanta
On 11/17/2015 3:23 AM, Phil Race wrote:
> Would it not be better to be explicit in this so as not to leave it to 
> chance ?
>
> -phil.
>
> On 11/05/2015 11:49 PM, prasanta sadhukhan wrote:
>> Hi Phil,
>>
>> On 11/5/2015 4:54 AM, Philip Race wrote:
>>> This does not look right to me. Who knows what data is on the canvas ?
>>> it is not clear that it is 'blank'. It could have data from a 
>>> previous glyph .. I am not
>>> sure how you know for sure. I can see that canvas is re-used. There 
>>> is reference
>>> to a "global shared canvas".
>>> And the actual function you invoke is one of two : one for grey and 
>>> one for lcd and
>>> looking at the grey one CGGI_CopyImageFromCanvasToAlphaInfo)
>>> it utilises info->width and info->height which can't be NaN because 
>>> they
>>> are uint16 but I don't know if they are valid .. and is the "image" 
>>> field
>>> allocated to be 0 length ? 
>> info->width & height is coming out to be 0 in case of NaN transform 
>> therefore "image" field will be of 0 length so "empty" glyph will be 
>> copied when we copy the "glyph image" from canvas (no matter what is 
>> there in canvas) to info->image via this code
>> http://hg.openjdk.java.net/jdk9/client/jdk/file/298d3fe64572/src/java.desktop/macosx/native/libawt_lwawt/font/CGGlyphImages.m#l294 
>>
>>
>>
>> size_t destRow = y * destRowWidth;
>> size_t srcRow = y * srcRowWidth;
>> size_t x;
>> for (x = 0; x < destRowWidth; x++) {
>> UInt32 p = src[srcRow + x];
>> dest[destRow + x] = CGGI_ConvertBWPixelToByteGray(p);
>>
>> where destRowWidth = destRow = info->width will be 0 so dest = 
>> info->image will be of 0 length
>>
>> Regards
>> Prasanta
>>> Could you step through how this is all guaranteed
>>> to be safe/correct ?
>>>
>>> -phil.
>>>
>>> On 11/2/15, 12:41 AM, prasanta sadhukhan wrote:
>>>> Hi Phil,
>>>>
>>>> I have modified as per your review to populate GlyphInfo with 
>>>> "empty" glyph
>>>> and also moved your existing test to "open"
>>>> I also added a Infinite Transform test along with your NaN 
>>>> transform just incase (in fact Sergey informally asked me to check)
>>>>
>>>> http://cr.openjdk.java.net/~psadhukhan/8023213/webrev.01/
>>>>
>>>> Regards
>>>> Prasanta
>>>> On 10/30/2015 1:03 AM, Phil Race wrote:
>>>>> Should this new check go before this :
>>>>>
>>>>> CGGI_ClearCanvas(canvas, info);
>>>>>
>>>>> since it is using info which is where you get NaN from.
>>>>>
>>>>>
>>>>> And should we then populate the returned canvas and info to
>>>>> ensure that we return an "empty" glyph rather than garbage values ?
>>>>> It is not clear to me that this is occurring.
>>>>>
>>>>> Why does the bug report not contain the evaluation below ?
>>>>> Also why is there a new test ? I would expect SQE would
>>>>> want to run the existing test to verify this fix.
>>>>> Should we not just open the existing test ?
>>>>>
>>>>>
>>>>> -phil.
>>>>>
>>>>> On 10/13/2015 04:49 AM, prasanta sadhukhan wrote:
>>>>>> Gentle reminder for review
>>>>>>
>>>>>> Regards
>>>>>> Prasanta
>>>>>> On 10/6/2015 3:25 PM, prasanta sadhukhan wrote:
>>>>>>> Hello All,
>>>>>>>
>>>>>>> Please review a fix for jdk9
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8023213
>>>>>>> webrev: http://cr.openjdk.java.net/~psadhukhan/8023213/webrev.00/
>>>>>>>
>>>>>>> drawString takes long time in mac native call 
>>>>>>> CGContextShowGlyphsAtPoint() if NaN transform is used which 
>>>>>>> translated to x & y being NaN.
>>>>>>> Fix is to prevent calling mac api for such invalid usage.
>>>>>>>
>>>>>>> Regards
>>>>>>> Prasanta
>>>>>>
>>>>>
>>>>
>>
>