davesecops opened a new pull request, #21537:
URL: https://github.com/apache/echarts/pull/21537
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
Adds null guards to `getDataParams()` and its 6 chart-specific overrides to
prevent TypeErrors when `getData()` returns null on disposed series during
mouse events.
### Fixed issues
- #9402: Cannot read property 'getRawIndex' of undefined (pie, reported 2018)
- #16998: Same error on stacked line chart click (open)
- #21535: Comprehensive analysis of the root cause and all affected code
paths
## Details
### Before: What was the problem?
When a chart series is disposed (via `echarts.dispose()`, component unmount
in React/Vue/Angular, `setOption` with `notMerge`, or toolbox restore),
`SeriesModel.getData()` returns `null`/`undefined`. The comment in `Series.ts`
explicitly acknowledges this:
```typescript
// When series is not alive (that may happen when click toolbox
// restore or setOption with not merge mode), series data may
// be still need to judge animation or something when graphic
// elements want to know whether fade out.
return inner(this).data; // ← can be undefined
```
However, `getDataParams()` and its overrides unconditionally call methods on
the result:
```typescript
// dataFormat.ts — base method
const data = this.getData(dataType);
const rawDataIndex = data.getRawIndex(dataIndex); // 💥 TypeError
// TreemapSeries.ts — override
const node = this.getData().tree.getNodeByDataIndex(dataIndex); // 💥
TypeError
// PieSeries.ts — override
const data = this.getData();
data.each(data.mapDimension('value'), ...); // 💥 TypeError
```
The primary crash trigger is the event handler in `echarts.ts` which calls
`getDataParams()` during `mousemove`/`mouseout` events that fire on stale DOM
elements after chart disposal. This affects **all chart types** in **all
frameworks** where component lifecycle causes disposal during user interaction.
### After: How does it behave after the fixing?
Three layers of defense:
**1. Base method guard** (`src/model/mixin/dataFormat.ts`):
```typescript
const data = this.getData(dataType);
if (!data) {
return {} as CallbackDataParams;
}
```
Returns `{}` when data is null — consistent with the existing fallback at
`echarts.ts:778` (`dataModel.getDataParams(...) || {}`). Protects all callers
outside the event handler (tooltips, labels, visual pipeline).
**2. Event handler guard** (`src/core/echarts.ts`):
```typescript
try {
params = (dataModel && dataModel.getDataParams(...) || {}) as
ECElementEvent;
} catch (e) {
params = {} as ECElementEvent;
}
```
Wraps the highest-risk call site in try/catch, catching crashes from any
chart override's `getData()` access.
**3. Override guards** (6 chart series files):
Each override now checks `getData()` before accessing chart-specific
properties:
- `TreemapSeries.ts` — guards `data.tree` access
- `SunburstSeries.ts` — guards `data.tree` access
- `TreeSeries.ts` — guards `data.tree` access
- `PieSeries.ts` — guards `data.each()`, `data.mapDimension()` access
- `FunnelSeries.ts` — guards `data.mapDimension()`, `data.getSum()` access
- `ChordSeries.ts` — guards `data.getName()`, `this.getGraph()` access
This pattern follows existing precedent in the codebase —
`SeriesModel.getAllData()` already null-guards `getData()`:
```typescript
const mainData = this.getData();
return mainData && mainData.getLinkedDataAll ? mainData.getLinkedDataAll() :
[{ data: mainData }];
```
### Related test cases
Added `test/ut/spec/model/getDataParams.test.ts` — 5 test cases covering
pie, treemap, funnel, sunburst, and tree series. Each test mocks `getData()` to
return null and verifies `getDataParams()` does not throw.
All existing tests continue to pass (the 1 pre-existing failure in
`time.test.ts:roundTime_locale` is a DST-dependent issue on `master`, unrelated
to this PR).
## Document Info
- [x] This PR doesn't relate to document changes
## Misc
### Security Checking
- [ ] This PR uses security-sensitive Web APIs.
### ZRender Changes
- [ ] This PR depends on ZRender changes.
### Related test cases or examples to use the new APIs
See `test/ut/spec/model/getDataParams.test.ts` — 5 unit tests covering all
guarded chart types.
### Merging options
- [x] Please squash the commits into a single one when merging.
### Other information
This bug has been open since 2018 (#9402) and affects all chart types. It is
the most commonly reported ECharts crash in React/Vue applications where charts
are rendered in routed views. The fix is minimal (35 lines added across 8
source files + 1 test file), safe (returns empty params matching existing
fallback patterns), and follows internal precedent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
