[css-variables] Token sequence size limit

[andruud]

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.

[AmeliaBR]

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.

[Loirooriol]

Please don't enforce a specific maximum, see #3462 (comment)

Must support up to X degree of nesting size; can support more but are not required to; must fail in a particular standardized way when the limit is exceeded.

[tabatkins]

Yup, I'm fine with specifying that limits are allowed (and encouraged!). The best behavior is likely just "drop all tokens after the limit".

[AmeliaBR]

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 var() function.

[tabatkins]

Yeah, you're right.

[tabatkins]

@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?

[tabatkins]

Ah, looks like the FF fix was backed out for linting failures. ^_^ Welp, it's a nice failing test for everyone right now, then.

[emilio]

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 :).

[tabatkins]

I'm not using anything, but the WPT TaskCluster instance, apparently using FF Nightly, is timing out on my test:
https://tools.taskcluster.net/groups/NeZQtx9rQ6O08PnfyLtA4Q/tasks/UqF3N5yyQ3-1Zie-2K47UA/runs/0/logs/public%2Flogs%2Flive.log.

(I didn't realize you'd committed a similar test already.)

[tabatkins]

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).

[emilio]

That test doesn't test anything? It doesn't have a test(function() {})?

[tabatkins]

Doesn't need to; a vacuous test passes as long as it doesn't crash.

[tabatkins]

(At least according to @domenic ^_^)

[emilio]

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?

[tabatkins]

Ah, kk. Well, since your test already exercises the functionality anyway, I'll go ahead and delete mine. ^_^

[tabatkins]

Domenic clarifies that he said "a test without assert()", not "a test without test()", tho he recognizes the distinction wasn't particularly clear. ^_^

[emilio]

Ah, that's true :)