Bug 66291 – Crash because CSSPrimitiveValue::computeLengthDouble assumes fontMetrics are available

• Component: CSS
• Status: RESOLVED FIXED

[Berend-Jan Wever]

Created attachment 104023 [details]
Repro

Repro:
<script>
  oContext2d=document.getCSSCanvasContext("2d","",0);
  oContext2d.font="small-caps.0ex G";
</script>

double CSSPrimitiveValue::computeLengthDouble(RenderStyle* style, RenderStyle* rootStyle, double multiplier, bool computingFontSize)
{
<<<snip>>>
    switch (type) {
<<<snip>>>
        case CSS_EXS:
            // FIXME: We have a bug right now where the zoom will be applied twice to EX units.
            // We really need to compute EX using fontMetrics for the original specifiedSize and not use
            // our actual constructed rendering font.
            applyZoomMultiplier = false;
            factor = style->fontMetrics().xHeight();
            break;

There is no font list (yet), leading to an ASSERT and NULL ptr here:

inline const SimpleFontData* Font::primaryFont() const
{
    ASSERT(m_fontList);
    return m_fontList->primarySimpleFontData(this);
}

id:             chrome.dll!WebCore::RenderStyle::fontMetrics ReadAV@NULL (73a53e2ce303ee898f439c13af2ceffd)
description:    Attempt to read from unallocated NULL pointer+0x30 in chrome.dll!WebCore::RenderStyle::fontMetrics
stack:          chrome.dll!WebCore::RenderStyle::fontMetrics
                chrome.dll!WebCore::CSSPrimitiveValue::computeLengthDouble
                chrome.dll!WebCore::CSSPrimitiveValue::computeLength<...>
                chrome.dll!WebCore::CSSStyleSelector::applyProperty
                chrome.dll!WebCore::CSSStyleSelector::applyProperty
                chrome.dll!WebCore::CSSStyleSelector::applyPropertyToStyle
                chrome.dll!WebCore::CanvasRenderingContext2D::setFont
                chrome.dll!WebCore::CanvasRenderingContext2DInternal::fontAttrSetter
                chrome.dll!v8::internal::JSObject::SetPropertyWithCallback
                chrome.dll!v8::internal::JSObject::SetPropertyForResult
                chrome.dll!v8::internal::JSReceiver::SetProperty
                chrome.dll!v8::internal::StoreIC::Store
                chrome.dll!v8::internal::StoreIC_Miss
                ...

[Berend-Jan Wever]

Chromium https://code.google.com/p/chromium/issues/detail?id=93035

[Simon Fraser (smfr)]

<rdar://problem/9970343>

[Julien Chaffraix]

Created attachment 108695 [details]
Proposed work-around: Add a manual update.

[Darin Adler]

Comment on attachment 108695 [details]
Proposed work-around: Add a manual update.

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

> Source/WebCore/css/CSSStyleSelector.cpp:2987
> +            updateFont();

This needs a “why” comment. It’s not at all clear why you need to call this here, and not, say, between each call to applyProperty. The comment should answer that question.

[Julien Chaffraix]

Created attachment 108747 [details]
Proposed work-around 2: Added a 'why' comment that should answer Darin's request.

[WebKit Review Bot]

Comment on attachment 108747 [details]
Proposed work-around 2: Added a 'why' comment that should answer Darin's request.

Clearing flags on attachment: 108747

Committed r96122: <http://trac.webkit.org/changeset/96122>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

One more thought post-landing. Now that I read the comment explaining why updateFont is needed, I am puzzled. If the font size is indeed using the "ex" unit, does that mean the font size is based on the font itself? Doesn’t the "ex" part have to be based on the font of the container or the inherited font or something? How can a font size be based on the font it’s sizing? It seems that if I specified "1ex" then there would be a sort of infinite recursion resulting in an infinitely small font.

[Julien Chaffraix]

(In reply to comment #8)
> One more thought post-landing. Now that I read the comment explaining why updateFont is needed, I am puzzled. If the font size is indeed using the "ex" unit, does that mean the font size is based on the font itself?

That's what is happening here. That's because the applyPropertyToStyle sets parentStyle to the current style.

> Doesn’t the "ex" part have to be based on the font of the container or the inherited font or something?

Normally you would use your parent's font size (which is what CSS defines). For canvas, the spec does not say whether we should use the parent's style to resolve any ambiguous properties like this case.

> How can a font size be based on the font it’s sizing?

People using 'ex' in this case likely have the wrong expectations. However nothing prevents CanvasRenderingContext from adding this circular dependency.

> It seems that if I specified "1ex" then there would be a sort of infinite recursion resulting in an infinitely small font.

This will actually not happen. We restart from a clean style every time you set the font on the CanvasRenderingContext. Also the style application in this case would resolve the font size once.

[Csaba Osztrogonác]

(In reply to comment #0)
> <script>
>   oContext2d=document.getCSSCanvasContext("2d","",0);
>   oContext2d.font="small-caps.0ex G";
> </script>

This tests fails on all platform with:
"CONSOLE MESSAGE: line 8: TypeError: Not enough arguments"

getCSSCanvasContext() has 4 parameters, and you passed only 3 parameters.

[Csaba Osztrogonác]

I tried to add 0 as a 4. parameter and the test pass for me:

- oContext2d=document.getCSSCanvasContext("2d","",0);
+ oContext2d=document.getCSSCanvasContext("2d","",0,0);

[Gabor Rapcsanyi]

Patch landed with this change in r96204.
http://trac.webkit.org/changeset/96204

[Csaba Osztrogonác]

Could you guys confirm if our fix is correct or not?

[Julien Chaffraix]

(In reply to comment #13)
> Could you guys confirm if our fix is correct or not?

Thanks, that's a good fix. The important lines are the one setting the font on the CanvasGraphicsContext2D.