[block-shareable-procedures] Toolbox breaks after deleting functions

[mikeharv]

Check for duplicates

• I have searched for similar issues before opening a new one.

Component

The block-shareable-procedures plugin has a bug where the dynamic Functions category cannot be opened.

Description

The bug happens when a call block is deleted automatically alongside its definition from the workspace. The error seems to suggest that Blockly has attempted to delete the same call block twice during clearOldBlocks(). When the definition block is disposed, this the call block below it to delete itself via doProcedureUpdate.

This is impacting all of Code.org's Blockly labs, even those that just use a single workspace. A local workaround seems possible. By overriding doProcedureUpdate, I was able to check !this.workspace.isFlyout before allowing the this.dispose() call to go through. I'm not sure whether this is the best solution or not, but it does unblock us for now.

Reproduction steps

1. Go to https://google.github.io/blockly-samples/plugins/block-shareable-procedures/test/index
2. Add a function definition and a function call to either workspace.
3. Delete the function definition
4. Attempt to re-open the Functions category of the toolbox

The flyout is not populated and an error is thrown:

Uncaught Error: Block not present in workspace's list of top-most blocks.
    at WorkspaceSvg$$module$build$src$core$workspace_svg.removeTopBlock

Stack trace

blockly_compressed.js:1126 Uncaught Error: Block not present in workspace's list of top-most blocks.
    at WorkspaceSvg$$module$build$src$core$workspace_svg.removeTopBlock (blockly_compressed.js:1126:10)
    at WorkspaceSvg$$module$build$src$core$workspace_svg.removeTopBlock (blockly_compressed.js:1181:339)
    at BlockSvg$$module$build$src$core$block_svg.dispose (blockly_compressed.js:973:156)
    at BlockSvg$$module$build$src$core$block_svg.dispose (blockly_compressed.js:1234:7)
    at VerticalFlyout$$module$build$src$core$flyout_vertical.clearOldBlocks (blockly_compressed.js:1415:401)
    at VerticalFlyout$$module$build$src$core$flyout_vertical.show (blockly_compressed.js:1409:481)
    at Toolbox$$module$build$src$core$toolbox$toolbox.updateFlyout_ (blockly_compressed.js:1627:387)
    at Toolbox$$module$build$src$core$toolbox$toolbox.setSelectedItem (blockly_compressed.js:1626:103)
    at Toolbox$$module$build$src$core$toolbox$toolbox.onClick_ (blockly_compressed.js:1615:97)
    at HTMLDivElement.f (blockly_compressed.js:85:149)

Screenshots

[johnnesky]

I'm doing some investigation on this. In the plugin, the procedureCallerUpdateShapeMixin's method doProcedureUpdate checks to see if the corresponding procedure id still exists in the procedure map, and if not, it calls dispose() on itself. If this dispose is commented out, then the error is not thrown. (But then the call blocks won't be deleted in any workspace.)

Another way to prevent the error from being thrown is to make sure that the most recently opened toolbox flyout is anything other than the procedures flyout. The flyout remembers whichever category was most recently opened, even though the first action it takes upon being revealed is to call clearOldBlocks() on itself, which disposes of all of the blocks in the flyout's workspace, and this is where the error is thrown. If the most recent flyout is for the procedures category, then something happens to the call block in this flyout's hidden workspace when the procedure is deleted (it's disposed like in any other workspace with shared procedures), that silently puts the flyout into a bad state but it doesn't throw the error until clearOldBlocks() tries to dispose the block again.

Notably, when adding or deleting procedures, if a different blockly injected workspace has the procedure flyout open at the same time, that flyout will not be immediately updated to show the added or removed call blocks. However, if you hide and show the second injected workspace's procedures flyout, then it will be updated, and if a procedure had been deleted then it will throw the error at this point. In general, the flyout is not rerendered in response to procedure changes, regardless of whether it's currently visible or not.