Skip to content

fix(tree): avoid crashes when data is emptied or nodes are removed. - #21735

Open
SEPURI-SAI-KRISHNA wants to merge 1 commit into
apache:masterfrom
SEPURI-SAI-KRISHNA:fix/tree-empty-and-node-removal
Open

fix(tree): avoid crashes when data is emptied or nodes are removed.#21735
SEPURI-SAI-KRISHNA wants to merge 1 commit into
apache:masterfrom
SEPURI-SAI-KRISHNA:fix/tree-empty-and-node-removal

Conversation

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

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.

chart.setOption({series: [{type: 'tree', data: []}]});
TypeError: Cannot read properties of null (reading '0')
    at legacyCopyOverallTrans (src/coord/View.ts:567)
    at viewCoordSysUpdateOverallTrans (src/coord/View.ts:548)
    ...
    at TreeView._updateViewCoordSys (src/chart/tree/TreeView.ts)

_updateViewCoordSys collects node positions and computes their extent:

const min: number[] = [];
const max: number[] = [];
bbox.fromPoints(points, min, max);

bbox.fromPoints returns immediately without writing anything when points is
empty, so min and max stay empty arrays. max[0] - min[0] is then NaN, the
existing zero-size corrections below only test === 0 so they never fire, and the
dataRect handed to the view coordinate system is NaN. That makes the resulting
matrix non-invertible, matrix.invert returns null, and legacyCopyOverallTrans
dereferences it.

This is not only the literal data: [] case — it also covers a tree whose nodes
have 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.

chart.setOption({animation: false, series: [{type: 'tree', data: fullTree}]});
// drop a subtree
chart.setOption({series: [{type: 'tree', data: smallerTree}]}, true);
TypeError: Cannot read properties of null (reading '__edge')
    at removeNodeEdge (src/chart/tree/TreeView.ts:590)
    at removeNode (src/chart/tree/TreeView.ts:690)
    at DataDiffer._remove

removeNodeEdge guards its own node's graphic element but not its source's:

const symbolEl = data.getItemGraphicEl(node.dataIndex) as TreeSymbol;
if (!symbolEl) {
    return;
}
const sourceSymbolEl = data.getItemGraphicEl(source.dataIndex) as TreeSymbol;
const sourceEdge = sourceSymbolEl.__edge;   // <- source may already be gone

removeNode sets data.setItemGraphicEl(dataIndex, null) in the removal callback.
With animation: false that callback runs synchronously, so when a batch of nodes
is removed the source node's element is frequently already null by the time its
children'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?

  1. min/max are seeded when there is no valid point — from the previous extent if
    there 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.
  2. sourceSymbolEl is guarded exactly like symbolEl immediately above it.
    sourceEdge is only used as a fallback and everything downstream is already
    behind 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.

  • This PR doesn't relate to document changes
  • The document should be updated later
  • The document changes have been made in apache/echarts-doc#xxx

Misc

Security Checking

  • This PR uses security-sensitive Web APIs.

ZRender Changes

  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

Added test/ut/spec/series/treeUpdate.test.ts, covering an initially empty tree, a
tree emptied after having data, removing a subtree, removing all but the root, and
refilling an empty tree — with animation both false and true.

On master the empty-data cases fail regardless of animation, and the node-removal
cases fail with animation: false. The animation: true removal cases pass before
and after, and are kept to document that the timing of the removal callback is what
exposes the second bug.

npm run test, npx tsc --noEmit and eslint on the changed file all pass.

Merging options

  • Please squash the commits into a single one when merging.

Other information

src/chart/tree/TreeView.ts is also touched by #18491, #21603 and #21681. Those
changes 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.

@echarts-bot

echarts-bot Bot commented Aug 15, 2026

Copy link
Copy Markdown

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant