Skip to content

Take the review of #508: depth scan and lexer cleanups - #510

Merged
jmanico merged 5 commits into
mainfrom
css-scan-cleanups
Sep 25, 2026
Merged

Take the review of #508: depth scan and lexer cleanups#510
jmanico merged 5 commits into
mainfrom
css-scan-cleanups

Conversation

@jmanico

@jmanico jmanico commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Follow-up to #508, from the review passes on that PR and on this one. No behaviour change.

CssGrammar

  • The function mark is set on its own line with a name, not inside an if condition that read as a typo for ==. The case body has braces, so the local is not in scope in the later cases.
  • The javadoc says that after a semicolon inside a function the scan counts functions the value parse never reads, so it can drop a declaration the parse would have kept. That can lose valid CSS, since if() separates its branches with semicolons (Sol-6's review). The loss is conservative, and only a value nested past the limit is affected.
  • Array growth stays plain doubling, with a comment saying why overflow is out of reach: the lexer's tables give out first.

CssTokens

  • The per-kind count of open brackets is three ints indexed by bracket kind, not 126 ints indexed by character.
  • The bound in the partner walk stays, as a second line of defence should the count ever drift from the stack.

Tests

  • CssFuzzerTest.testBracketPairingMatchesAStackWalk compares the lexer's pairing with a plain stack walk on random bracket soup. Checked against mutations: a count that is never incremented, or falls behind, fails it; one that runs ahead with the bound removed throws ArrayIndexOutOfBoundsException; with the bound kept, that drift is harmless.
  • The churn timing test stays at 500,000 repeats under the suite's usual 20 s budget. Its comment now says what it catches, and that catching the BitSet by the clock would take an input needing about 1 GB of heap.

Verification

  • ./mvnw clean verify passes on JDK 11, 17, 21 and 25 (711 library tests, 7 in the JPMS consumer check).
  • The whole library suite passes with the forked test JVM at -Xmx192m.
  • 500,000 random inputs and 27 realistic values give output identical to main. Lexer normalization of orphaned and mismatched closes is byte-identical. The depth fuzz never sees more than sixteen functions deep, and hostile shapes stay linear.

🤖 Generated with Claude Code

jmanico and others added 5 commits September 24, 2026 19:15
@jmanico @claude
Take the review: make the depth scan's marking plain to read
cf9567f 
The review of #508 asked for three things in skipIfNestedTooDeeply,
none of them a change in behaviour.  The mark was set inside the if
condition, which reads as a typo for ==; give the flag a name and set
it on its own line.  The array doubled without bound, which would wrap
past a billion brackets; saturate at the largest array size, which no
input reaches since the lexer's token tables fill memory first.  And
say in the javadoc that, after a semicolon inside a function, the scan
counts functions the value parse never reads, so it can drop a value
the parse would have kept: it errs towards dropping, and only on a
value no browser keeps.

The churn timing test guarded only a gross regression: the BitSet it
was written against passed it in a second.  At three million repeats
the BitSet took 27 s and the array 0.8 s, so run it at that size with a
10 s budget, which tells them apart with a margin on both sides.

500,000 random inputs and 27 realistic values give the same output as
before; the fuzz still never sees more than sixteen functions deep.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jmanico @claude
Take the review: count open brackets in three ints, not 126
de57cff 
The count of open brackets per close character, added in #508 to drop
an unmatched close without walking the stack, was an int array indexed
by the character, 126 ints zeroed for every lexer.  Index it by bracket
kind instead, three ints.  The orphan check inside the walk that the
count made unreachable is gone; the count says a partner is on the
stack, so the walk ends at it.  Normalized output is unchanged, and the
unmatched-close shapes stay linear.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jmanico @claude
Take the review: say the depth scan's over-count can drop valid CSS
f95efad 
The javadoc added in cf9567f said the scan's over-count, after a
semicolon inside a function, drops only values no browser keeps.  Not
so: CSS if() separates its branches with semicolons, so
"margin: 1px if(media(print): 2px; else: calc(...))" is valid CSS.  The
scan drops it whole once the else branch nests sixteen calc() deep,
where the parse before #508 kept "margin:1px" at any depth.  Found by
Sol-6's review.

Say that it can lose valid CSS, that the loss is conservative, and that
only a value nested past the limit is affected.  No code change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jmanico @claude
Take the review: keep the walk's guard, drop the dead growth cap
4c910d3 
Three points from the review of this PR.

de57cff removed the bound in CssTokens.Lexer.closeBracket's partner
walk, since the per-kind count makes it unreachable.  But it cost
nothing, and without it any future drift of the count ahead of the
stack reads open[-1] and throws ArrayIndexOutOfBoundsException out of
sanitize().  Keep it, and say it is a second line of defence.

cf9567f capped the depth scan's array growth "never past what an array
can hold".  It did not: at the cap Arrays.copyOf returns the same
length and the next store throws, and the lexer's own tables, several
ints per bracket, give out long before depth gets there anyway.  Go
back to plain doubling with a comment saying why that is enough.  The
local declared in the case group now has braces, so it is not in scope
in the cases after it.

And the review asked for a hostile test of the count itself.
CssFuzzerTest.testBracketPairingMatchesAStackWalk compares the lexer's
pairing with a plain stack walk on random bracket soup, function
openers included.  Checked against mutations: a count never
incremented, or one that falls behind, fails it on output; one that
runs ahead with the bound removed throws ArrayIndexOutOfBoundsException
in it; with the bound kept, the same drift is harmless.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jmanico @claude
Take the review: size the churn test for an ordinary heap
dfe357e 
cf9567f ran the churn timing test at three million repeats with a
10 s budget, so that it would fail on the BitSet it was written
against.  The review measured what that costs: the input needs about a
gigabyte of heap, and runs out of memory at 512 and 768 MB.  Surefire
sets no heap size, so the default of a quarter of RAM fails it on any
machine or container with less than about 4 GB.  With the heap tight,
the array took two seconds rather than the "under a second" the
comment claimed, and a machine three times faster would pass the
BitSet inside 10 s anyway.

Go back to 500,000 repeats, which fits in 192 MB, under the suite's
usual 20 s budget.  Say plainly what that catches, bookkeeping whose
work per close grows with the depth, and what it does not: the BitSet,
whose cost per clear is a sixty-fourth of that.  The method javadoc no
longer lists the BitSet among the shapes the test tells apart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jmanico

jmanico commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

Review follow-up: the code-review pass on this PR and Sol-6's review. Every finding was checked against main and the previous head de57cff before classifying it.

Finding Outcome Evidence
Sol-6: javadoc says the over-count only drops values "no browser would keep" Fixed in f95efad margin: 1px if(media(print): 2px; else: calc(...)) keeps margin:1px at any depth before #508, and is dropped whole here once the else branch nests 16 calc( deep. if() separates branches with semicolons, so this is valid CSS. Fail-closed; wording corrected.
Churn test at 3M repeats needs ~1 GB heap Fixed in dfe357e Measured: runs out of memory at 512 and 768 MB, passes at 1 GB. Surefire sets no heap, so machines under ~4 GB RAM fail. Back to 500k, which fits in 192 MB.
10 s wall-clock budget is machine-dependent Fixed in dfe357e Back to the suite's usual 20 s. The comment now says the test cannot tell the BitSet apart without ~1 GB of heap.
Comment's timings not reproducible Fixed in dfe357e Measured 2.1 s, not "under a second", with the heap tight. The numbers are gone.
Removing the partner walk's bound turns count drift into an exception out of sanitize() Fixed in 4c910d3 Mutation with the count running ahead and the bound removed throws ArrayIndexOutOfBoundsException; with the bound kept, the same drift is harmless.
"Saturating" growth still throws at the cap Fixed in 4c910d3 True by reading: at the cap Arrays.copyOf returns the same length. Reverted to plain doubling; unreachable because the lexer's tables give out first.
No hostile test for the count refactor Fixed in 4c910d3 New pairing fuzz against a plain stack walk. Two falling-behind mutations fail it on output.
Case-local isFunction leaks into later cases Fixed in 4c910d3 Case body braced, as the lexer's own switch does.
expandIfNecessary overflows near 2^30 elements Not changed Pre-existing since before #508, and needs over 2^29 brackets in one attribute, a 4 GB int[]. The heap runs out first.
Three switches per bracket pair Not changed A switch on a char costs nanoseconds. Storing kinds in the stack instead would rewrite more of the pairing code to save them.
PR bundles several changes Not changed Each commit is one review item and reverts on its own.

Verification after the follow-up: clean verify green on JDK 11, 17, 21, 25 (711 tests); whole suite green at -Xmx192m; 500k random inputs identical to main; depth fuzz max 16; hostile shapes linear.

@jmanico
jmanico merged commit 05ca527 into main Sep 25, 2026
7 checks passed
This was referenced Sep 25, 2026
Merged
Merged
@github-actions github-actions Bot added the released Issue has been released label Sep 25, 2026
@github-actions

github-actions Bot commented Sep 25, 2026

Copy link
Copy Markdown

🎉 This issue has been resolved in release-20260924.2 (Release Notes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

released Issue has been released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jmanico