From 011551276711abe80e60cee12253c7ceb95c6c70 Mon Sep 17 00:00:00 2001
From: Kevin Barabash <kevinb@khanacademy.org>
Date: Thu, 28 Jan 2016 11:04:19 -0800
Subject: [PATCH] Revert code that caused a memory leak

Summary:
As I was refactoring how we do code injection I noticed that the `this.instances` in
PJSOutput wasn't hooked up to anything so `if (this.instances) { this.instances.push(obj); }`
wasn't doing anything.  To "fix" this I added an `instances` array to the PJSOutput class object
and modified the `injectCode` instance method to use `PJSOutput.instances` instead.
Now that PJSOutput had an `instances` array, we were pushing every object ever created onto
that array.  I thought that reset the array whenever `injectCode` was called would handle
memory issues but I hadn't considered programs that new up object indefinitely.

This diff reverts these changes.  This means that all code having to do with `instances`
does nothing.  I'll remove this code in a following diff.

Test Plan: - test/output/pjs/index.html

Reviewers: pamela, john

Reviewed By: john

Differential Revision: https://phabricator.khanacademy.org/D25010
---
 build/js/live-editor.output_pjs.js | 13 ++++++-------
 js/output/pjs/pjs-code-injector.js | 12 ++++++------
 js/output/pjs/pjs-output.js        |  1 -
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/build/js/live-editor.output_pjs.js b/build/js/live-editor.output_pjs.js
index a19b9218e..1fac13c3a 100644
--- a/build/js/live-editor.output_pjs.js
+++ b/build/js/live-editor.output_pjs.js
@@ -650,7 +650,7 @@ var PJSCodeInjector = (function () {
             this.grabObj = {};
 
             // Extract a list of instances that were created using applyInstance
-            PJSOutput.instances = [];
+            this.instances = [];
 
             // If we have a draw function then we need to do injection
             // If we had a draw function then we still need to do injection
@@ -699,13 +699,13 @@ var PJSCodeInjector = (function () {
 
                 // Keep track of all the constructor functions that may
                 // have to be reinitialized
-                for (var i = 0, l = PJSOutput.instances.length; i < l; i++) {
-                    constructors[PJSOutput.instances[i].constructor.__name] = true;
+                for (var i = 0, l = this.instances.length; i < l; i++) {
+                    constructors[this.instances[i].constructor.__name] = true;
                 }
 
                 // The instantiated instances have changed, which means that
                 // we need to re-run everything.
-                if (this.oldInstances && PJSOutput.stringifyArray(this.oldInstances) !== PJSOutput.stringifyArray(PJSOutput.instances)) {
+                if (this.oldInstances && PJSOutput.stringifyArray(this.oldInstances) !== PJSOutput.stringifyArray(this.instances)) {
                     rerun = true;
                 }
 
@@ -727,8 +727,8 @@ var PJSCodeInjector = (function () {
                 }
 
                 // Reset the instances list
-                this.oldInstances = PJSOutput.instances;
-                PJSOutput.instances = [];
+                this.oldInstances = this.instances;
+                this.instances = [];
 
                 var _loop = function (i) {
                     // Reconstruction the function call
@@ -2962,7 +2962,6 @@ window.PJSOutput = Backbone.View.extend({
 
 // Add in some static helper methods
 _.extend(PJSOutput, {
-    instances: [],
 
     // Turn a JavaScript object into a form that can be executed
     // (Note: The form will not necessarily be able to pass a JSON linter)
diff --git a/js/output/pjs/pjs-code-injector.js b/js/output/pjs/pjs-code-injector.js
index 775a159be..4cfa040a8 100644
--- a/js/output/pjs/pjs-code-injector.js
+++ b/js/output/pjs/pjs-code-injector.js
@@ -590,7 +590,7 @@ class PJSCodeInjector {
         this.grabObj = {};
 
         // Extract a list of instances that were created using applyInstance
-        PJSOutput.instances = [];
+        this.instances = [];
 
         // If we have a draw function then we need to do injection
         // If we had a draw function then we still need to do injection
@@ -642,15 +642,15 @@ class PJSCodeInjector {
 
             // Keep track of all the constructor functions that may
             // have to be reinitialized
-            for (let i = 0, l = PJSOutput.instances.length; i < l; i++) {
-                constructors[PJSOutput.instances[i].constructor.__name] = true;
+            for (let i = 0, l = this.instances.length; i < l; i++) {
+                constructors[this.instances[i].constructor.__name] = true;
             }
 
             // The instantiated instances have changed, which means that
             // we need to re-run everything.
             if (this.oldInstances &&
                 PJSOutput.stringifyArray(this.oldInstances) !==
-                PJSOutput.stringifyArray(PJSOutput.instances)) {
+                PJSOutput.stringifyArray(this.instances)) {
                 rerun = true;
             }
 
@@ -672,8 +672,8 @@ class PJSCodeInjector {
             }
 
             // Reset the instances list
-            this.oldInstances = PJSOutput.instances;
-            PJSOutput.instances = [];
+            this.oldInstances = this.instances;
+            this.instances = [];
 
             // Look for new top-level function calls to inject
             for (let i = 0; i < fnCalls.length; i++) {
diff --git a/js/output/pjs/pjs-output.js b/js/output/pjs/pjs-output.js
index fd204b76a..009db6fcc 100644
--- a/js/output/pjs/pjs-output.js
+++ b/js/output/pjs/pjs-output.js
@@ -497,7 +497,6 @@ window.PJSOutput = Backbone.View.extend({
 
 // Add in some static helper methods
 _.extend(PJSOutput, {
-    instances: [],
 
     // Turn a JavaScript object into a form that can be executed
     // (Note: The form will not necessarily be able to pass a JSON linter)