Bug 144739 - Fix sizes crash and add invalid value tests.
Summary: Fix sizes crash and add invalid value tests.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-07 04:25 PDT by Yoav Weiss
Modified: 2015-05-11 01:50 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.44 KB, patch)
2015-05-07 04:43 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (7.54 KB, patch)
2015-05-07 05:20 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (6.98 KB, patch)
2015-05-07 13:27 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2015-05-07 04:25:09 PDT
Fix sizes crash and add invalid value tests.
Comment 1 Yoav Weiss 2015-05-07 04:43:14 PDT
Created attachment 252585 [details]
Patch
Comment 2 Yoav Weiss 2015-05-07 05:20:41 PDT
Created attachment 252586 [details]
Patch
Comment 3 Darin Adler 2015-05-07 09:20:33 PDT
Comment on attachment 252586 [details]
Patch

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

I’m OK with this patch, but I think it’s unnecessarily awkward.

> Source/WebCore/css/SourceSizeList.cpp:59
> -static unsigned computeLength(CSSValue* value, RenderStyle& style, RenderView* view)
> +static bool computeLength(CSSValue* value, RenderStyle& style, RenderView* view, unsigned& sourceSizeLength)

Seems a little inelegant to do the checking in the same function that computes lengths, given that we do this work in a loop and we only need the check once. I suggest putting the checking into a separate function. Maybe we should even have parseSizesAttribute use two separate loops for CSSPrimitiveValue and CSSCalcValue rather than constantly branching inside the loop.

> Source/WebCore/css/SourceSizeList.cpp:86
> +            if (!computeLength(sourceSize.length.get(), style, view, sourceSizeLength))
> +                break;

I think it’s strange to check if the value is a length inside this loop instead of checking it outside the loop. The break here is illogical until you think it through.

> Source/WebCore/css/SourceSizeList.cpp:93
> -    return computeLength(CSSPrimitiveValue::create(100, CSSPrimitiveValue::CSS_VW).ptr(), style, view);
> +    if (computeLength(CSSPrimitiveValue::create(100, CSSPrimitiveValue::CSS_VW).ptr(), style, view, sourceSizeLength))
> +        return sourceSizeLength;
> +    ASSERT_NOT_REACHED();
> +    return 0;

Combining the checking with the computation makes this ugly too. That ASSERT_NOT_REACHED is self-inflicted.

Also seems a shame that we have to allocation memory just to reuse code. Could we refactor this so we don’t have to heap-allocated a reference counted object and then delete it just to do the length computation?
Comment 4 Yoav Weiss 2015-05-07 13:27:40 PDT
Created attachment 252614 [details]
Patch
Comment 5 Yoav Weiss 2015-05-07 13:37:44 PDT
Comment on attachment 252614 [details]
Patch

I've refactored the code so that it would be more straight forward.

The length check has to happen only after we know we have a CSSPrimitiveLength on our hands, to it has to happen in the loop, but only happens on the first matching expression (in case it is in fact a CSSPrimitiveValue). We also need to maintain a single loop for both lengths and calcs, since the algorithm is a first-match.

Regarding the creation of CSSPrimitiveValue just to calculate length, that is a shame. I'll try to refactor out the computeLength logic of CSSPrimitiveValue to be static, so that it can be used here without creating a new object. I'll do that as a separate issue though, since I suspect it may be a large patch.
Comment 6 WebKit Commit Bot 2015-05-07 14:23:57 PDT
Comment on attachment 252614 [details]
Patch

Clearing flags on attachment: 252614

Committed r183948: <http://trac.webkit.org/changeset/183948>
Comment 7 WebKit Commit Bot 2015-05-07 14:24:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2015-05-10 15:32:46 PDT
Comment on attachment 252614 [details]
Patch

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

> Source/WebCore/css/SourceSizeList.cpp:61
> +    return CSSPrimitiveValue::create(100, CSSPrimitiveValue::CSS_VW)->computeLength<unsigned>(CSSToLengthConversionData(&style, &style, view));

This needs a FIXME. We want to reimplement this so it does it without allocating a CSSPrimitiveValue object in the future.
Comment 9 Yoav Weiss 2015-05-11 01:50:46 PDT
Added a fixme at https://bugs.webkit.org/show_bug.cgi?id=144861