Merge pull request #1254 from paulkaplan/fix-undefined-block-parsing

Skip unknown opcode blocks while parsing

src/serialization/sb2.js

@@ -137,7 +137,7 @@ const parseBlockList = function (blockList, addBroadcastMsg, getVariableId, exte
         // Update commentIndex
         commentIndex = parsedBlockAndComments[1];
 
-        if (typeof parsedBlock === 'undefined') continue;
+        if (!parsedBlock) continue;
         if (previousBlock) {
             parsedBlock.parent = previousBlock.id;
             previousBlock.next = parsedBlock.id;
@@ -706,9 +706,20 @@ const specMapBlock = function (block) {
  * and second item is the updated comment index (after this block and its children are parsed)
  */
 const parseBlock = function (sb2block, addBroadcastMsg, getVariableId, extensions, comments, commentIndex) {
+    const commentsForParsedBlock = (comments && typeof commentIndex === 'number' && !isNaN(commentIndex)) ?
+        comments[commentIndex] : null;
     const blockMetadata = specMapBlock(sb2block);
     if (!blockMetadata) {
-        return;
+        // No block opcode found, exclude this block, increment the commentIndex,
+        // make all block comments into workspace comments and send them to zero/zero
+        // to prevent serialization issues.
+        if (commentsForParsedBlock) {
+            commentsForParsedBlock.forEach(comment => {
+                comment.blockId = null;
+                comment.x = comment.y = 0;
+            });
+        }
+        return [null, commentIndex + 1];
     }
     const oldOpcode = sb2block[0];
 
@@ -731,15 +742,18 @@ const parseBlock = function (sb2block, addBroadcastMsg, getVariableId, extension
     };
 
     // Attach any comments to this block..
-    const commentsForParsedBlock = (comments && typeof commentIndex === 'number' && !isNaN(commentIndex)) ?
-        comments[commentIndex] : null;
     if (commentsForParsedBlock) {
-        // TODO currently only attaching the last comment to the block if there are multiple...
+        // Attach only the last comment to the block, make all others workspace comments
-        // not sure what to do here.. concatenate all the messages in all the comments and only
-        // keep one around?
         activeBlock.comment = commentsForParsedBlock[commentsForParsedBlock.length - 1].id;
         commentsForParsedBlock.forEach(comment => {
-            comment.blockId = activeBlock.id;
+            if (comment.id === activeBlock.comment) {
+                comment.blockId = activeBlock.id;
+            } else {
+                // All other comments don't get a block ID and are sent back to zero.
+                // This is important, because if they have `null` x/y, serialization breaks.
+                comment.blockId = null;
+                comment.x = comment.y = 0;
+            }
         });
     }
     commentIndex++;

test/integration/unknown-opcode.js

@@ -0,0 +1,55 @@
+const path = require('path');
+const test = require('tap').test;
+const makeTestStorage = require('../fixtures/make-test-storage');
+const readFileToBuffer = require('../fixtures/readProjectFile').readFileToBuffer;
+const VirtualMachine = require('../../src/index');
+
+const uri = path.resolve(__dirname, '../fixtures/unknown-opcode.sb2');
+const project = readFileToBuffer(uri);
+
+test('unknown opcode', t => {
+    const vm = new VirtualMachine();
+    vm.attachStorage(makeTestStorage());
+
+    vm.start();
+    vm.clear();
+    vm.setCompatibilityMode(false);
+    vm.setTurboMode(false);
+    vm.loadProject(project).then(() => {
+        vm.greenFlag();
+
+        // The project has 3 blocks in a single stack:
+        //      play sound => "undefined" => play sound
+        // the "undefined" block has opcode "0" and was found in the wild.
+        // It should be parsed in without error and it should bridge together
+        // the two play sound blocks, so there should not be a third block.
+        const blocks = vm.runtime.targets[0].blocks;
+        const topBlockId = blocks.getScripts()[0];
+        const secondBlockId = blocks.getNextBlock(topBlockId);
+        const thirdBlockId = blocks.getNextBlock(secondBlockId);
+
+        t.equal(blocks.getBlock(topBlockId).opcode, 'sound_play');
+        t.equal(blocks.getBlock(secondBlockId).opcode, 'sound_play');
+        t.equal(thirdBlockId, null);
+
+        const target = vm.runtime.targets[0];
+        const topCommentId = blocks.getBlock(topBlockId).comment;
+        const secondCommentId = blocks.getBlock(secondBlockId).comment;
+
+        t.equal(target.comments[topCommentId].text, 'pop1 comment');
+        t.equal(target.comments[secondCommentId].text, 'pop2 comment');
+
+        // The comment previously attached to the undefined block should become
+        // a workspace comment, at 0/0, with the same text as it had.
+        const undefinedCommentId = Object.keys(target.comments).filter(id =>
+            id !== topCommentId && id !== secondCommentId)[0];
+        const undefinedComment = target.comments[undefinedCommentId];
+        t.equal(undefinedComment.blockId, null);
+        t.equal(undefinedComment.text, 'undefined comment');
+        t.equal(undefinedComment.x, 0);
+        t.equal(undefinedComment.y, 0);
+
+        t.end();
+        process.nextTick(process.exit);
+    });
+});