From b19af399fad3868f590858aa0dd17062ae3ee421 Mon Sep 17 00:00:00 2001
From: Christopher Willis-Ford <cwillisf@media.mit.edu>
Date: Fri, 17 Nov 2017 12:12:07 -0800
Subject: [PATCH] Make _removeThread safe during thread iteration

Before: `_removeThread` stops a thread and immediately removes it from
the runtime's thread array. If this happens while iterating over the
thread array, such as in the case of clone deletion, some elements of
the array can be missed.

After: `_removeThread` has been renamed to `_stopThread`. It still stops
the thread immediately but it no longer removes the thread from the
runtime's thread array. Instead, `_stopThread` marks the thread for
later removal. Such threads are removed at the top of the next `_step`.
---
 src/engine/runtime.js | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/src/engine/runtime.js b/src/engine/runtime.js
index 2f34cd8d0..4a2233259 100644
--- a/src/engine/runtime.js
+++ b/src/engine/runtime.js
@@ -786,17 +786,14 @@ class Runtime extends EventEmitter {
     }
 
     /**
-     * Remove a thread from the list of threads.
-     * @param {?Thread} thread Thread object to remove from actives
+     * Stop a thread: stop running it immediately, and remove it from the thread list later.
+     * @param {!Thread} thread Thread object to remove from actives
      */
-    _removeThread (thread) {
+    _stopThread (thread) {
+        // Mark the thread for later removal
+        thread.isKilled = true;
         // Inform sequencer to stop executing that thread.
         this.sequencer.retireThread(thread);
-        // Remove from the list.
-        const i = this.threads.indexOf(thread);
-        if (i > -1) {
-            this.threads.splice(i, 1);
-        }
     }
 
     /**
@@ -859,7 +856,7 @@ class Runtime extends EventEmitter {
                     // edge activated hat thread that runs every frame
                     continue;
                 }
-                this._removeThread(this.threads[i]);
+                this._stopThread(this.threads[i]);
                 return;
             }
         }
@@ -1035,8 +1032,7 @@ class Runtime extends EventEmitter {
                 continue;
             }
             if (this.threads[i].target === target) {
-                this.threads[i].isKilled = true;
-                this._removeThread(this.threads[i]);
+                this._stopThread(this.threads[i]);
             }
         }
     }
@@ -1075,11 +1071,7 @@ class Runtime extends EventEmitter {
         }
         this.targets = newTargets;
         // Dispose all threads.
-        const threadsCopy = this.threads.slice();
-        while (threadsCopy.length > 0) {
-            const poppedThread = threadsCopy.pop();
-            this._removeThread(poppedThread);
-        }
+        this.threads.forEach(thread => this._stopThread(thread));
     }
 
     /**
@@ -1093,6 +1085,10 @@ class Runtime extends EventEmitter {
             }
             this.profiler.start(stepProfilerId);
         }
+
+        // Clean up threads that were told to stop during or since the last step
+        this.threads = this.threads.filter(thread => !thread.isKilled);
+
         // Find all edge-activated hats, and add them to threads to be evaluated.
         for (const hatType in this._hats) {
             if (!this._hats.hasOwnProperty(hatType)) continue;