Bug sweep#594
Merged
Merged
Conversation
INodeList.DescendantsAndSelf() yielded each top-level node, then called INode.DescendantsAndSelf() on it, which already includes the node itself. Every top-level node was returned twice.
INodeList.Descendants() called INode.DescendantsAndSelf() on each node, so the top-level nodes were returned alongside their descendants.
… list The descendant walk is lazy, so removing a div that is a direct member of the enumerated list invalidated the enumerator and threw InvalidOperationException. Materialize the walk before scrubbing.
The single-child branch appended the child to the parent's end rather than replacing the div in place, and left the emptied div behind, so <section><div><p>x</p></div><footer/></section> became <section><div></div><footer/><p>x</p></section>. It also tested Children.Length (elements) while moving FirstChild (nodes), so any leading whitespace meant the wrong node was moved. Unwrapping now requires a single element child with only whitespace around it, which also avoids discarding text.
element.Parent was dereferenced with a null-forgiving operator, so unwrapping a detached div threw NullReferenceException.
Assigning div.InnerHtml serialized and re-parsed the whole subtree for every div. That replaced every descendant node, so nodes already collected for scrubbing were orphaned and nested divs were left unscrubbed, and it cost a full parse per div.
Previously it returned true for any div without attributes, including divs it left untouched, and false for divs it had already trimmed. The result now means what the Try prefix implies: true only when the div was removed or unwrapped. Behaviour change for callers reading the return value.
Culture-aware comparison routes markup prefix checks through ICU collation for no benefit. The class already defines an OrdinalIgnoreCase comparer.
The prefix check only matched "<!DOCTYPE html>" and "<html>" literally, so a document with attributes on the html element, leading whitespace, or a legacy doctype identifier fell through to fragment parsing. Fragment parsing drops the html, head, and body elements, so <html lang="en"> collapsed to just its body content. The ASP.NET Core default layout uses <html lang="en">.
ScrubAspCacheBusterTagHelper returned attr.Value for every attribute it did not care about, and the null check treated that as a value to write, so every attribute in the document was reassigned. Return null instead, and skip the write when the new value matches the old one.
Regex.Replace's static overload does a lock protected cache lookup on every call, and it ran for every href and src in the document. Hold a single Regex instance, and skip the match entirely when the value contains no literal v=, which the pattern requires.
StringWriter writes straight to its StringBuilder, so the explicit Flush before disposal did nothing. Initialize used a check then set on a static property, which two callers could both pass.
Measures each performance change against a verbatim copy of the code as it was before, so the deltas are real rather than asserted. Also measures the throwaway context document that fragment parsing allocates, which has not been changed.
Fragment parsing needs a context element, which meant parsing "<html><body></body></html>" on every call. An empty document yields the same html, head, and body for less work, and produces identical fragment output.
This was referenced Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug-Sweepbranch summaryCommits
DescendantsAndSelfyielding top-level nodes twiceDescendantsincluding the nodes themselvesScrubEmptyDivsthrowing when a scrubbed div is in the passed node listTryScrubDivrelocating unwrapped content to the end of the parentTryScrubDivthrowing on a div with no parentInnerHtmlround trip with a direct trailing whitespace trimTryScrubDivreturn whether the div was actually scrubbed<html>InitializeatomicProduction changes
Three files, +133 / −25:
src/Verify.AngleSharp/HtmlPrettyPrint.csTryScrubDiv; addedTrimTrailingWhitespace,TryGetOnlyElement,IsDocument; cached regex; skipped no-op attribute writessrc/Verify.AngleSharp/AngleSharpExtensions.csDescendantsAndSelfno longer double-yields;Descendantsno longer includes selfsrc/Verify.AngleSharp/VerifyAngleSharpDiffing.csInitializeviaInterlocked.ExchangeTests
+24 tests across 3 new files, all of which fail against the pre-fix code:
src/Tests/ScrubEmptyDivsTests.cs— 122 lines, 15 tests (unwrap placement, whitespace, nesting, text preservation, return contract, detached div)src/Tests/AngleSharpExtensionsTests.cs— 51 lines, 4 testssrc/Tests/PrettyPrintHtmlTests.cs— 58 lines, 4 Verify tests + verified filessrc/Tests/ScrubAspCacheBusterTagHelperTests.cs— +1 test for the regex pre-checkInfrastructure
New
src/Benchmarksproject (9 files, 356 lines), added to the solution andDirectory.Packages.props.Legacy.csholds verbatim copies of the pre-fix implementations so every perf claim is a measured before/after rather than an assertion.BenchmarkDotNet.Artifacts/gitignored.Impact
TryScrubDiv's return meaning; commits 4, 6, 9 change scrubbed output, so downstream verified files will need re-accepting