Skip to content

Fix circular custom properties during computed style resolution - #242

Merged
FlorianRappl merged 2 commits into
AngleSharp:hotfix/custom-property-cyclesfrom
sebastienros:sebros/custom-property-cycles
Sep 5, 2026
Merged

Fix circular custom properties during computed style resolution#242
FlorianRappl merged 2 commits into
AngleSharp:hotfix/custom-property-cyclesfrom
sebastienros:sebros/custom-property-cycles

Conversation

@sebastienros

@sebastienros sebastienros commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Types of Changes

Prerequisites

Please make sure you can check the following two boxes:

  • I have read the CONTRIBUTING document
  • My code follows the code style of this project

Contribution Type

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue, please reference the issue id)
  • New feature (non-breaking change which adds functionality, make sure to open an associated issue first)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Description

Circular CSS custom properties currently cause unbounded recursion during computed style resolution, terminating the host process with an uncatchable stack overflow. This change makes every member of a cyclic component guaranteed-invalid, allowing consumers to use fallbacks or normal computed-value defaulting instead.

Fixes: #241

Variable resolution is confined to computed-style preparation. GetDeclarations, ComputeExplicitStyle, ComputeCascadedStyle, and render-tree SpecifiedStyle retain the original variable expressions. Computed results are separate declarations; stylesheet and inline values are not rewritten.

Within computation, dependencies are discovered using the CSS tokenizer, including references in unused fallbacks but excluding literal strings, comments, and URLs. An iterative strongly connected component traversal resolves local custom properties before inheritance. Inherited aliases retain their parent's resolved token values, while declarations explicitly matching both parent and child resolve locally. Selector matching is reused rather than repeated to identify local properties.

Consumer and shorthand substitution preserves the complete token stream and token boundaries, deferring grammar conversion until the consuming property is known. Invalid-at-computed-value declarations inherit or use their initial value rather than exposing an earlier cascaded declaration. Custom-property names remain case-sensitive through CSSOM lookup and mutation.

Compatibility revision

The follow-up restores the three public behaviors identified in review: raw cascades do not resolve variables, nested var() fallbacks remain CssVarValue objects, and direct CssReferenceValue.Compute uses the supplied References array, including modified entries and its first-success behavior. Public parser source-position behavior is preserved. Caller-modified references are also honored in computed custom properties and shorthands. Nested fallback parsing, serialization, and computation remain iterative.

Bounds and limitations

To bound exponential expansion during style computation, substituted values have a documented limit of 1,048,576 UTF-16 code units, including inserted token separators. Existing property grammar and layout limitations remain unchanged.

Validation

  • Full unit suite: 2,167 passed on .NET 8 and 2,167 passed on .NET 10. The .NET 10 run used -p:TargetFrameworks=net10.0 -p:RestoreEnablePackagePruning=false to retain the existing test dependency references without editing project manifests.
  • Library builds for netstandard2.0, net8.0, and net10.0 passed with no warnings or errors. Windows-only net462 and net472 targets were not exercised on macOS.
  • Regression coverage includes the original reproduction, self/multi-node/overlapping cycles, unused fallback edges, inherited aliases, shared rules, render-tree computation, CSSOM mutation and priorities, shorthand substitution, token boundaries, 4,096-member chains and cycles, 8,192 nested fallbacks, expansion-limit boundaries, and 100 randomized graphs checked against reachability.
  • Added 19 compatibility cases covering raw declarations and specified styles, nested public fallback trees, source-position preservation, supplied and mutable references, modified custom-property/shorthand references, empty versus absent fallbacks, exception propagation, and 8,192-level public fallback parsing, serialization, and computation. Testhosts used a 60-second hang timeout with core dumps disabled.

Resolve per-element custom-property dependency components iteratively before inheritance, including unused fallback edges. Preserve computed token values, substitute complete consumer and shorthand values, and apply computed-value defaults without recursive variable evaluation.

Add regression coverage for cycles, inheritance, shared rules, CSSOM mutation, token boundaries, long chains, nested fallbacks, and bounded expansion. Document the substitution limit.

