Skip to content

Commit 4050123

Browse files
committed
[Slider] fix memory leaks
1 parent 50c656b commit 4050123

File tree

2 files changed

+35
-8
lines changed

2 files changed

+35
-8
lines changed

packages/material-ui-lab/src/Slider/Slider.js

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,10 @@ function addEventListener(node, event, handler, capture) {
142142
},
143143
};
144144
}
145+
// mock return value of addEventListener with noop; used as default
146+
const noopEventListener = {
147+
remove: () => {},
148+
};
145149

146150
function percentToValue(percent, min, max) {
147151
return ((max - min) * percent) / 100 + min;
@@ -198,6 +202,12 @@ if (process.env.NODE_ENV !== 'production' && !React.createContext) {
198202
class Slider extends React.Component {
199203
state = { currentState: 'initial' };
200204

205+
globalMouseMoveListener = noopEventListener;
206+
207+
globalMouseUpListener = noopEventListener;
208+
209+
jumpAnimationTimeoutId = -1;
210+
201211
componentDidMount() {
202212
if (this.containerRef) {
203213
this.containerRef.addEventListener('touchstart', preventPageScrolling, { passive: false });
@@ -206,6 +216,9 @@ class Slider extends React.Component {
206216

207217
componentWillUnmount() {
208218
this.containerRef.removeEventListener('touchstart', preventPageScrolling, { passive: false });
219+
this.globalMouseMoveListener.remove();
220+
this.globalMouseUpListener.remove();
221+
clearTimeout(this.jumpAnimationTimeoutId);
209222
}
210223

211224
static getDerivedStateFromProps(nextProps, prevState) {
@@ -303,13 +316,8 @@ class Slider extends React.Component {
303316
handleMouseUp = event => {
304317
this.setState({ currentState: 'normal' });
305318

306-
if (this.globalMouseUpListener) {
307-
this.globalMouseUpListener.remove();
308-
}
309-
310-
if (this.globalMouseMoveListener) {
311-
this.globalMouseMoveListener.remove();
312-
}
319+
this.globalMouseMoveListener.remove();
320+
this.globalMouseUpListener.remove();
313321

314322
if (typeof this.props.onDragEnd === 'function') {
315323
this.props.onDragEnd(event);
@@ -373,7 +381,8 @@ class Slider extends React.Component {
373381

374382
playJumpAnimation() {
375383
this.setState({ currentState: 'jumped' }, () => {
376-
setTimeout(() => {
384+
clearTimeout(this.jumpAnimationTimeoutId);
385+
this.jumpAnimationTimeoutId = setTimeout(() => {
377386
this.setState({ currentState: 'normal' });
378387
}, this.props.theme.transitions.duration.complex);
379388
});

packages/material-ui-lab/src/Slider/Slider.test.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,24 @@ describe('<Slider />', () => {
5555
assert.strictEqual(handleDragEnd.callCount, 1, 'should have called the handleDragEnd cb');
5656
});
5757

58+
it('should not cause leaks', () => {
59+
const wrapper = mount(<Slider value={0} />);
60+
61+
wrapper.simulate('mousedown');
62+
wrapper.unmount();
63+
64+
// would cause errors because we try to access refs that are null
65+
const mousemoveEvent = document.createEvent('HTMLEvents');
66+
mousemoveEvent.initEvent('mousemove', false, true);
67+
document.dispatchEvent(mousemoveEvent);
68+
69+
// if we did not remove the global event listener react will throw a warning
70+
// because we called setState on an unmounted component
71+
const mouseupEvent = document.createEvent('HTMLEvents');
72+
mouseupEvent.initEvent('mouseup', false, true);
73+
document.dispatchEvent(mouseupEvent);
74+
});
75+
5876
describe('prop: vertical', () => {
5977
it('should render with the default and vertical classes', () => {
6078
const wrapper = shallow(<Slider vertical value={0} />);

0 commit comments

Comments
 (0)