fix(tree): avoid crashes when data is emptied or nodes are removed. - #21735
Open
SEPURI-SAI-KRISHNA wants to merge 1 commit into
Open
fix(tree): avoid crashes when data is emptied or nodes are removed.#21735SEPURI-SAI-KRISHNA wants to merge 1 commit into
SEPURI-SAI-KRISHNA wants to merge 1 commit into
Conversation
|
Thanks for your contribution! Please DO NOT commit the files in dist, i18n, and ssr/client/dist folders in a non-release pull request. These folders are for release use only. |
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
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.
Brief Information
This pull request is in the type of:
What does this PR do?
Fixes two crashes in the tree series: rendering a tree with no laid-out nodes, and removing several nodes in one update.
Fixed issues
Details
Before: What was the problem?
Two independent crashes, both hit by ordinary tree data updates.
1. A tree with no nodes to lay out throws.
_updateViewCoordSyscollects node positions and computes their extent:bbox.fromPointsreturns immediately without writing anything whenpointsisempty, so
minandmaxstay empty arrays.max[0] - min[0]is thenNaN, theexisting zero-size corrections below only test
=== 0so they never fire, and thedataRect handed to the view coordinate system is
NaN. That makes the resultingmatrix non-invertible,
matrix.invertreturnsnull, andlegacyCopyOverallTransdereferences it.
This is not only the literal
data: []case — it also covers a tree whose nodeshave no valid layout yet, e.g. rendering before an async fetch resolves, or after
filtering the data down to nothing.
2. Removing several nodes in one update throws.
removeNodeEdgeguards its own node's graphic element but not its source's:removeNodesetsdata.setItemGraphicEl(dataIndex, null)in the removal callback.With
animation: falsethat callback runs synchronously, so when a batch of nodesis removed the source node's element is frequently already
nullby the time itschildren's edges are cleaned up.
This one is independent of the empty-data case: it fires whenever removed nodes
include a parent, even when the resulting tree is not empty (dropping a subtree, or
collapsing back to just the root).
After: How does it behave after the fixing?
min/maxare seeded when there is no valid point — from the previous extent ifthere is one (the mechanism already used for the collapsed-root case), otherwise
from zero. The existing zero-size corrections then expand it into a usable rect,
so the view transform stays invertible.
sourceSymbolElis guarded exactly likesymbolElimmediately above it.sourceEdgeis only used as a fallback and everything downstream is alreadybehind
if (edge), so a missing source simply means there is no edge to remove.Emptying a tree, refilling it, and removing subtrees all work with animation on and off.
Document Info
One of the following should be checked.
Misc
Security Checking
ZRender Changes
Related test cases or examples to use the new APIs
Added
test/ut/spec/series/treeUpdate.test.ts, covering an initially empty tree, atree emptied after having data, removing a subtree, removing all but the root, and
refilling an empty tree — with
animationbothfalseandtrue.On
masterthe empty-data cases fail regardless of animation, and the node-removalcases fail with
animation: false. Theanimation: trueremoval cases pass beforeand after, and are kept to document that the timing of the removal callback is what
exposes the second bug.
npm run test,npx tsc --noEmitandeslinton the changed file all pass.Merging options
Other information
src/chart/tree/TreeView.tsis also touched by #18491, #21603 and #21681. Thosechanges are in different functions from the two guards here, though whichever lands
first may need a trivial rebase.
Both crashes end in the same place from a user's point of view — clearing or
shrinking tree data — so they are fixed together rather than split across two PRs
that would conflict in the same file.