From 34b0aff6370bf54bbefa24e25745a633796bb0ba Mon Sep 17 00:00:00 2001
From: Karishma Chadha <kchadha@scratch.mit.edu>
Date: Mon, 4 Dec 2017 18:01:29 -0500
Subject: [PATCH] Bugfix for scratch-gui issue #994, where executing a
 broadcast block from the flyout was creating a conflicting variable, causing
 a fatal error.

---
 src/blocks/scratch3_event.js | 50 +++++++++++++++++++-----------------
 src/engine/target.js         | 23 +++++++++++------
 test/unit/blocks_event.js    |  6 +++--
 test/unit/engine_target.js   | 17 ++----------
 4 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/src/blocks/scratch3_event.js b/src/blocks/scratch3_event.js
index 5fbaa5b7d..5ee2fab1d 100644
--- a/src/blocks/scratch3_event.js
+++ b/src/blocks/scratch3_event.js
@@ -56,36 +56,40 @@ class Scratch3EventBlocks {
     }
 
     broadcast (args, util) {
-        const broadcastVar = util.runtime.getTargetForStage().lookupOrCreateBroadcastMsg(
+        const broadcastVar = util.runtime.getTargetForStage().lookupBroadcastMsg(
             args.BROADCAST_OPTION.id, args.BROADCAST_OPTION.name);
-        const broadcastOption = broadcastVar.name;
-        util.startHats('event_whenbroadcastreceived', {
-            BROADCAST_OPTION: broadcastOption
-        });
+        if (broadcastVar) {
+            const broadcastOption = broadcastVar.name;
+            util.startHats('event_whenbroadcastreceived', {
+                BROADCAST_OPTION: broadcastOption
+            });
+        }
     }
 
     broadcastAndWait (args, util) {
-        const broadcastVar = util.runtime.getTargetForStage().lookupOrCreateBroadcastMsg(
+        const broadcastVar = util.runtime.getTargetForStage().lookupBroadcastMsg(
             args.BROADCAST_OPTION.id, args.BROADCAST_OPTION.name);
-        const broadcastOption = broadcastVar.name;
-        // Have we run before, starting threads?
-        if (!util.stackFrame.startedThreads) {
-            // No - start hats for this broadcast.
-            util.stackFrame.startedThreads = util.startHats(
-                'event_whenbroadcastreceived', {
-                    BROADCAST_OPTION: broadcastOption
+        if (broadcastVar) {
+            const broadcastOption = broadcastVar.name;
+            // Have we run before, starting threads?
+            if (!util.stackFrame.startedThreads) {
+                // No - start hats for this broadcast.
+                util.stackFrame.startedThreads = util.startHats(
+                    'event_whenbroadcastreceived', {
+                        BROADCAST_OPTION: broadcastOption
+                    }
+                );
+                if (util.stackFrame.startedThreads.length === 0) {
+                    // Nothing was started.
+                    return;
                 }
-            );
-            if (util.stackFrame.startedThreads.length === 0) {
-                // Nothing was started.
-                return;
             }
-        }
-        // We've run before; check if the wait is still going on.
-        const instance = this;
-        const waiting = util.stackFrame.startedThreads.some(thread => instance.runtime.isActiveThread(thread));
-        if (waiting) {
-            util.yield();
+            // We've run before; check if the wait is still going on.
+            const instance = this;
+            const waiting = util.stackFrame.startedThreads.some(thread => instance.runtime.isActiveThread(thread));
+            if (waiting) {
+                util.yield();
+            }
         }
     }
 }
diff --git a/src/engine/target.js b/src/engine/target.js
index 39273fcc2..4e574177e 100644
--- a/src/engine/target.js
+++ b/src/engine/target.js
@@ -4,6 +4,7 @@ const Blocks = require('./blocks');
 const Variable = require('../engine/variable');
 const uid = require('../util/uid');
 const {Map} = require('immutable');
+const log = require('../util/log');
 
 /**
  * @fileoverview
@@ -93,19 +94,25 @@ class Target extends EventEmitter {
     }
 
     /**
-     * Look up a broadcast message object, and create it if one doesn't exist.
+     * Look up a broadcast message object with the given id and return it
+     * if it exists.
      * @param {string} id Id of the variable.
      * @param {string} name Name of the variable.
      * @return {!Variable} Variable object.
      */
-    lookupOrCreateBroadcastMsg (id, name) {
+    lookupBroadcastMsg (id, name) {
         const broadcastMsg = this.lookupVariableById(id);
-        if (broadcastMsg) return broadcastMsg;
-        // No variable with this name exists - create it locally.
-        const newBroadcastMsg = new Variable(id, name,
-            Variable.BROADCAST_MESSAGE_TYPE, false);
-        this.variables[id] = newBroadcastMsg;
-        return newBroadcastMsg;
+        if (broadcastMsg) {
+            if (broadcastMsg.name !== name) {
+                log.error(`Found broadcast message with id: ${id}, but` +
+                    `its name, ${broadcastMsg.name} did not match expected name ${name}.`);
+            }
+            if (broadcastMsg.type !== Variable.BROADCAST_MESSAGE_TYPE) {
+                log.error(`Found variable with id: ${id}, but its type ${broadcastMsg.type}` +
+                    `did not match expected type ${Variable.BROADCAST_MESSAGE_TYPE}`);
+            }
+            return broadcastMsg;
+        }
     }
 
     /**
diff --git a/test/unit/blocks_event.js b/test/unit/blocks_event.js
index 35a1f0b4f..672ea332c 100644
--- a/test/unit/blocks_event.js
+++ b/test/unit/blocks_event.js
@@ -6,6 +6,7 @@ const Event = require('../../src/blocks/scratch3_event');
 const Runtime = require('../../src/engine/runtime');
 const Target = require('../../src/engine/target');
 const Thread = require('../../src/engine/thread');
+const Variable = require('../../src/engine/variable');
 
 test('#760 - broadcastAndWait', t => {
     const broadcastAndWaitBlock = {
@@ -52,6 +53,7 @@ test('#760 - broadcastAndWait', t => {
     b.createBlock(receiveMessageBlock);
     const tgt = new Target(rt, b);
     tgt.isStage = true;
+    tgt.createVariable('testBroadcastID', 'message', Variable.BROADCAST_MESSAGE_TYPE);
     rt.targets.push(tgt);
 
     let th = rt._pushThread('broadcastAndWaitBlock', t);
@@ -67,12 +69,12 @@ test('#760 - broadcastAndWait', t => {
     // yields when some thread is active
     t.strictEqual(th.status, Thread.STATUS_YIELD);
     th.status = Thread.STATUS_RUNNING;
-    e.broadcastAndWait({BROADCAST_OPTION: 'message'}, util);
+    e.broadcastAndWait({BROADCAST_OPTION: {id: 'testBroadcastID', name: 'message'}}, util);
     t.strictEqual(th.status, Thread.STATUS_YIELD);
     // does not yield once all threads are done
     th.status = Thread.STATUS_RUNNING;
     rt.threads[1].status = Thread.STATUS_DONE;
-    e.broadcastAndWait({BROADCAST_OPTION: 'message'}, util);
+    e.broadcastAndWait({BROADCAST_OPTION: {id: 'testBroadcastID', name: 'message'}}, util);
     t.strictEqual(th.status, Thread.STATUS_RUNNING);
 
     // restarts done threads that are in runtime threads
diff --git a/test/unit/engine_target.js b/test/unit/engine_target.js
index 4f663906d..30ff5eb82 100644
--- a/test/unit/engine_target.js
+++ b/test/unit/engine_target.js
@@ -174,20 +174,7 @@ test('lookupOrCreateList returns list if one with given id exists', t => {
     t.end();
 });
 
-test('lookupOrCreateBroadcastMsg creates a var if one does not exist', t => {
-    const target = new Target();
-    const variables = target.variables;
-
-    t.equal(Object.keys(variables).length, 0);
-    const broadcastVar = target.lookupOrCreateBroadcastMsg('foo', 'bar');
-    t.equal(Object.keys(variables).length, 1);
-    t.equal(broadcastVar.id, 'foo');
-    t.equal(broadcastVar.name, 'bar');
-
-    t.end();
-});
-
-test('lookupOrCreateBroadcastMsg returns the var with given id if exists', t => {
+test('lookupBroadcastMsg returns the var with given id if exists', t => {
     const target = new Target();
     const variables = target.variables;
 
@@ -195,7 +182,7 @@ test('lookupOrCreateBroadcastMsg returns the var with given id if exists', t =>
     target.createVariable('foo', 'bar', Variable.BROADCAST_MESSAGE_TYPE);
     t.equal(Object.keys(variables).length, 1);
 
-    const broadcastMsg = target.lookupOrCreateBroadcastMsg('foo', 'bar');
+    const broadcastMsg = target.lookupBroadcastMsg('foo', 'bar');
     t.equal(Object.keys(variables).length, 1);
     t.equal(broadcastMsg.id, 'foo');
     t.equal(broadcastMsg.name, 'bar');