diff --git a/src/js/component.js b/src/js/component.js index 89f4430a9e..7bf0ae6f63 100644 --- a/src/js/component.js +++ b/src/js/component.js @@ -98,6 +98,11 @@ class Component { this.childIndex_ = {}; this.childNameIndex_ = {}; + this.setTimeoutIds_ = new Set(); + this.setIntervalIds_ = new Set(); + this.rafIds_ = new Set(); + this.clearingTimersOnDispose_ = false; + // Add any child components in options if (options.initChildren !== false) { this.initChildren(); @@ -1293,16 +1298,16 @@ class Component { fn = Fn.bind(this, fn); + this.clearTimersOnDispose_(); + timeoutId = window.setTimeout(() => { - this.off('dispose', disposeFn); + if (this.setTimeoutIds_.has(timeoutId)) { + this.setTimeoutIds_.delete(timeoutId); + } fn(); }, timeout); - disposeFn = () => this.clearTimeout(timeoutId); - - disposeFn.guid = `vjs-timeout-${timeoutId}`; - - this.on('dispose', disposeFn); + this.setTimeoutIds_.add(timeoutId); return timeoutId; } @@ -1323,13 +1328,10 @@ class Component { * @see [Similar to]{@link https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers/clearTimeout} */ clearTimeout(timeoutId) { - window.clearTimeout(timeoutId); - - const disposeFn = function() {}; - - disposeFn.guid = `vjs-timeout-${timeoutId}`; - - this.off('dispose', disposeFn); + if (this.setTimeoutIds_.has(timeoutId)) { + this.setTimeoutIds_.delete(timeoutId); + window.clearTimeout(timeoutId); + } return timeoutId; } @@ -1357,13 +1359,11 @@ class Component { setInterval(fn, interval) { fn = Fn.bind(this, fn); - const intervalId = window.setInterval(fn, interval); - - const disposeFn = () => this.clearInterval(intervalId); + this.clearTimersOnDispose_(); - disposeFn.guid = `vjs-interval-${intervalId}`; + const intervalId = window.setInterval(fn, interval); - this.on('dispose', disposeFn); + this.setIntervalIds_.add(intervalId); return intervalId; } @@ -1384,13 +1384,10 @@ class Component { * @see [Similar to]{@link https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers/clearInterval} */ clearInterval(intervalId) { - window.clearInterval(intervalId); - - const disposeFn = function() {}; - - disposeFn.guid = `vjs-interval-${intervalId}`; - - this.off('dispose', disposeFn); + if (this.setIntervalIds_.has(intervalId)) { + this.setIntervalIds_.delete(intervalId); + window.clearInterval(intervalId); + } return intervalId; } @@ -1421,28 +1418,27 @@ class Component { * @see [Similar to]{@link https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame} */ requestAnimationFrame(fn) { - // declare as variables so they are properly available in rAF function - // eslint-disable-next-line - var id, disposeFn; - - if (this.supportsRaf_) { - fn = Fn.bind(this, fn); - - id = window.requestAnimationFrame(() => { - this.off('dispose', disposeFn); - fn(); - }); + // Fall back to using a timer. + if (!this.supportsRaf_) { + return this.setTimeout(fn, 1000 / 60); + } - disposeFn = () => this.cancelAnimationFrame(id); + this.clearTimersOnDispose_(); - disposeFn.guid = `vjs-raf-${id}`; - this.on('dispose', disposeFn); + // declare as variables so they are properly available in rAF function + // eslint-disable-next-line + var id; + fn = Fn.bind(this, fn); - return id; - } + id = window.requestAnimationFrame(() => { + if (this.rafIds_.has(id)) { + this.rafIds_.delete(id); + } + fn(); + }); + this.rafIds_.add(id); - // Fall back to using a timer. - return this.setTimeout(fn, 1000 / 60); + return id; } /** @@ -1462,20 +1458,47 @@ class Component { * @see [Similar to]{@link https://developer.mozilla.org/en-US/docs/Web/API/window/cancelAnimationFrame} */ cancelAnimationFrame(id) { - if (this.supportsRaf_) { - window.cancelAnimationFrame(id); + // Fall back to using a timer. + if (!this.supportsRaf_) { + return this.clearTimeout(id); + } - const disposeFn = function() {}; + if (this.rafIds_.has(id)) { + this.rafIds_.delete(id); + window.cancelAnimationFrame(id); + } - disposeFn.guid = `vjs-raf-${id}`; + return id; - this.off('dispose', disposeFn); + } - return id; + /** + * A function to setup `requestAnimationFrame`, `setTimeout`, + * and `setInterval`, clearing on dispose. + * + * > Previously each timer added and removed dispose listeners on it's own. + * For better performance it was decided to batch them all, and use `Set`s + * to track outstanding timer ids. + * + * @private + */ + clearTimersOnDispose_() { + if (this.clearingTimersOnDispose_) { + return; } - // Fall back to using a timer. - return this.clearTimeout(id); + this.clearingTimersOnDispose_ = true; + this.one('dispose', () => { + [ + ['rafIds_', 'cancelAnimationFrame'], + ['setTimeoutIds_', 'clearTimeout'], + ['setIntervalIds_', 'clearInterval'] + ].forEach(([idName, cancelName]) => { + this[idName].forEach(this[cancelName], this); + }); + + this.clearingTimersOnDispose_ = false; + }); } /** diff --git a/test/unit/component.test.js b/test/unit/component.test.js index 0c842328bc..0da9d90511 100644 --- a/test/unit/component.test.js +++ b/test/unit/component.test.js @@ -978,25 +978,20 @@ QUnit.test('*AnimationFrame methods fall back to timers if rAF not supported', f QUnit.test('setTimeout should remove dispose handler on trigger', function(assert) { const comp = new Component(getFakePlayer()); - const el = comp.el(); - const data = DomData.get(el); comp.setTimeout(() => {}, 1); - assert.equal(data.handlers.dispose.length, 2, 'we got a new dispose handler'); - assert.ok(/vjs-timeout-\d/.test(data.handlers.dispose[1].guid), 'we got a new dispose handler'); + assert.equal(comp.setTimeoutIds_.size, 1, 'we removed our dispose handle'); this.clock.tick(1); - assert.equal(data.handlers.dispose.length, 1, 'we removed our dispose handle'); + assert.equal(comp.setTimeoutIds_.size, 0, 'we removed our dispose handle'); comp.dispose(); }); QUnit.test('requestAnimationFrame should remove dispose handler on trigger', function(assert) { const comp = new Component(getFakePlayer()); - const el = comp.el(); - const data = DomData.get(el); const oldRAF = window.requestAnimationFrame; const oldCAF = window.cancelAnimationFrame; @@ -1012,12 +1007,40 @@ QUnit.test('requestAnimationFrame should remove dispose handler on trigger', fun comp.requestAnimationFrame(spyRAF); - assert.equal(data.handlers.dispose.length, 2, 'we got a new dispose handler'); - assert.ok(/vjs-raf-\d/.test(data.handlers.dispose[1].guid), 'we got a new dispose handler'); + assert.equal(comp.rafIds_.size, 1, 'we got a new dispose handler'); + + this.clock.tick(1); + + assert.equal(comp.rafIds_.size, 0, 'we removed our dispose handle'); + + comp.dispose(); + + window.requestAnimationFrame = oldRAF; + window.cancelAnimationFrame = oldCAF; +}); + +QUnit.test('requestAnimationFrame should remove dispose handler on trigger', function(assert) { + const comp = new Component(getFakePlayer()); + const oldRAF = window.requestAnimationFrame; + const oldCAF = window.cancelAnimationFrame; + + // Stub the window.*AnimationFrame methods with window.setTimeout methods + // so we can control when the callbacks are called via sinon's timer stubs. + window.requestAnimationFrame = (fn) => window.setTimeout(fn, 1); + window.cancelAnimationFrame = (id) => window.clearTimeout(id); + + // Make sure the component thinks it supports rAF. + comp.supportsRaf_ = true; + + const spyRAF = sinon.spy(); + + comp.requestAnimationFrame(spyRAF); + + assert.equal(comp.rafIds_.size, 1, 'we got a new dispose handler'); this.clock.tick(1); - assert.equal(data.handlers.dispose.length, 1, 'we removed our dispose handle'); + assert.equal(comp.rafIds_.size, 0, 'we removed our dispose handle'); comp.dispose(); @@ -1025,6 +1048,87 @@ QUnit.test('requestAnimationFrame should remove dispose handler on trigger', fun window.cancelAnimationFrame = oldCAF; }); +QUnit.test('setTimeout should be canceled on dispose', function(assert) { + const comp = new Component(getFakePlayer()); + let called = false; + let clearId; + const setId = comp.setTimeout(() => { + called = true; + }, 1); + + const clearTimeout = comp.clearTimeout; + + comp.clearTimeout = (id) => { + clearId = id; + return clearTimeout.call(comp, id); + }; + + assert.equal(comp.setTimeoutIds_.size, 1, 'we added a timeout id'); + + comp.dispose(); + + assert.equal(comp.setTimeoutIds_.size, 0, 'we removed our timeout id'); + assert.equal(clearId, setId, 'clearTimeout was called'); + + this.clock.tick(1); + + assert.equal(called, false, 'setTimeout was never called'); +}); + +QUnit.test('requestAnimationFrame should be canceled on dispose', function(assert) { + const comp = new Component(getFakePlayer()); + let called = false; + let clearId; + const setId = comp.requestAnimationFrame(() => { + called = true; + }); + + const cancelAnimationFrame = comp.cancelAnimationFrame; + + comp.cancelAnimationFrame = (id) => { + clearId = id; + return cancelAnimationFrame.call(comp, id); + }; + + assert.equal(comp.rafIds_.size, 1, 'we added a raf id'); + + comp.dispose(); + + assert.equal(comp.rafIds_.size, 0, 'we removed a raf id'); + assert.equal(clearId, setId, 'clearAnimationFrame was called'); + + this.clock.tick(1); + + assert.equal(called, false, 'requestAnimationFrame was never called'); +}); + +QUnit.test('setInterval should be canceled on dispose', function(assert) { + const comp = new Component(getFakePlayer()); + let called = false; + let clearId; + const setId = comp.setInterval(() => { + called = true; + }); + + const clearInterval = comp.clearInterval; + + comp.clearInterval = (id) => { + clearId = id; + return clearInterval.call(comp, id); + }; + + assert.equal(comp.setIntervalIds_.size, 1, 'we added an interval id'); + + comp.dispose(); + + assert.equal(comp.setIntervalIds_.size, 0, 'we removed a raf id'); + assert.equal(clearId, setId, 'clearInterval was called'); + + this.clock.tick(1); + + assert.equal(called, false, 'setInterval was never called'); +}); + QUnit.test('$ and $$ functions', function(assert) { const comp = new Component(getFakePlayer()); const contentEl = document.createElement('div');