-
Notifications
You must be signed in to change notification settings - Fork 656
-
Notifications
You must be signed in to change notification settings - Fork 656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[css-variables] Token sequence size limit #3733
Comments
A limit of some sort makes sense (and should be documented) but it should be set to be as large as any legal value you support for a CSS property — including data URIs for images. |
Please don't enforce a specific maximum, see #3462 (comment)
|
Yup, I'm fine with specifying that limits are allowed (and encouraged!). The best behavior is likely just "drop all tokens after the limit". |
Wait, so you're suggesting they should pass through part of the value? That seems very bad and inconsistent with normal CSS error handling. I'd say that if you can't handle the entire value as set by the author, you treat it as an invalid declaration or invalid result of a |
Yeah, you're right. |
@emilio In the WPT for this, Firefox still times out. Is your mitigation recent or something, and not yet picked up by the WPT bots? |
Ah, looks like the FF fix was backed out for linting failures. ^_^ Welp, it's a nice failing test for everyone right now, then. |
Which version of Firefox are you using? It landed in 65 (it surely relanded, otherwise the test wouldn't be upstream). https://cras.sh/crash.html doesn't crash on Firefox, for that matter, nor does http://w3c-test.org/css/css-variables/variable-exponential-blowup.html. Or am I missing something? I'd be interested if so :). |
I'm not using anything, but the WPT TaskCluster instance, apparently using FF Nightly, is timing out on my test: (I didn't realize you'd committed a similar test already.) |
The only difference I can see is that I wrote mine out manually, vs yours in JS (shouldn't be a difference) and mine expands to 10^20 at max, while yours tops out at 2^30 (~10^12). |
That test doesn't test anything? It doesn't have a |
Doesn't need to; a vacuous test passes as long as it doesn't crash. |
(At least according to @domenic ^_^) |
I'm pretty sure that's not the case. I can reproduce the timeout if I check out your test locally, but that's because the harness expects at least one test to run. As soon as I apply: diff --git a/css/billion-laughs.html b/css/billion-laughs.html
index 0d80bbb7d7..648d750e09 100644
--- a/css/billion-laughs.html
+++ b/css/billion-laughs.html
@@ -46,3 +46,9 @@ an impl that correctly implements the mitigation described in the spec
will instead execute this no-op page
and thus "pass" it by default.
-->
+<script>
+let t = async_test("Foo");
+onload = t.step_func_done(function() {
+ document.documentElement.offsetTop; // ensure layout runs.
+});
+</script> The test passes just fine as expected. @jgraham do you know what's the expected behavior of a test without a test function? |
Ah, kk. Well, since your test already exercises the functionality anyway, I'll go ahead and delete mine. ^_^ |
Domenic clarifies that he said "a test without assert()", not "a test without test()", tho he recognizes the distinction wasn't particularly clear. ^_^ |
Ah, that's true :) |
Automatic update from web-platform-tests Billion Laughs Tests <w3c/csswg-drafts#3733> -- Merge pull request #15875 from web-platform-tests/tabatkins-patch-1 Billion Laughs -- wpt-commits: 74d3b810810986e21d663ec01bbe3155fb140029, 81ccef95fc4403006b46e0eb102f07b641bd571a wpt-pr: 15875
Automatic update from web-platform-tests Billion Laughs Tests <w3c/csswg-drafts#3733> -- Merge pull request #15875 from web-platform-tests/tabatkins-patch-1 Billion Laughs -- wpt-commits: 74d3b810810986e21d663ec01bbe3155fb140029, 81ccef95fc4403006b46e0eb102f07b641bd571a wpt-pr: 15875
Automatic update from web-platform-tests Billion Laughs Tests <w3c/csswg-drafts#3733> -- Merge pull request #15875 from web-platform-tests/tabatkins-patch-1 Billion Laughs -- wpt-commits: 74d3b810810986e21d663ec01bbe3155fb140029, 81ccef95fc4403006b46e0eb102f07b641bd571a wpt-pr: 15875 UltraBlame original commit: e5f751cff1dd3177f4a0eaae5633cafe04894c47
Automatic update from web-platform-tests Billion Laughs Tests <w3c/csswg-drafts#3733> -- Merge pull request #15875 from web-platform-tests/tabatkins-patch-1 Billion Laughs -- wpt-commits: 74d3b810810986e21d663ec01bbe3155fb140029, 81ccef95fc4403006b46e0eb102f07b641bd571a wpt-pr: 15875 UltraBlame original commit: e5f751cff1dd3177f4a0eaae5633cafe04894c47
Automatic update from web-platform-tests Billion Laughs Tests <w3c/csswg-drafts#3733> -- Merge pull request #15875 from web-platform-tests/tabatkins-patch-1 Billion Laughs -- wpt-commits: 74d3b810810986e21d663ec01bbe3155fb140029, 81ccef95fc4403006b46e0eb102f07b641bd571a wpt-pr: 15875 UltraBlame original commit: e5f751cff1dd3177f4a0eaae5633cafe04894c47
Should we add a size/complexity limit on the token sequence produced by var()-references?
@emilio created a test which (if allowed to complete) would spend 30GB+ (IIRC). This test uses JS, but it's easy to achieve the same with just manually typing 30ish lines of CSS.
Sidenote: Mozilla already added a 1MB limit.
The text was updated successfully, but these errors were encountered: