perf: setTimeout and requestAnimationFrame memory leak (videojs#5294)

gkatsev · web-flow · commit d7f27b711281 · 2018-07-05T16:29:03.000-04:00
Our setTimeout and requestAnimationFrame methods added dispose handlers so that they get cancelled if the component is disposed before they get a chance to run. However, we were only clearing out these dispose handlers if we cleared the timeout or the rAF manually. Instead make sure that we remove the dispose handler when it is no longer needed. Fixes videojs#5199.
diff --git a/src/js/component.js b/src/js/component.js
@@ -1245,10 +1245,18 @@ class Component {
    * @see [Similar to]{@link https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers/setTimeout}
    */
   setTimeout(fn, timeout) {
+    // declare as variables so they are properly available in timeout function
+    // eslint-disable-next-line
+    var timeoutId, disposeFn;
+
     fn = Fn.bind(this, fn);
 
-    const timeoutId = window.setTimeout(fn, timeout);
-    const disposeFn = () => this.clearTimeout(timeoutId);
+    timeoutId = window.setTimeout(() => {
+      this.off('dispose', disposeFn);
+      fn();
+    }, timeout);
+
+    disposeFn = () => this.clearTimeout(timeoutId);
 
     disposeFn.guid = `vjs-timeout-${timeoutId}`;
 
@@ -1371,11 +1379,19 @@ 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);
 
-      const id = window.requestAnimationFrame(fn);
-      const disposeFn = () => this.cancelAnimationFrame(id);
+      id = window.requestAnimationFrame(() => {
+        this.off('dispose', disposeFn);
+        fn();
+      });
+
+      disposeFn = () => this.cancelAnimationFrame(id);
 
       disposeFn.guid = `vjs-raf-${id}`;
       this.on('dispose', disposeFn);
diff --git a/test/unit/component.test.js b/test/unit/component.test.js
@@ -957,6 +957,55 @@ QUnit.test('*AnimationFrame methods fall back to timers if rAF not supported', f
   window.cancelAnimationFrame = oldCAF;
 });
 
+QUnit.test('setTimeout should remove dispose handler on trigger', function(assert) {
+  const comp = new Component(getFakePlayer());
+  const el = comp.el();
+  const data = DomData.getData(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');
+
+  this.clock.tick(1);
+
+  assert.equal(data.handlers.dispose.length, 1, '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.getData(el);
+  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(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');
+
+  this.clock.tick(1);
+
+  assert.equal(data.handlers.dispose.length, 1, 'we removed our dispose handle');
+
+  comp.dispose();
+
+  window.requestAnimationFrame = oldRAF;
+  window.cancelAnimationFrame = oldCAF;
+});
+
 QUnit.test('$ and $$ functions', function(assert) {
   const comp = new Component(getFakePlayer());
   const contentEl = document.createElement('div');
