Stop DeepClone/DeepCopy and node teardown from recursing per tree level - #1091
Open
afonsojanu wants to merge 1 commit into
Open
Conversation
A tree built up at runtime through the normal Insert*Child API isn't bounded by XMLDocument::DepthTracker the way a parsed one is, so a long enough chain of single-child nodes can push both operations past their stack limit. DeepClone (and DeepCopy, which just calls it per top level node) walked children recursively, and even after the destructor got its sibling-iteration cleanup in leethomason#1075, tearing down a node still recursed once per level through DeleteNode for anything nested more than a couple of siblings deep. Both now gather the subtree with an explicit, heap backed stack instead. For teardown, every descendant has its own _firstChild link cleared before it's actually destroyed, so each one's derived destructor runs without trying to recurse back into children that are already queued up in the same pass. Added a test with a 60000 level deep tree that reliably overflowed the stack before this change and now runs in well under a second. Reproduces the crash reported in leethomason#839 for both DeepClone/DeepCopy and plain destruction; Accept() has the same recursive shape but is a separate code path and isn't touched here.
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
2 participants
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.
Fixes the stack overflow half of #839.
A tree built up at runtime through the normal
Insert*ChildAPI isn't bounded byXMLDocument::DepthTrackerthe way a parsed one is, so a long chain of single-child nodes can push both operations past the stack limit.DeepClone(andDeepCopy, which just calls it once per top level node) walked children recursively. Node teardown had a partial fix in #1075 for the sibling case, but a node still recurses throughDeleteNodeonce per level for anything nested more than a couple of levels deep, since destroying a node destroys its child, whose destructor then destroys its own child, and so on.Both now gather the subtree with an explicit, heap backed stack instead of recursing. For teardown specifically, every descendant gets its own
_firstChildlink cleared before it's actually destroyed, so each one's (possibly virtual, derived class) destructor still runs correctly but finds no children left to recurse into, since they're already queued up for destruction in the same pass.Added a test that builds a 60000 level deep tree, which reliably overflowed the stack on my machine before this change and now finishes in well under a second, covering both
DeepCopy/DeepCloneand plain destruction.Accept()has the same recursive shape (mentioned in the issue too) but it's a separate code path with its own visitor dispatch, so I left it out of this one to keep the diff focused. Happy to take a pass at that separately if useful.Ran the full
xmltestsuite locally (530 tests) before and after: 1 crash pre-fix (SIGSEGV, confirmed with the new test reverted onto the old code), 530 passing post-fix.