interpreter: Do not re-evaluate procedure arguments after stop this s…

…cript or return (#202)

"stop this script" and "return" would previously cause all the procedure
arguments to be re-evaluated and then discarded. In vanilla Scratch the
worst case is a performance penalty, but with our custom reporters it
can cause infinite loops.

Fixes #201

src/engine/thread.js

@@ -316,12 +316,22 @@ class Thread {
         let blockID = this.peekStack();
         while (blockID !== null) {
             const block = this.target.blocks.getBlock(blockID);
-            if (
-                (typeof block !== 'undefined' && block.opcode === 'procedures_call') ||
-                this.peekStackFrame().waitingReporter
-            ) {
+
+            // Reporter form of procedures_call
+            if (this.peekStackFrame().waitingReporter) {
                 break;
             }
+
+            // Command form of procedures_call
+            if (typeof block !== 'undefined' && block.opcode === 'procedures_call') {
+                // By definition, if we get here, the procedure is done, so skip ahead so
+                // the arguments won't be re-evaluated and then discarded as frozen state
+                // about which arguments have been evaluated is lost.
+                // This fixes https://github.com/TurboWarp/scratch-vm/issues/201
+                this.goToNextBlock();
+                break;
+            }
+
             this.popStack();
             blockID = this.peekStack();
         }

test/snapshot/__snapshots__/tw-gh-201-stop-script-does-not-reevaluate-arguments.sb3.tw-snapshot

@@ -0,0 +1,25 @@
+// TW Snapshot
+// Input SHA-256: fd981742e0e4299bad5a89349635d3a7d0467d8163ae77ba4bafe43c97849bae
+
+// Sprite1 script
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+const b0 = runtime.getOpcodeFunction("looks_say");
+return function* genXYZ () {
+yield* executeInCompatibilityLayer({"MESSAGE":"plan 0",}, b0, false, false, "d", null);
+thread.procedures["Zfoo %s"]("");
+yield* executeInCompatibilityLayer({"MESSAGE":"end",}, b0, false, false, "g", null);
+retire(); return;
+}; })
+
+// Sprite1 foo %s
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+return function funXYZ_foo_ (p0) {
+return "";
+return "";
+}; })
+
+// Sprite1 no op
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+return function funXYZ_no_op () {
+return "";
+}; })

test/snapshot/__snapshots__/warp-timer/tw-gh-201-stop-script-does-not-reevaluate-arguments.sb3.tw-snapshot

@@ -0,0 +1,25 @@
+// TW Snapshot
+// Input SHA-256: fd981742e0e4299bad5a89349635d3a7d0467d8163ae77ba4bafe43c97849bae
+
+// Sprite1 script
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+const b0 = runtime.getOpcodeFunction("looks_say");
+return function* genXYZ () {
+yield* executeInCompatibilityLayer({"MESSAGE":"plan 0",}, b0, false, false, "d", null);
+thread.procedures["Zfoo %s"]("");
+yield* executeInCompatibilityLayer({"MESSAGE":"end",}, b0, false, false, "g", null);
+retire(); return;
+}; })
+
+// Sprite1 foo %s
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+return function funXYZ_foo_ (p0) {
+return "";
+return "";
+}; })
+
+// Sprite1 no op
+(function factoryXYZ(thread) { const target = thread.target; const runtime = target.runtime; const stage = runtime.getTargetForStage();
+return function funXYZ_no_op () {
+return "";
+}; })

test/unit/engine_thread.js

@@ -223,7 +223,7 @@ test('stopThisScript', t => {
     th.pushStack('arbitraryString');
     th.pushStack('secondString');
     th.stopThisScript();
-    t.strictEquals(th.peekStack(), 'secondString');
+    t.strictEquals(th.peekStack(), null);
 
     t.end();
 });

test/unit/tw_stop_this_script.js

@@ -0,0 +1,52 @@
+const {test} = require('tap');
+const Thread = require('../../src/engine/thread');
+const Runtime = require('../../src/engine/runtime');
+const Target = require('../../src/engine/target');
+
+test('stopThisScript procedures_call reporter form', t => {
+    const rt = new Runtime();
+    const target = new Target(rt, null);
+
+    target.blocks.createBlock({
+        id: 'reporterCall',
+        opcode: 'procedures_call',
+        inputs: {},
+        fields: {},
+        mutation: {
+            return: '1'
+        },
+        shadow: false,
+        topLevel: true,
+        parent: null,
+        next: 'afterReporterCall'
+    });
+    target.blocks.createBlock({
+        id: 'afterReporterCall',
+        opcode: 'motion_ifonedgebounce',
+        inputs: {},
+        fields: {},
+        mutation: null,
+        shadow: false,
+        topLevel: false,
+        parent: null,
+        next: null
+    });
+
+    const thread = new Thread('reporterCall');
+    thread.target = target;
+
+    // pretend to run reporterCall
+    thread.pushStack('reporterCall');
+    thread.peekStackFrame().waitingReporter = true;
+
+    // pretend to run scripts inside of the procedure
+    thread.pushStack('fakeBlock');
+
+    // stopping or returning should always return to reporterCall, not the block after
+    thread.stopThisScript();
+    t.same(thread.stack, [
+        'reporterCall'
+    ]);
+
+    t.end();
+});