Bug 222402 – REGRESSION(r269957): Empty font names passed to canvas2d cause all text routines to crash

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

REGRESSION(r269957): Empty font names passed to canvas2d cause all text routines to crash

[Myles C. Maxfield]

Created attachment 421497 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/72621268>

[Chris Lord]

Comment on attachment 421497 [details]
Patch

This looks good to me - I especially like the added assert and tests. Indeed, it was an oversight in the original change, it looks like I missed the `if (parsedStyle->isEmpty()) return` that was previously there... Doh!

[Darin Adler]

Comment on attachment 421497 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:137
> +    modifiableState().unparsedFont = newFontSafeCopy;
> +
> +    modifiableState().font.initialize(document.fontSelector(), *fontStyle);

I think this code is easier to understand if these two are in a single paragraph, not two separate ones.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:139
> +    ASSERT(state().font.realized() && state().font.isPopulated());

Why not two separate asserts? That way it’s slightly easier to tell which of the two failed.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:238
> +        bool isPopulated() const { return m_font.fonts(); }

It’s a little frustrating to make something like this public just so we can do an assertion. Minimizing interfaces often makes the code more maintainable in the long run.

[Myles C. Maxfield]

Comment on attachment 421497 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:238
>> +        bool isPopulated() const { return m_font.fonts(); }
> 
> It’s a little frustrating to make something like this public just so we can do an assertion. Minimizing interfaces often makes the code more maintainable in the long run.

I can surround it with #if ASSERT_ENABLED

[Myles C. Maxfield]

Created attachment 421551 [details]
Patch for committing

[Darin Adler]

Comment on attachment 421497 [details]
Patch

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

>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:238
>>> +        bool isPopulated() const { return m_font.fonts(); }
>> 
>> It’s a little frustrating to make something like this public just so we can do an assertion. Minimizing interfaces often makes the code more maintainable in the long run.
> 
> I can surround it with #if ASSERT_ENABLED

Well, I don’t think that makes it better, but I don’t object to doing that if you do think it does.

[EWS]

Committed r273512: <https://commits.webkit.org/r273512>

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