Fixes AngleSharp#241

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@sebastienros

Copy link
Copy Markdown
Contributor Author

@FlorianRappl this is obviously AI generated (Astra) and I won't be offended if you prefer not to deal with this PR. At this point I don't even know if you think the issue needs to be solved yet. At least if that helps you validate a solution that's good enough for me. Otherwise don't hesitate to shut it down.

@sebastienros

Copy link
Copy Markdown
Contributor Author

Note: behavioral changes on noncyclic inputs:

  • GetDeclarations() now resolves custom properties: an alias previously returned as var(--a) now returns red. Callers inspecting uncomputed declarations can be affected.
  • ParseVarFallback() changes the returned value-tree type: a nested fallback previously returned CssVarValue; it now returns CssAnyValue. Downstream casts or tree inspection can break.
  • CssReferenceValue.Compute() no longer follows supplied or modified References metadata: it derives references from the stored CSS text instead.

@FlorianRappl

Copy link
Copy Markdown
Contributor

Yes the behavioral changes should not be there. For instance, only the style computations should resolve CSS variables. This is also necessary and the real browser behavior.

Keep variable resolution inside computed-style preparation; leave raw cascades and render-tree specified styles unchanged. Restore nested CssVarValue fallback trees and direct References-based computation, retaining iterative parsing, serialization, and fallback traversal.

Honor caller-modified references in custom-property and shorthand resolution. Add compatibility regressions for raw declarations, public parser cursor and value-tree behavior, mutable references, and deep public fallbacks.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@sebastienros

Copy link
Copy Markdown
Contributor Author

Addressed in 99c535a. Variable resolution now stays in computed-style preparation; GetDeclarations, ComputeCascadedStyle, and render-tree SpecifiedStyle retain the raw expressions. Nested fallbacks again expose CssVarValue objects, and direct CssReferenceValue.Compute honors supplied/modified References and its previous first-success behavior. The fallback traversal remains iterative. Added compatibility regressions, including caller-modified custom-property and shorthand references. The full suite passes on .NET 8 and .NET 10 (2,167 tests each), including the original cycle and deep-fallback cases.

Comment thread src/AngleSharp.Css/Values/Functions/CssVarValue.cs
Comment thread src/AngleSharp.Css/Values/Functions/CssVarValue.cs

@FlorianRappl FlorianRappl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho the PR is larger than it needs / should be, but as style computation is complex and most likely not yet fully there it should be a good basis.

I think we can merge this when the comments have been addressed and performance does not show regression (missing comparison: beforehand vs now - how much are the changes costing in terms of perf).

@FlorianRappl
FlorianRappl changed the base branch from devel to hotfix/custom-property-cycles September 5, 2026 19:36
@FlorianRappl FlorianRappl added this to the v1.1.0 milestone Sep 5, 2026
@FlorianRappl
FlorianRappl merged commit 4f469d6 into AngleSharp:hotfix/custom-property-cycles Sep 5, 2026
5 checks passed
@FlorianRappl

Copy link
Copy Markdown
Contributor

There is a slight perf. penalty here, but it's acceptable:

Baseline was:

Method Mean Error StdDev Gen0 Gen1 Allocated
ComputedStyle 8.425 ms 0.1580 ms 0.1941 ms 156.2500 - 1.33 MB
RenderTree 13.420 ms 0.2514 ms 0.2469 ms 609.3750 406.2500 4.93 MB
ParseInlineDeclarations 1.237 ms 0.0243 ms 0.0648 ms 220.7031 5.8594 1.76 MB

With this PR we get:

Method Mean Error StdDev Gen0 Gen1 Allocated
ComputedStyle 8.804 ms 0.1753 ms 0.4200 ms 218.7500 - 1.84 MB
RenderTree 13.468 ms 0.2659 ms 0.3363 ms 640.6250 437.5000 5.16 MB
ParseInlineDeclarations 1.188 ms 0.0237 ms 0.0389 ms 220.7031 5.8594 1.76 MB

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Circular CSS custom properties cause a process-fatal stack overflow during computed style resolution

2 participants