Bug 88866 – ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size.

• Component: CSS
• Status: RESOLVED FIXED

[Michael Brüning]

Created attachment 147071 [details]
Test case html file that triggers the assert.

Noticed with a Qt 5 WK2 trunk debug build (r119588).

To trigger, load the attached test.html file in a debug build (I used MiniBrowser).

ASSERTS with the following stack trace: 
/home/mibrunin/swork/webkit/Source/WebCore/css/StylePropertySet.cpp(285) : WTF::String WebCore::StylePropertySet::fontValue() const
1   0x7f97978f055f [...]/libQtWebKit.so.5(_ZNK7WebCore16StylePropertySet9fontValueEv+0x305) [0x7f97978f055f]
2   0x7f97978efb21 [...]/libQtWebKit.so.5(_ZNK7WebCore16StylePropertySet16getPropertyValueENS_13CSSPropertyIDE+0x3e7) [0x7f97978efb21]
3   0x7f979789eeb1 [...]/libQtWebKit.so.5(_ZN7WebCore30PropertySetCSSStyleDeclaration24getPropertyValueInternalENS_13CSSPropertyIDE+0x2d) [0x7f979789eeb1]
4   0x7f97976d822b [...]/libQtWebKit.so.5(+0x23d322b) [0x7f97976d822b]
5   0x7f97976d8512 [...]/libQtWebKit.so.5(+0x23d3512) [0x7f97976d8512]
6   0x7f97976d85ac [...]/libQtWebKit.so.5(+0x23d35ac) [0x7f97976d85ac]
7   0x7f9797579800 [...]/libQtWebKit.so.5(_ZNK3JSC12PropertySlot8getValueEPNS_9ExecStateENS_12PropertyNameE+0x76) [0x7f9797579800]
8   0x7f9797735927 [...]/libQtWebKit.so.5(_ZNK3JSC7JSValue3getEPNS_9ExecStateENS_12PropertyNameERNS_12PropertySlotE+0xd3) [0x7f9797735927]
9   0x7f9797735851 [...]/libQtWebKit.so.5(_ZNK3JSC7JSValue3getEPNS_9ExecStateENS_12PropertyNameE+0x4f) [0x7f9797735851]
10  0x7f9798c28646 [...]/libQtWebKit.so.5(cti_op_get_by_val+0x435) [0x7f9798c28646]
11  0x7f9798c22a0c [...]/libQtWebKit.so.5(+0x391da0c) [0x7f9798c22a0c]

I debugged this a bit and found out that after setting the font size through JS, the font size property is set, but not all the other longhand font properties that are expected to be present at least implicitly (e.g. CSSPropertyFontStyle) are not set and therefore, the assert is reached.

Alexander, if you can provide me with some guidance on where to look into fixing this, I'll be happy to do so.

[Alexander Pavlov (apavlov)]

OK, some pre-history. Originally, the "font" property was a first-class CSS property, not a shorthand (which broke a few things across WebKit, including the Web Inspector). I fixed this to the extent of relying on the valid longhand property values found in the style. Originally the asserting code was seemingly found in the CSSComputedStyleDeclaration class but was since then moved into StylePropertySet by [e-mail] (:anttik). He knows best how things work in the CSS land, since he's its primary maintainer, and is in the process of refactoring the entire thing.

Antti, can you advise on possible behavior impact introduced by moving the asserting code (String StylePropertySet::fontValue() const) from CSSStyleDeclaration into StylePropertySet? It relied on "inital" font longhand values being always present in the computed style, but no longer seems to be the case...

Alexis, I've added you just in case you may know something about the underlying matter, since you changed the CSSParser font parsing code around 2 months ago (http://trac.webkit.org/changeset/114895).

[Jonathan Liu]

I can also reproduce this running the Qt 5 fancybrowser example in debug build and clicking Tools -> Remove GIF images. Affects Qt 5.0.0 release and Qt 5 stable branch.

[Antti Koivisto]

I have just been moving code. I don't think I have done any intentional behavior changes here. Alexis knows this stuff.

[Alexis Menard (darktears)]

(In reply to comment #3)
> I have just been moving code. I don't think I have done any intentional behavior changes here. Alexis knows this stuff.

From what I can see with a quick debug, when setting directly the font-size the rest of the other font properties are not set to a value which is fine as all the longhands (part of a shorthand) are behaving the same (for example, setting background-position only will not set other background properties to "initial"). Now the problem lies in the code reconstructing the font shorthand which assume that the font related properties were set using the font shorthand (which then will set the unspecified longhands to "initial" like any other shorthand) but as shown by the test case it's not always the case. I don't think Antti has anything to do with the problem, the bug has been there for a while I believe. I'll give a shot on a fix.

[Alexis Menard (darktears)]

Created attachment 181528 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #5)
> Created an attachment (id=181528) [details]
> Patch

Let's see EWS.

Unless I miss something I think this ASSERT is not accurate. The history shows that the original writer is Alexander, maybe he can explain a bit more the part where an "invalid shorthand is constructed".

[Alexis Menard (darktears)]

Created attachment 181540 [details]
Attempt to fix the test in cross-platform way

[Alexander Pavlov (apavlov)]

Comment on attachment 181540 [details]
Attempt to fix the test in cross-platform way

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

> Source/WebCore/ChangeLog:15
> +        other shorthands in of WebKit. When reconstructing the shorthand other

stray "of"

> Source/WebCore/ChangeLog:18
> +        shortest shorthand possible) or if the value is set or not (if set then

..but not shorter than it is allowed to be :) (see below)

> Source/WebCore/ChangeLog:22
> +        construct a valid value at it takes care of adding ' ' or '/' when

