Bug 219177 – zero and rename boundingRect pointer in Font::platformBoundsForGlyph() to CTFontGetBoundingRectsForGlyphs()

• Component: Text
• Status: RESOLVED FIXED

[Julian Gonzalez]

Follow up to 218812 - zero out the CGRect structures on the stack from that change, so as not to pass uninitialized memory to CTFontGetBoundingRectsForGlyphs().

<rdar://problem/71547171>

[Julian Gonzalez]

Created attachment 414618 [details]
Patch

[Darin Adler]

Comment on attachment 414618 [details]
Patch

Good to have the code match what the change log says. And it was my comment that led to this change.

But why does this need to be a zero rect rather than uninitialized? And why can’t we pass nullptr instead?

A little confused about that.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #2)
> Comment on attachment 414618 [details]
> Patch
> 
> Good to have the code match what the change log says. And it was my comment
> that led to this change.

Yeah, we should mention this in the change log.

> But why does this need to be a zero rect rather than uninitialized? And why
> can’t we pass nullptr instead?

So the person who maintains this function told us that passing nullptr will result in heap memory allocation so it's slightly more efficient to pass in a rect.

[Julian Gonzalez]

Created attachment 417721 [details]
Patch

[Darin Adler]

Comment on attachment 417721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417721&action=review

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:703
> +    CGRect zeroRect = { };
> +    boundingBox = CTFontGetBoundingRectsForGlyphs(m_platformData.ctFont(), platformData().orientation() == FontOrientation::Vertical ? kCTFontOrientationVertical : kCTFontOrientationHorizontal, &glyph, &zeroRect, 1);

After more research I now realize this is an *out* argument from CTFontGetBoundingRectsForGlyphs. So it must not be named "emptyRect" or "zeroRect", since it will get a value and will not be empty or zero! Instead we should pass nullptr. The CTFontGetBoundingRectsForGlyphs documentation for this arguments says "Can be NULL, in which case only the overall bounding rect is calculated." I suggest we use nullptr.

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:748
> +        CGRect zeroRect = { };
> +        if (!CGRectIsEmpty(CTFontGetBoundingRectsForGlyphs(platformFont, kCTFontOrientationDefault, &lowercaseAGlyph, &zeroRect, 1)))

Ditto.

