From e7e300caeb9bd90a44c30af4fa64c71f23e0ec83 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Tue, 19 Jun 2018 15:08:27 -0400 Subject: [PATCH 1/4] Skip unknown opcode blocks while parsing --- src/serialization/sb2.js | 21 +++++++++------- test/fixtures/unknown-opcode.sb2 | Bin 0 -> 3949 bytes test/integration/unknown-opcode.js | 37 +++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/unknown-opcode.sb2 create mode 100644 test/integration/unknown-opcode.js diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index ffde8220a..5ce617c2e 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -133,17 +133,20 @@ const parseBlockList = function (blockList, addBroadcastMsg, getVariableId, exte // eslint-disable-next-line no-use-before-define const parsedBlockAndComments = parseBlock(block, addBroadcastMsg, getVariableId, extensions, comments, commentIndex); - const parsedBlock = parsedBlockAndComments[0]; - // Update commentIndex - commentIndex = parsedBlockAndComments[1]; - if (typeof parsedBlock === 'undefined') continue; - if (previousBlock) { - parsedBlock.parent = previousBlock.id; - previousBlock.next = parsedBlock.id; + if (parsedBlockAndComments) { // Could fail due to unrecognized op-codes + const parsedBlock = parsedBlockAndComments[0]; + // Update commentIndex + commentIndex = parsedBlockAndComments[1]; + + if (typeof parsedBlock === 'undefined') continue; + if (previousBlock) { + parsedBlock.parent = previousBlock.id; + previousBlock.next = parsedBlock.id; + } + previousBlock = parsedBlock; + resultingList.push(parsedBlock); } - previousBlock = parsedBlock; - resultingList.push(parsedBlock); } return [resultingList, commentIndex]; }; diff --git a/test/fixtures/unknown-opcode.sb2 b/test/fixtures/unknown-opcode.sb2 new file mode 100644 index 0000000000000000000000000000000000000000..81e191b46ba03a01c321b289419237b07155e655 GIT binary patch literal 3949 zcmWIWW@h1HU|`^2D5*H)v+CEE6OR}f7&4d{7nx=q;Xz_i>;p`rhO=Lk>A(c(C0`~CHt>a(|t z8dYllaoM_;3*L&hC=ju}V6rRpn#G5gnuiNRQa1i}TGf1guMB%x`y&p`l^68e7pGL! zJ}3>>cz!Y9ZSKy9Id6Ba=I>em;qSG^lPq6!z6**r?potD&2)=E%QV)5CdobpTBmwM zp1xxBE9QI=dfa^7gwmN7>(wHfzwW(ca`0yh$I9vth8&i^Z7<|3*_eDjzhmCSnOd#S z%r6HmeV!-6^Pg#b4Ev>*^$+r-Z%kjZ%<_1H@ucpi<$Dg6`kt~{Dyfzkw&qH~jMrPY zv45Q_eY1Dl)Wcg>OcS%&^sxEquZ!8IC-ukJ*ZNJ3G3|}AKAt92WN+~AxlR0%9|fn4 z^H+&*XtG@B`m~4Tn#z(kE(6sijaAPoPwvj)ioB=&^X2VxH(eS0bRQSpU8($ILa5vf zncIDqdUd-RqpUez^4Mpl#2w*ZcXo;7+)YZ6s;e{4oY1&?>$G2UmWq|Z@!36hs~&HY zRe$GKX!*3a?e*8jcAYKxcO*0V1mjdMHVedBe|TY>z4l7*hD)Djnww=fNnD>=-Ieo6 z_F#^c+MxTUSKLZ zyg~M9f2CGuoLP&_Nk)d^i9j(45Hd&rp#u$!K*Yez20{>tkt_!Z6zt(LGUOl$0zFMY z{613TR2fh*1hVI$67bA20_E_P60aDgZ?Z0w%s)8|SRUz?xJHx&=ckpFCl;kLBvtAq z<>cq5q*Ur97gZLNRnIfEpP*UHx3vIVCiqmWUPF z1*<*)OG6L_m57GKmWXw*644S^w{jdYX#1Q$^EgYe(n9acs~H-?M~g&!MgGVr_JF>G zlx{e4{%Dbi)J7PA#U8#QA77Dpj^Q&K=iK54H@j$1B+mcXy3T+J*f<1YNF%X4u`I~b z%}oO+5bhY}nwDFlAON(O5eNmf85oqp7#Nrs1Q=2hOA=XtYOXRcGw)?D;(ad~ENi1Y zTji0$YO!Lr(2w!A5{~q4&RjWZ#nX-MM;#x2W_qi3(cLStHeb1bEAgDmD_O12{3o-P z|DCAX`)-Ei))#jl@yxYyNhm1WTDd6WhHJXG*OQ#BRg>;DX15j2?>nLR=aE5e?ET6o z_2z{ueYeT1eD1%aXBu;7=0yK>UvI|nFxUjdzt0!X4D;hsDf|?9G=1^5$rUqyY|6N` zn6u9;J6tnuQT!gaukt58F`xRn_S_=*)ka4?zB(YX+EOF%W0;AjpEfg#&h34>-mKtT zd2vtA-92nJdJWzpk?KLQmTQD%UKJi)yH<1guFcXHHhfzuS7m=9@Kj*7eXacSpO>#S z?vvjZx^K;CNr#xKu&Q(?O;{7J@3zD4RJe+}c4Po~d#AGSTLdXfHRA(M_kyp(`^ zk8GTnEqBSk=bui!n)bZq<%9P>e_UbW;Pw)jBzTk0oKupi^7q_t)4tsOdi|&T|2~%K z9O7JE9HA_U{{p-jnYb8m_d { + 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); + t.end(); + process.nextTick(process.exit); + }); +}); From 9f517bd4832dd03dbacbdbd48045f29582a36c9d Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Tue, 26 Jun 2018 16:41:10 -0400 Subject: [PATCH 2/4] Deal with comments attached to undefined blocks Also fix an issue where you couldn't save/load projects that had multiple comments linked to a single block. --- src/serialization/sb2.js | 47 ++++++++++++++++++----------- test/fixtures/unknown-opcode.sb2 | Bin 3949 -> 4009 bytes test/integration/unknown-opcode.js | 18 +++++++++++ 3 files changed, 47 insertions(+), 18 deletions(-) diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 5ce617c2e..d9dced0f7 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -134,19 +134,17 @@ const parseBlockList = function (blockList, addBroadcastMsg, getVariableId, exte const parsedBlockAndComments = parseBlock(block, addBroadcastMsg, getVariableId, extensions, comments, commentIndex); - if (parsedBlockAndComments) { // Could fail due to unrecognized op-codes - const parsedBlock = parsedBlockAndComments[0]; - // Update commentIndex - commentIndex = parsedBlockAndComments[1]; + const parsedBlock = parsedBlockAndComments[0]; + // Update commentIndex + commentIndex = parsedBlockAndComments[1]; - if (typeof parsedBlock === 'undefined') continue; - if (previousBlock) { - parsedBlock.parent = previousBlock.id; - previousBlock.next = parsedBlock.id; - } - previousBlock = parsedBlock; - resultingList.push(parsedBlock); + if (!parsedBlock) continue; + if (previousBlock) { + parsedBlock.parent = previousBlock.id; + previousBlock.next = parsedBlock.id; } + previousBlock = parsedBlock; + resultingList.push(parsedBlock); } return [resultingList, commentIndex]; }; @@ -709,9 +707,19 @@ 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 comment id and + // send all linked comments back 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]; @@ -734,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... - // not sure what to do here.. concatenate all the messages in all the comments and only - // keep one around? + // Attach only the last comment to the block, make all others workspace comments 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++; diff --git a/test/fixtures/unknown-opcode.sb2 b/test/fixtures/unknown-opcode.sb2 index 81e191b46ba03a01c321b289419237b07155e655..cc3341aa54349145058f98fa14dd91332eaca590 100644 GIT binary patch delta 744 zcmaDWw^F`7z?+$ci-CcGgF(0ckk8}g;rHd37#KuY7#Mhfq6J0yS*gh-dRfK!dAY&2 z`)(Ns)V8hf_#>kjUv$@;t2Sa~$!ew0=!JGZLeiUjADuAF319v9d*9=NOxBKs`5!Hd z(`+i|pYPC%+vlWx(%E5~*tbb79DGYxR$D9VxP8=9VoANy5z&{@6}Mj{hucmtP}uwK zr2Y}kul41Ne(XH8w&eDyyr9&kHpjk8I>%U-{|LCiBgCk(xFe)gef5GbDvwIitf#qU zsO>tu$WC>)dxFmH!w=>iT+j3VYN$a~jeBEe{ksX;T6+%lXS1wizok{CZp2tQYiVeR zSf`NdERHyr(4{F0>o0kzynH0;m(H2-+ihLI38l%$XSVZ3Oz$>4mE5n98�baq#c) zjLlb?ZMS_o_M&CZTCsqC>v+miPK6y%*neQQ$>Pj!@;f5?-vv(;-}vN!vfyeLqsUJ- z;+f~VI=IDDyZ7Cm|26Bj)Bjk$U6!|^%EJQ}>sq9=`c8dz^Qzdz>V@;``zBw^_H|D- z*zl><<`K`InZLZNZr=UXbGoNdptDIyJ7tbjXwXDwwmkxiTK>HGd}#L_k&T<)eqEj~ zeMeonV3qdD>Av+0@vse0hW>G`p)fdTI*5oB<>

@AU{`Zx?)jwXCeJf5a^NndBFHgJl)U!pc#$rK#jOO`OF8f~e=xnU~XKEo#??g31726_c~=>gu1Ok510 uBndKR@_#-VlPGou2187_Lq79AwyrY(X#`?cunriJJK3LKg6#kwhz|g*=r=6@ delta 680 zcmZ1}|5mO(z?+$ci-CcGgQ2A2kk6`LUrsz?WMIf(W?=j8^+ z=Fc_|sr9R4{bbyA;MwN<$W85Dr>%4^U0HeSu<15os{_+=FNKEw|D7W|DMyRrpzZh9 zbE?nYE^1V%{l{hNUM_em+M+&}$YSUTPjL3`wcq_}gh!^Yy(l>}BnbI5byY z&~IOyQdRq)G+g8P#elcDJ0s@2-MN~-XZ?r2*BVcJfSRiq)@}^F`=!^K}zSXIiXRi)jA3_mauMpDi3Kt3McWSpK%Xkh5fC^7;IZ zc@t-9wLUYy9JKU#o=82@?3Z5FKgg55F@4E0%i|5kle(Lh?>SiNd&+95q*`X! znkxk}UT@vT{&lYO&E9QO4{u#DP0VJ~!{(>IE@q#e)E{GC>o+yVv^UE7c$!d=y}`fd zHt|b-6r48BUnRn!$#S9V(;k*ecOPjI!o<$zz|H5_g1u-Pt9Qb2lkPs;6*enoh{o#di_S!4K8!mmCX>OL` zByoLebyvo4q*~t zWdhP^?1Esrk|&sn4>g@kzQv~lR5F=^Uq+mViGjgDuOKfyz?+eYivg6ZK*mhA2Fl5? lGcXupk;~(kkv9Y;H3Pl!#IgWyRyL3x79hL~q+9qvJOK3JAz}ak diff --git a/test/integration/unknown-opcode.js b/test/integration/unknown-opcode.js index d7a5519f6..53b9b64f9 100644 --- a/test/integration/unknown-opcode.js +++ b/test/integration/unknown-opcode.js @@ -31,6 +31,24 @@ test('unknown opcode', t => { 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); }); From 4967155285c5f0a12990a9d48516dd5c952f1252 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Wed, 27 Jun 2018 10:44:28 -0400 Subject: [PATCH 3/4] Fix nits --- src/serialization/sb2.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index d9dced0f7..5e638be00 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -133,7 +133,6 @@ const parseBlockList = function (blockList, addBroadcastMsg, getVariableId, exte // eslint-disable-next-line no-use-before-define const parsedBlockAndComments = parseBlock(block, addBroadcastMsg, getVariableId, extensions, comments, commentIndex); - const parsedBlock = parsedBlockAndComments[0]; // Update commentIndex commentIndex = parsedBlockAndComments[1]; @@ -711,8 +710,9 @@ const parseBlock = function (sb2block, addBroadcastMsg, getVariableId, extension comments[commentIndex] : null; const blockMetadata = specMapBlock(sb2block); if (!blockMetadata) { - // No block opcode found, exclude this block, increment the comment id and - // send all linked comments back to zero/zero to prevent serialization issues. + // No block opcode found, exclude this block, increment the comment id, + // 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; From 0e3413e6bdd6802bf2a70e77c77d40e822d76c34 Mon Sep 17 00:00:00 2001 From: Paul Kaplan Date: Wed, 27 Jun 2018 14:17:33 -0400 Subject: [PATCH 4/4] Fix comment --- src/serialization/sb2.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/serialization/sb2.js b/src/serialization/sb2.js index 5e638be00..460921a5f 100644 --- a/src/serialization/sb2.js +++ b/src/serialization/sb2.js @@ -710,7 +710,7 @@ const parseBlock = function (sb2block, addBroadcastMsg, getVariableId, extension comments[commentIndex] : null; const blockMetadata = specMapBlock(sb2block); if (!blockMetadata) { - // No block opcode found, exclude this block, increment the comment id, + // 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) {