Bug 133796 – ASSERT_NOT_REACHED() in WebCore::fontWeightIsBold

• Component: HTML Editing
• Status: NEW

[Tibor Mészáros]

ASSERT_NOT_REACHED() in WebCore::fontWeightIsBold in WebCore::fontWeightIsBold(WebCore::CSSValue*) (Source/WebCore/editing/EditingStyle.cpp)
GIT rev.: 17538480d681894206b58ad8a613fa83bc726cbe
Debug EWebLauncher

[Tibor Mészáros]

Created attachment 232953 [details]
Patch

The patch was tested with the two new test, and it works.

[Build Bot]

Comment on attachment 232953 [details]
Patch

Attachment 232953 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5003576082432000

New failing tests:
editing/undo/remove-css-property-and-remove-style.html
css1/font_properties/font_weight_bolder.html
css1/font_properties/font_weight_lighter.html
editing/style/push-down-inline-styles.html

[Build Bot]

Created attachment 232955 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232953 [details]
Patch

Attachment 232953 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6266763410931712

New failing tests:
editing/undo/remove-css-property-and-remove-style.html
css1/font_properties/font_weight_bolder.html
css1/font_properties/font_weight_lighter.html
editing/style/push-down-inline-styles.html

[Build Bot]

Created attachment 232960 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[zalan]

Comment on attachment 232953 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:1516
>  static bool fontWeightIsBold(CSSValue* fontWeight)

This should take CSSValue&.

> Source/WebCore/editing/EditingStyle.cpp:1551
> +static bool fontWeightNeedsResolving(CSSValue* fontWeight)

This should take CSSValue& too.

