Bug 66291 - Crash because CSSPrimitiveValue::computeLengthDouble assumes fontMetrics are available
Summary: Crash because CSSPrimitiveValue::computeLengthDouble assumes fontMetrics are ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Julien Chaffraix
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-08-16 04:25 PDT by Berend-Jan Wever
Modified: 2011-09-28 07:04 PDT (History)
10 users (show)

See Also:


Attachments
Repro (110 bytes, text/html)
2011-08-16 04:25 PDT, Berend-Jan Wever
no flags Details
Proposed work-around: Add a manual update. (4.09 KB, patch)
2011-09-26 11:34 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed work-around 2: Added a 'why' comment that should answer Darin's request. (4.56 KB, patch)
2011-09-26 16:14 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2011-08-16 04:25:49 PDT
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
                ...
Comment 1 Berend-Jan Wever 2011-08-16 04:27:09 PDT
Chromium https://code.google.com/p/chromium/issues/detail?id=93035
Comment 2 Simon Fraser (smfr) 2011-08-23 12:18:28 PDT
<rdar://problem/9970343>
Comment 3 Julien Chaffraix 2011-09-26 11:34:42 PDT
Created attachment 108695 [details]
Proposed work-around: Add a manual update.
Comment 4 Darin Adler 2011-09-26 14:34:05 PDT
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.
Comment 5 Julien Chaffraix 2011-09-26 16:14:54 PDT
Created attachment 108747 [details]
Proposed work-around 2: Added a 'why' comment that should answer Darin's request.
Comment 6 WebKit Review Bot 2011-09-27 09:55:06 PDT
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>
Comment 7 WebKit Review Bot 2011-09-27 09:55:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2011-09-27 09:57:47 PDT
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.
Comment 9 Julien Chaffraix 2011-09-27 10:59:55 PDT
(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.
Comment 10 Csaba Osztrogonác 2011-09-28 00:57:34 PDT
(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.
Comment 11 Csaba Osztrogonác 2011-09-28 00:59:41 PDT
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);
Comment 12 Gabor Rapcsanyi 2011-09-28 01:23:23 PDT
Patch landed with this change in r96204.
http://trac.webkit.org/changeset/96204
Comment 13 Csaba Osztrogonác 2011-09-28 03:35:42 PDT
Could you guys confirm if our fix is correct or not?
Comment 14 Julien Chaffraix 2011-09-28 07:04:10 PDT
(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.