at -> as

> Source/WebCore/css/StylePropertySet.cpp:262
> +    return;

You definitely can drop it :)

> LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt:6
> +PASS style.font is '20px'

According to http://www.w3.org/TR/CSS2/fonts.html#font-shorthand, "20px" is not a valid value for "font" (if a non-keyword value is used, the <'font-size'> and <'font-family'> components are mandatory.) Thus,
style.font should be "20px foobar", not bare "20px".

> LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt:37
> +PASS style.font is 'bold 40px'

This doesn't seem a valid value, either

> LayoutTests/fast/css/font-shorthand-from-longhands.html:10
> +    font-family: "foobar";

It would be nice to test the case when there's no explicit "font-family" value for the checked element.

> LayoutTests/fast/css/font-shorthand-from-longhands.html:17
> +description("Test the parsing and the computed style values of the transitions properties.")

doesn't look like "transitions properties" :)

> LayoutTests/fast/css/font-shorthand-from-longhands.html:24
> +e = document.getElementById('test');

Use "var" declarations for all variables - this is the common "right" way

> LayoutTests/fast/css/font-shorthand-from-longhands.html:28
> +// This function check the return value of style.font and verify WebKit can parse it.

checks
verifies

> LayoutTests/fast/css/font-shorthand-from-longhands.html:33
> +    return (e.style.getPropertyValue('font') == before);

My impression is that all JS code uses '===' whenever an identity check is implied (at least the Web Inspector)

[Alexis Menard (darktears)]

Created attachment 181933 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #8)
> (From update of attachment 181540 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181540&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        other shorthands in of WebKit. When reconstructing the shorthand other
> 
> stray "of"
> 
> > Source/WebCore/ChangeLog:18
> > +        shortest shorthand possible) or if the value is set or not (if set then
> 
> ..but not shorter than it is allowed to be :) (see below)
> 
> > Source/WebCore/ChangeLog:22
> > +        construct a valid value at it takes care of adding ' ' or '/' when
> 
> at -> as
> 
> > Source/WebCore/css/StylePropertySet.cpp:262
> > +    return;
> 
> You definitely can drop it :)
> 
> > LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt:6
> > +PASS style.font is '20px'
> 
> According to http://www.w3.org/TR/CSS2/fonts.html#font-shorthand, "20px" is not a valid value for "font" (if a non-keyword value is used, the <'font-size'> and <'font-family'> components are mandatory.) Thus,
> style.font should be "20px foobar", not bare "20px".
> 

Oops sorry for overlooking that part. So in fact what I did in the new patch is that if the font-family is not specified on the style then we can't construct the shorthand. From StylePropertySet I can't dig "foobar" as the font-family value because it's later on that it gets resolved (and you get it in the computed style). So it's better to return nothing than a broken value as I can't build a valid font shorthand.

> > LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt:37
> > +PASS style.font is 'bold 40px'
> 
> This doesn't seem a valid value, either
> 
> > LayoutTests/fast/css/font-shorthand-from-longhands.html:10
> > +    font-family: "foobar";
> 
> It would be nice to test the case when there's no explicit "font-family" value for the checked element.

Well I tried to do that but in fact the resolved font-family is different from port to port, platform to platform so there is no easy way to test it without landing specific port expected output.

> 
> > LayoutTests/fast/css/font-shorthand-from-longhands.html:17
> > +description("Test the parsing and the computed style values of the transitions properties.")
> 
> doesn't look like "transitions properties" :)

:(

> 
> > LayoutTests/fast/css/font-shorthand-from-longhands.html:24
> > +e = document.getElementById('test');
> 
> Use "var" declarations for all variables - this is the common "right" way
> 
> > LayoutTests/fast/css/font-shorthand-from-longhands.html:28
> > +// This function check the return value of style.font and verify WebKit can parse it.
> 
> checks
> verifies
> 
> > LayoutTests/fast/css/font-shorthand-from-longhands.html:33
> > +    return (e.style.getPropertyValue('font') == before);
> 
> My impression is that all JS code uses '===' whenever an identity check is implied (at least the Web Inspector)

[Alexander Pavlov (apavlov)]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 181540 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181540&action=review
> > 
> > > LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt:6
> > > +PASS style.font is '20px'
> > 
> > According to http://www.w3.org/TR/CSS2/fonts.html#font-shorthand, "20px" is not a valid value for "font" (if a non-keyword value is used, the <'font-size'> and <'font-family'> components are mandatory.) Thus,
> > style.font should be "20px foobar", not bare "20px".
> > 
> 
> Oops sorry for overlooking that part. So in fact what I did in the new patch is that if the font-family is not specified on the style then we can't construct the shorthand. From StylePropertySet I can't dig "foobar" as the font-family value because it's later on that it gets resolved (and you get it in the computed style). So it's better to return nothing than a broken value as I can't build a valid font shorthand.

Right, that's why I used to return empty values in some cases.

> > > LayoutTests/fast/css/font-shorthand-from-longhands.html:10
> > > +    font-family: "foobar";
> > 
> > It would be nice to test the case when there's no explicit "font-family" value for the checked element.
> 
> Well I tried to do that but in fact the resolved font-family is different from port to port, platform to platform so there is no easy way to test it without landing specific port expected output.

What I meant was having an explicit font-family inherited from an ancestor element (e.g. BODY). Will that work or is it the same as the case you described above?

[Alexander Pavlov (apavlov)]

Comment on attachment 181933 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 181933 [details]
Patch

Clearing flags on attachment: 181933

Committed r139313: <http://trac.webkit.org/changeset/139313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.