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

[Sergey Bylokhov]

Looks fine.

On 11/01/16 12:00, prasanta sadhukhan wrote:
> Hi Sergey, Phil,
>
> I guess as we discussed offline, it probably is not possible to do the
> fix in java as you thought and approved webrev.02 offline.
> But as we found the fix, even though it works in osx9 it does not work
> for osx10 because CTFontCreatePathForGlyph() cannot accept NaN transform
> on osx10 onwards resulting in crash
> /
> Assertion failed: (transform_is_valid(m)), function CGMutablePathRef
> CGPathCreateMutableCopyByTransformingPath(CGPathRef, const
> CGAffineTransform *), file Paths/CGPath.cc, line 168. //
> //Abort trap: 6//
> /
> The below webrev takes care of the crash is osx10.
> http://cr.openjdk.java.net/~psadhukhan/8023213/webrev.03/
>
> Can you please review and approve the same?
>
> Regards
> Prasanta
> On 11/19/2015 7:46 PM, Sergey Bylokhov wrote:
>> I still think that possibly it will be better to block such transforms
>> on the upper level, somewhere in the java probably in CStrike.java.
>>
>>
>> On 17.11.15 9:12, prasanta sadhukhan wrote:
>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.