Bug 166637 - [css-grid] Fix crash clamping grid lines
Summary: [css-grid] Fix crash clamping grid lines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords: BlinkMergeCandidate, InRadar
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2017-01-02 02:49 PST by Manuel Rego Casasnovas
Modified: 2017-06-05 13:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.56 KB, patch)
2017-01-02 02:49 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (4.60 KB, patch)
2017-01-02 02:50 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (13.05 KB, patch)
2017-01-04 05:06 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (14.89 KB, patch)
2017-01-04 13:58 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2017-01-02 02:49:34 PST
[css-grid] Fix crash clamping grid lines
Comment 1 Manuel Rego Casasnovas 2017-01-02 02:49:54 PST
Created attachment 297887 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2017-01-02 02:50:46 PST
Created attachment 297888 [details]
Patch
Comment 3 Manuel Rego Casasnovas 2017-01-02 02:52:45 PST
This has been ported from Blink:
https://codereview.chromium.org/2546993002
Comment 4 Darin Adler 2017-01-02 18:50:02 PST
Comment on attachment 297888 [details]
Patch

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

Looks like good work, but I am not saying review+ or review- because I have so many doubts.

Link error saying this symbol can’t be found on the Apple platforms:

   "WebCore::CSSPrimitiveValue::doubleValue() const"

The fix is to add WEBCORE_EXPORT to the doubleValue() function in CSSPrimitiveValue.h or change the code so it doesn’t call this.

> Source/WebCore/ChangeLog:8
> +        Avoid issues with very big values for the grid lines clamping them during parsing time.

Is parsing time the right place for these kinds of limits? I’d like to hear what Hyatt has to say about that, unless you are absolutely sure.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2988
> +    if (numericValue)
> +        numericValue = CSSPrimitiveValue::create(clampTo(numericValue->intValue(), -kGridMaxTracks, kGridMaxTracks), CSSPrimitiveValue::UnitType::CSS_NUMBER);

It seems unfortunate to create a new CSSPrimitiveValue object for this rather than clamping before creating the CSSPrimitiveValue object; feels unnecessarily inefficient. If clamping during parsing is going to be a normal pattern, can we make a consumeInteger function that does the clamping perhaps?

> Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:48
> +static int getGridPositionInteger(CSSValueList& valueList)

Normally we would not use "get" in a function name in WebKit unless the name came from the web platform.

> Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:51
> +    const auto* primitiveValue = downcast<CSSPrimitiveValue>(valueList.item(0));

Given this is a test the assertions aren’t great. We’d rather have the test system assertions instead of WebKit’s own internal assertions, I think.

I would use auto* here; no need to explicitly write const. But also, why not use a reference instead a pointer?

> Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:53
> +    return primitiveValue->intValue();

Using intValue isn’t ideal for the test, because intValue itself does some clamping when it fetches the value. I suggest we use doubleValue() instead. Although perhaps the actual grid code uses intValue? (This seems a bit messy to me that we have are adding clamping in the parser and already have clamping in the intValue function.)

Given that intValue itself clamps, I think the clamping might belong in the grid, not in the style parsing. Or in the style computation. It’s a bit peculiar to have clamping that is visible through the CSS object model.

> Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:96
> +TEST(CSSPropertyParserTest, GridPositionLimits)

Given that we implemented the clamping in the parser, this could be tested easily with code in JavaScript using the CSS object model. So it seems unnecessary to make kind of unit-test-style test for it.

But I think maybe not in the parser.

> Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:122
> +        EXPECT_EQ(getGridPositionInteger(*downcast<CSSValueList>(value.get())), testCase.output);

Normally we would write downcast<CSSValueList>(*value) instead of the thing written here.
Comment 5 Manuel Rego Casasnovas 2017-01-04 05:06:58 PST
Created attachment 298006 [details]
Patch
Comment 6 Manuel Rego Casasnovas 2017-01-04 05:12:48 PST
Thanks for the review!

(In reply to comment #4)
> > Source/WebCore/ChangeLog:8
> > +        Avoid issues with very big values for the grid lines clamping them during parsing time.
> 
> Is parsing time the right place for these kinds of limits? I’d like to hear
> what Hyatt has to say about that, unless you are absolutely sure.

We are already clamping on parsing time for "repaet()" syntax.
Hoever I've tried a different approach for this patch, clamping on GridPosition, which seems to be working fine too.

I'm creating a new unit test to check the expected behavior.
The reason to do a unit test instead of a regular layout test is to avoid creating a very huge grid to run the tests.
That's why we originally created CSSParser.cpp test in bug #136217.

PTAL to the new version.
Comment 7 Manuel Rego Casasnovas 2017-01-04 05:13:53 PST
BTW, when I reported the bug I forgot to mention how to reproduce it.
It's enough with a very simple grid like:
  <!DOCTYPE html>
  <div style="display: grid;">
    <div style="grid-column-start: 500000000000000000000000 foo;">test</div>
  </div>
Comment 8 Darin Adler 2017-01-04 10:04:04 PST
(In reply to comment #6)
> We are already clamping on parsing time for "repeat()" syntax.

I believe this is a web-platform-observable choice. On the face of it, it seems always bad to me to change values as part of parsing them rather than in code that is interpreting the values. It reminds me of the kind of thing we did early in the WebKit project when we didn't understand the web platform as well as we do now.

Thanks for the new patch, I think it’s better.
Comment 9 Darin Adler 2017-01-04 10:04:23 PST
(In reply to comment #7)
> BTW, when I reported the bug I forgot to mention how to reproduce it.
> It's enough with a very simple grid like:
>   <!DOCTYPE html>
>   <div style="display: grid;">
>     <div style="grid-column-start: 500000000000000000000000 foo;">test</div>
>   </div>

Besides the unit test, I’d like to see a test like that one too.
Comment 10 Darin Adler 2017-01-04 10:05:03 PST
Comment on attachment 298006 [details]
Patch

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

> Source/WebCore/rendering/style/GridPosition.h:136
> +    void setIntegerPosition(int integerPosition)
> +    {
> +        m_integerPosition = clampTo(integerPosition, -kGridMaxTracks, kGridMaxTracks);
> +    }

Normally we would put private function members before the data members, and we would put a blank line before "private:".
Comment 11 Manuel Rego Casasnovas 2017-01-04 13:58:36 PST
Created attachment 298042 [details]
Patch for landing
Comment 12 Manuel Rego Casasnovas 2017-01-04 14:00:34 PST
Comment on attachment 298006 [details]
Patch

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

Uploaded patch for landing, adding layout test as requested.
Thanks for the review.

>> Source/WebCore/rendering/style/GridPosition.h:136
>> +    }
> 
> Normally we would put private function members before the data members, and we would put a blank line before "private:".

Sure. Done.
Comment 13 WebKit Commit Bot 2017-01-04 23:04:07 PST
Comment on attachment 298042 [details]
Patch for landing

Clearing flags on attachment: 298042

Committed r210320: <http://trac.webkit.org/changeset/210320>
Comment 14 WebKit Commit Bot 2017-01-04 23:04:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2017-06-05 13:47:20 PDT
<rdar://problem/32571846>