[Webkit-unassigned] [Bug 216407] Safely handle overly-long CSS variable values

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Sep 12 14:35:45 PDT 2020


https://bugs.webkit.org/show_bug.cgi?id=216407

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com

--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 408573
  --> https://bugs.webkit.org/attachment.cgi?id=408573
Patch

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

Thanks for contributing this. I am not sure it’s OK to have a test that needs a multi-second timeout. We should figure out how to handle this without it becoming a drag on the project.

> Source/WebCore/css/CSSVariableReferenceValue.h:55
> +    static const size_t maxSubstitutionTokens = 65536;

Please use constexpr for this kind of thing in new code.

> Tools/ChangeLog:9
> +        Add ability to parse `timeoutMs` from test headers for tests who need a timeout
> +        value different from the command-wide timeout.

Big picture, I am not so sure it’s good to have tests that take a super-long time to run, so not sure we should be adding this feature. Also, I’d think we’d need this feature in DumpRenderTree too, not just WebKitTestRunner, so we can run this test for WebKitLegacy.

> Tools/WebKitTestRunner/TestController.cpp:1407
> +    double val = WTF::String(value.c_str()).toDouble(&successfulConversion);

While this is OK, there’s no need to convert a std::string to a WTF::String just to use WTF’s parsing. The parseDouble function in <wtf/dtoa.h> works directly on C strings without having to first convert them to a WTF::String. Probably the best one to use is the overload that takes a StringView.

WebKit coding style uses words, so we would not use "val" here. We’d use a single word or two words, maybe "doubleValue" or "parsedValue".

> Tools/WebKitTestRunner/TestOptions.h:109
> +    Seconds timeout { Seconds() };

There’s no need to write { Seconds() } here: unlike scalar types, Seconds objects already get initialized that way without explicitly stating the value.

> LayoutTests/ChangeLog:9
> +        * fast/css/variables/invalidate-overly-long-variable-values.html: Added.
> +        * fast/css/variables/invalidate-overly-long-variable-values-expected.html: Added.

Should this be a WPT test instead of a WebKit-only test?

> LayoutTests/fast/css/variables/invalidate-overly-long-variable-values.html:1
> +<!-- webkit-test-runner [ timeoutMs=5000 ] -->

It’s not good for the project to add a test that takes so long to run; we’re constantly running tests on lots of different computers and a single multi-second test is a huge cost. We’ve already skipped some of the WPT tests since they take so long. Is there some other way we can deal with this?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200912/1c5e869c/attachment-0001.htm>


More information about the webkit-unassigned mailing list