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

@SEPURI-SAI-KRISHNA SEPURI-SAI-KRISHNA commented Aug 15, 2026

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.
#18491 is in the same function as the second guard here and fixes the same crash
(more thoroughly — it threads the symbols through so sourceSymbolEl is never
null, and also fixes polyline edge cleanup when animation is off). If you would
rather land that one, I am happy to drop the removeNodeEdge hunk and scope this
PR to the empty-data crash, which #18491 does not cover. #21603 and #21681 are in
different functions; 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.

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown
Author

Noting some prior work I came across: the removeNodeEdge guard in this PR overlaps #18491, which fixes the same Cannot read properties of null (reading '__edge') crash. That PR goes further, it threads the current/source symbols into removeNodeEdge so sourceSymbolEl is never null to begin with, and it also fixes polyline edge cleanup when animation is off (#18448, #18490).

The other change here is independent of #18491: when series.data is emptied, or when no node has a layout yet, bbox.fromPoints returns early and leaves min/max as empty arrays. max[0] - min[0] is then NaN, which the zero-size checks below don't correct, so the resulting dataRect produces a singular view transform, matrix.invert returns null, and legacyCopyOverallTrans throws.

If you'd rather land #18491 for the removeNodeEdge part, I'm happy to drop that hunk and scope this PR to the empty-data crash alone. Just let me know which you prefer.

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