[Julian Gonzalez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 417721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417721&action=review
> 
> > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:703
> > +    CGRect zeroRect = { };
> > +    boundingBox = CTFontGetBoundingRectsForGlyphs(m_platformData.ctFont(), platformData().orientation() == FontOrientation::Vertical ? kCTFontOrientationVertical : kCTFontOrientationHorizontal, &glyph, &zeroRect, 1);
> 
> After more research I now realize this is an *out* argument from
> CTFontGetBoundingRectsForGlyphs. So it must not be named "emptyRect" or
> "zeroRect", since it will get a value and will not be empty or zero! Instead
> we should pass nullptr. The CTFontGetBoundingRectsForGlyphs documentation
> for this arguments says "Can be NULL, in which case only the overall
> bounding rect is calculated." I suggest we use nullptr.
> 

Darin, the reason for 218812 was that passing nullptr is not as performant as passing stack-allocated memory, because a heap allocation is performed in that case.
This is what Ryosuke was getting at when he wrote:

"So the person who maintains this function told us that passing nullptr will result in heap memory allocation so it's slightly more efficient to pass in a rect."

> > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:748
> > +        CGRect zeroRect = { };
> > +        if (!CGRectIsEmpty(CTFontGetBoundingRectsForGlyphs(platformFont, kCTFontOrientationDefault, &lowercaseAGlyph, &zeroRect, 1)))
> 
> Ditto.

If we want to ignore the recommendation and just use nullptr, then we should just revert 218812.

[Darin Adler]

Comment on attachment 417721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417721&action=review

>>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:703
>>> +    boundingBox = CTFontGetBoundingRectsForGlyphs(m_platformData.ctFont(), platformData().orientation() == FontOrientation::Vertical ? kCTFontOrientationVertical : kCTFontOrientationHorizontal, &glyph, &zeroRect, 1);
>> 
>> After more research I now realize this is an *out* argument from CTFontGetBoundingRectsForGlyphs. So it must not be named "emptyRect" or "zeroRect", since it will get a value and will not be empty or zero! Instead we should pass nullptr. The CTFontGetBoundingRectsForGlyphs documentation for this arguments says "Can be NULL, in which case only the overall bounding rect is calculated." I suggest we use nullptr.
> 
> Darin, the reason for 218812 was that passing nullptr is not as performant as passing stack-allocated memory, because a heap allocation is performed in that case.
> This is what Ryosuke was getting at when he wrote:
> 
> "So the person who maintains this function told us that passing nullptr will result in heap memory allocation so it's slightly more efficient to pass in a rect."

Oops, forgot that. Then we should not name this "zero rect" but otherwise this is great.

[Julian Gonzalez]

(In reply to Darin Adler from comment #7)
> Comment on attachment 417721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417721&action=review
> 
> >>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:703
> >>> +    boundingBox = CTFontGetBoundingRectsForGlyphs(m_platformData.ctFont(), platformData().orientation() == FontOrientation::Vertical ? kCTFontOrientationVertical : kCTFontOrientationHorizontal, &glyph, &zeroRect, 1);
> >> 
> >> After more research I now realize this is an *out* argument from CTFontGetBoundingRectsForGlyphs. So it must not be named "emptyRect" or "zeroRect", since it will get a value and will not be empty or zero! Instead we should pass nullptr. The CTFontGetBoundingRectsForGlyphs documentation for this arguments says "Can be NULL, in which case only the overall bounding rect is calculated." I suggest we use nullptr.
> > 
> > Darin, the reason for 218812 was that passing nullptr is not as performant as passing stack-allocated memory, because a heap allocation is performed in that case.
> > This is what Ryosuke was getting at when he wrote:
> > 
> > "So the person who maintains this function told us that passing nullptr will result in heap memory allocation so it's slightly more efficient to pass in a rect."
> 
> Oops, forgot that. Then we should not name this "zero rect" but otherwise
> this is great.

Sounds good. How does 'dummyRect' sound? (possibly with a comment?)

[Darin Adler]

Comment on attachment 417721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417721&action=review

>>>>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:703
>>>>> +    boundingBox = CTFontGetBoundingRectsForGlyphs(m_platformData.ctFont(), platformData().orientation() == FontOrientation::Vertical ? kCTFontOrientationVertical : kCTFontOrientationHorizontal, &glyph, &zeroRect, 1);
>>>> 
>>>> After more research I now realize this is an *out* argument from CTFontGetBoundingRectsForGlyphs. So it must not be named "emptyRect" or "zeroRect", since it will get a value and will not be empty or zero! Instead we should pass nullptr. The CTFontGetBoundingRectsForGlyphs documentation for this arguments says "Can be NULL, in which case only the overall bounding rect is calculated." I suggest we use nullptr.
>>> 
>>> Darin, the reason for 218812 was that passing nullptr is not as performant as passing stack-allocated memory, because a heap allocation is performed in that case.
>>> This is what Ryosuke was getting at when he wrote:
>>> 
>>> "So the person who maintains this function told us that passing nullptr will result in heap memory allocation so it's slightly more efficient to pass in a rect."
>> 
>> Oops, forgot that. Then we should not name this "zero rect" but otherwise this is great.
> 
> Sounds good. How does 'dummyRect' sound? (possibly with a comment?)

That’s fine. I might also suggest "ignoredRect".

[Julian Gonzalez]

Created attachment 417743 [details]
Patch

[Darin Adler]

Comment on attachment 417743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417743&action=review

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:702
> +    CGRect ignoredRect = { };

No reason for the "= { }". The name emptyRect confused me into thinking it was needed. So the only thing wrong was the name, the code was totally fine. This patch is OK as is, but also one that just ranges would be fine. And the bug could be retitled since this is just naming clarity, no need to zero anything.

[Darin Adler]

Comment on attachment 417743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417743&action=review

>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:702
>> +    CGRect ignoredRect = { };
> 
> No reason for the "= { }". The name emptyRect confused me into thinking it was needed. So the only thing wrong was the name, the code was totally fine. This patch is OK as is, but also one that just ranges would be fine. And the bug could be retitled since this is just naming clarity, no need to zero anything.

that just *renames*, not that just *ranges*

[Ryosuke Niwa]

Comment on attachment 417743 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417743&action=review

>>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:702
>>> +    CGRect ignoredRect = { };
>> 
>> No reason for the "= { }". The name emptyRect confused me into thinking it was needed. So the only thing wrong was the name, the code was totally fine. This patch is OK as is, but also one that just ranges would be fine. And the bug could be retitled since this is just naming clarity, no need to zero anything.
> 
> that just *renames*, not that just *ranges*

It's probably still a good practice to zero it out so that there is no ambiguity we won't have indeterministic behavior here.

[Julian Gonzalez]

OK - I'll rename the variable (keeping the zeroing as requested by Ryosuke) and retitle the bug here slightly to reflect the new plan. Thanks folks.

[Julian Gonzalez]

Created attachment 418080 [details]
Patch

[EWS]

Committed r271715: <https://trac.webkit.org/changeset/271715>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418080 [details].