> Source/WebCore/editing/EditingStyle.cpp:1574
> +    if (RefPtr<CSSValue> baseFontWeight = extractPropertyValue(baseStyle, CSSPropertyFontWeight)) {
> +        if (RefPtr<CSSValue> fontWeight = mutableStyle->getPropertyCSSValue(CSSPropertyFontWeight)) {
> +            if (!fontWeightNeedsResolving(fontWeight.get()) && (fontWeightIsBold(fontWeight.get()) == fontWeightIsBold(baseFontWeight.get())))

I can't comment on the correctness of this, but surely you could shave off at least one level of nesting here.

[Ryosuke Niwa]

Comment on attachment 232953 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:1556
> +    return !(value == CSSValueLighter || value == CSSValueBolder);

I'd prefer putting CSSValueLighter and CSSValueBolder into fontWeightIsBold instead for now.
It would be incorrect in the case where lighter still results in bolded text or bolder resulting in "light" text
but that's probably better than not handling them correctly at all.

[Build Bot]

Comment on attachment 232953 [details]
Patch

Attachment 232953 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6309241711230976

New failing tests:
editing/undo/remove-css-property-and-remove-style.html
css1/font_properties/font_weight_bolder.html
css1/font_properties/font_weight_lighter.html
editing/style/push-down-inline-styles.html

[Build Bot]

Created attachment 233035 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Tibor Mészáros]

Created attachment 234912 [details]
Patch v2

    Source/WebCore/editing/EditingStyle.cpp:1516
    static bool fontWeightIsBold(CSSValue* fontWeight)

Is now CSSValue&.

    Source/WebCore/editing/EditingStyle.cpp:1551
    +static bool fontWeightNeedsResolving(CSSValue* fontWeight)

Is now CSSValue&.

    Source/WebCore/editing/EditingStyle.cpp:1574
    + if (RefPtr<CSSValue> baseFontWeight = extractPropertyValue(baseStyle, CSSPropertyFontWeight)) {
    + if (RefPtr<CSSValue> fontWeight = mutableStyle->getPropertyCSSValue(CSSPropertyFontWeight)) {
    + if (!fontWeightNeedsResolving(fontWeight.get()) && (fontWeightIsBold(fontWeight.get()) fontWeightIsBold(baseFontWeight.get())))

Merged the upper three "If" into one

    Source/WebCore/editing/EditingStyle.cpp:1556
    + return !(value CSSValueLighter || value == CSSValueBolder);

I had put CSSValueLighter and CSSValueBolder into fontWeightIsBold. It will be correct in the case where lighter still results in bolded text or bolder resulting in "light" text.

[Build Bot]

Comment on attachment 234912 [details]
Patch v2

Attachment 234912 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5523394724364288

New failing tests:
css1/font_properties/font_weight_bolder.html

[Build Bot]

Created attachment 234914 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234912 [details]
Patch v2

Attachment 234912 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6531884183977984

New failing tests:
css1/font_properties/font_weight_bolder.html

[Build Bot]

Created attachment 234916 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234912 [details]
Patch v2

Attachment 234912 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6507261136470016

New failing tests:
css1/font_properties/font_weight_bolder.html

[Build Bot]

Created attachment 234917 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Tibor Mészáros]

Created attachment 234923 [details]
Patch v3

The test expectation has been fixed.

[Myles C. Maxfield]

Comment on attachment 234923 [details]
Patch v3

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

> Source/WebCore/editing/EditingStyle.cpp:1557
> +    ASSERT(baseStyle);

If you're going to ASSERT this, why not make it a reference?

[Myles C. Maxfield]

Comment on attachment 234923 [details]
Patch v3

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

> Source/WebCore/editing/EditingStyle.cpp:1533
> +        case CSSValueLighter:

This doesn't seem to conceptually make sense... "Lighter" means bold?

[Darin Adler]

Comment on attachment 234923 [details]
Patch v3

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

Patch is well meaning but wrong.

The real problem is far away from this code, and quieting the assertion here is not a real fix.

> LayoutTests/ChangeLog:12
> +        * css1/font_properties/font_weight_bolder.html: Added.
> +        * css1/font_properties/font_weight_lighter.html: Added.

Test cases should be reference tests.

I think it’s really strange to have these tests in the css1 directory. These are editing tests and belong in the editing directory.

In a CSS test directory (not this css1 one) we could have tests to make sure that bolder and lighter are rejected as keyword values during the CSS parsing process. Only a few special paths in editing code should parse in a mode that allows these values.

> LayoutTests/css1/font_properties/font_weight_bolder.html:4
> +        font-weight:bolder;

We should not allow this syntax in a style sheet. The “bolder” and “lighter” values are a special CSS hack for editing, and it’s an error to allow a style rule to actually contain that value. The real fix would be to find a way to address that issue.

> LayoutTests/css1/font_properties/font_weight_bolder.html:12
> +               <animatemotion onload='document.execCommand("selectall")'></animatemotion>

Why on earth does this test case include SVG? Is that needed to reproduce the bug or just a strange choice we are making?

>> Source/WebCore/editing/EditingStyle.cpp:1533
>> +        case CSSValueLighter:
> 
> This doesn't seem to conceptually make sense... "Lighter" means bold?

Myles is absolutely right. It’s wrong to say that bolder and lighter are bold weights. The current behavior of this function where it asserts in that case is correct.

To correctly deal with the values “bolder” and “lighter”, they need to never be considered the same as existing style; a correct fix would always skip the removeProperty call when the mutable style’s property was bolder or lighter, and continue to assert if the other style contains bolder or lighter. See below.

>> Source/WebCore/editing/EditingStyle.cpp:1557
>> +    ASSERT(baseStyle);
> 
> If you're going to ASSERT this, why not make it a reference?

Good idea for future refactoring. Both arguments to this function template should be references.

> Source/WebCore/editing/EditingStyle.cpp:1572
> +    if ((baseFontWeight = extractPropertyValue(baseStyle, CSSPropertyFontWeight))
> +        && (fontWeight = mutableStyle->getPropertyCSSValue(CSSPropertyFontWeight))
> +        && (fontWeightIsBold(*fontWeight) == fontWeightIsBold(*baseFontWeight))) {
> +            mutableStyle->removeProperty(CSSPropertyFontWeight);
> +    }

If fontWeight is bolder or lighter, then we should skip removeProperty regardless of the value of baseFontWeight. That needs to be done here; I don’t see a way to correctly encapsulate it in the fontWeightIsBold helper function.

If the font weight is the minimum the we could remove “lighter”, and if the font weight is the maximum we could remove “bolder”. That would be nice to get right, but it’s an edge case that I don’t think is critical.

I’d like to see test cases covering this behavior, including the edge cases.

If baseFontWeight is bolder or lighter, then the bug has already happened. Such styles should never be allowed in CSS styles on actual elements. Those values are a hack to be used inside the editing machinery and in API not exposed to JavaScript. If JavaScript can ever see those values, then we have a bug.

The refactoring to use references instead of pointers we are doing here messes up the logic here and makes this function harder to read. We should think about how to handle that, and why boldness is different from the other properties below in this respect. I suspect that refactoring is not really part of this bug fix and we should consider doing it separately.

[Darin Adler]

There are really two different bugs here.

One bug is that the CSS machinery is allowing these invalid values (bolder and lighter) to become property values on actual elements and in actual stylesheets. That should be fixed by rejecting these values the same way we’d reject any other illegal keywords.

A second possible bug is that the editing code may not handle lighter and bolder correctly in the process of doing editing operations.

Both would result in hitting this assertion.

The existing patch is not the correct fix for either of these problems.

[Ryosuke Niwa]

Comment on attachment 234923 [details]
Patch v3

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

>> LayoutTests/ChangeLog:12
>> +        * css1/font_properties/font_weight_lighter.html: Added.
> 
> Test cases should be reference tests.
> 
> I think it’s really strange to have these tests in the css1 directory. These are editing tests and belong in the editing directory.
> 
> In a CSS test directory (not this css1 one) we could have tests to make sure that bolder and lighter are rejected as keyword values during the CSS parsing process. Only a few special paths in editing code should parse in a mode that allows these values.

bolder and lighter are real CSS2.1 values:
http://www.w3.org/TR/CSS21/fonts.html#font-boldness

>> Source/WebCore/editing/EditingStyle.cpp:1572
>> +    }
> 
> If fontWeight is bolder or lighter, then we should skip removeProperty regardless of the value of baseFontWeight. That needs to be done here; I don’t see a way to correctly encapsulate it in the fontWeightIsBold helper function.
> 
> If the font weight is the minimum the we could remove “lighter”, and if the font weight is the maximum we could remove “bolder”. That would be nice to get right, but it’s an edge case that I don’t think is critical.
> 
> I’d like to see test cases covering this behavior, including the edge cases.
> 
> If baseFontWeight is bolder or lighter, then the bug has already happened. Such styles should never be allowed in CSS styles on actual elements. Those values are a hack to be used inside the editing machinery and in API not exposed to JavaScript. If JavaScript can ever see those values, then we have a bug.
> 
> The refactoring to use references instead of pointers we are doing here messes up the logic here and makes this function harder to read. We should think about how to handle that, and why boldness is different from the other properties below in this respect. I suspect that refactoring is not really part of this bug fix and we should consider doing it separately.

lighter and bolder are valid CSS values.  They bolden & lighten text weight relative to the inherited font weight.
I do agree that one correct approach is avoid removing font-weight property when those values are used.

Alternatively, we can also try to resolve the computed font weight by pluming values from render style but that's going to be a lot of work.

[Darin Adler]

OK, sorry I got that wrong about bolder and lighter being a special editing hack.

The rest of my comments are still relevant. I think we should be able to fix this by treating bolder and lighter as “never to be removed”. Treating them as “bold” is not what we want. We need test cases that cover this behavior, not just test cases that check if there is a crash or not.

[Brent Fulgham]

<rdar://problem/37505183>

[Brent Fulgham]

This is actually:

<rdar://problem/27711671>