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

[Philip Race]

I don't think it is impossible to fix in Java and it might even be 
preferable
except that we perhaps need the low-level check anyway to be safe 
(definitely the
case for the crash) and I was not sure we figured out the best place to 
put in Java code
to minimise performance issues on normal cases.

Anyway this seems like it should resolve the issues.

Approved.

-phil.

On 1/11/16, 1:00 AM, 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20160112/766caebe/attachment.html>