fix: disposing of a workspace that has overwritten shadows (#6424)

* fix: disposing of a workspace that has overwritten shadows

* fix: try slightly different placement to fix tests

* fix: add disposing parameter to guaruntee consistent behavior

* chore: wrap properties in a isDeadOrDying method

* chore: make disposing private

core/block.ts

@@ -180,6 +180,11 @@ export class Block implements IASTNodeLocation, IDeletable {
   protected collapsed_ = false;
   protected outputShape_: number|null = null;
 
+  /**
+   * Is the current block currently in the process of being disposed?
+   */
+  private disposing = false;
+
   /**
    * A string representing the comment attached to this block.
    *
@@ -316,7 +321,7 @@ export class Block implements IASTNodeLocation, IDeletable {
    * @suppress {checkTypes}
    */
   dispose(healStack: boolean) {
-    if (this.disposed) {
+    if (this.isDeadOrDying()) {
       return;
     }
 
@@ -338,6 +343,7 @@ export class Block implements IASTNodeLocation, IDeletable {
       this.workspace.removeTypedBlock(this);
       // Remove from block database.
       this.workspace.removeBlockById(this.id);
+      this.disposing = true;
 
       // First, dispose of all my children.
       for (let i = this.childBlocks_.length - 1; i >= 0; i--) {
@@ -360,6 +366,16 @@ export class Block implements IASTNodeLocation, IDeletable {
     }
   }
 
+  /**
+   * Returns true if the block is either in the process of being disposed, or
+   * is disposed.
+   *
+   * @internal
+   */
+  isDeadOrDying(): boolean {
+    return this.disposing || this.disposed;
+  }
+
   /**
    * Call initModel on all fields on the block.
    * May be called more than once.
@@ -772,7 +788,7 @@ export class Block implements IASTNodeLocation, IDeletable {
    * @returns True if deletable.
    */
   isDeletable(): boolean {
-    return this.deletable_ && !this.isShadow_ && !this.disposed &&
+    return this.deletable_ && !this.isShadow_ && !this.isDeadOrDying() &&
         !this.workspace.options.readOnly;
   }
 
@@ -791,7 +807,7 @@ export class Block implements IASTNodeLocation, IDeletable {
    * @returns True if movable.
    */
   isMovable(): boolean {
-    return this.movable_ && !this.isShadow_ && !this.disposed &&
+    return this.movable_ && !this.isShadow_ && !this.isDeadOrDying() &&
         !this.workspace.options.readOnly;
   }
 
@@ -865,7 +881,8 @@ export class Block implements IASTNodeLocation, IDeletable {
    * @returns True if editable.
    */
   isEditable(): boolean {
-    return this.editable_ && !this.disposed && !this.workspace.options.readOnly;
+    return this.editable_ && !this.isDeadOrDying() &&
+        !this.workspace.options.readOnly;
   }
 
   /**

core/block_svg.ts

@@ -525,7 +525,7 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg,
 
   /** Snap this block to the nearest grid point. */
   snapToGrid() {
-    if (this.disposed) {
+    if (this.isDeadOrDying()) {
       return;  // Deleted block.
     }
     if (this.workspace.isDragging()) {
@@ -861,8 +861,7 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg,
    * @suppress {checkTypes}
    */
   override dispose(healStack?: boolean, animate?: boolean) {
-    if (this.disposed) {
-      // The block has already been deleted.
+    if (this.isDeadOrDying()) {
       return;
     }
     Tooltip.dispose();
@@ -1067,13 +1066,12 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg,
     if (this.workspace.isDragging()) {
       // Don't change the warning text during a drag.
       // Wait until the drag finishes.
-      this.warningTextDb.set(
-          id, setTimeout(() => {
-            if (!this.disposed) {  // Check block wasn't deleted.
-              this.warningTextDb.delete(id);
-              this.setWarningText(text, id);
-            }
-          }, 100));
+      this.warningTextDb.set(id, setTimeout(() => {
+                               if (!this.isDeadOrDying()) {
+                                 this.warningTextDb.delete(id);
+                                 this.setWarningText(text, id);
+                               }
+                             }, 100));
       return;
     }
     if (this.isInFlyout) {
@@ -1541,11 +1539,11 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg,
    * connected should not coincidentally line up on screen.
    */
   override bumpNeighbours() {
-    if (this.disposed) {
-      return;  // Deleted block.
+    if (this.isDeadOrDying()) {
+      return;
     }
     if (this.workspace.isDragging()) {
-      return;  // Don't bump blocks during a drag.
+      return;
     }
     const rootBlock = this.getRootBlock();
     if (rootBlock.isInFlyout) {

core/connection.ts

@@ -571,7 +571,7 @@ export class Connection implements IASTNodeLocationWithBlock {
     const parentBlock = this.getSourceBlock();
     const shadowState = this.getShadowState();
     const shadowDom = this.getShadowDom();
-    if (parentBlock.disposed || !shadowState && !shadowDom) {
+    if (parentBlock.isDeadOrDying() || !shadowState && !shadowDom) {
       return null;
     }
 

core/contextmenu_items.ts

@@ -249,7 +249,7 @@ function deleteNext_(deleteList: BlockSvg[], eventGroup: string) {
   eventUtils.setGroup(eventGroup);
   const block = deleteList.shift();
   if (block) {
-    if (!block.disposed) {
+    if (!block.isDeadOrDying()) {
       block.dispose(false, true);
       setTimeout(deleteNext_, DELAY, deleteList, eventGroup);
     } else {

core/field.ts

@@ -258,7 +258,8 @@ export abstract class Field implements IASTNodeLocationSvg,
    * @returns The renderer constant provider.
    */
   getConstants(): ConstantProvider|null {
-    if (!this.constants_ && this.sourceBlock_ && !this.sourceBlock_.disposed &&
+    if (!this.constants_ && this.sourceBlock_ &&
+        !this.sourceBlock_.isDeadOrDying() &&
         this.sourceBlock_.workspace.rendered) {
       this.constants_ = (this.sourceBlock_.workspace as WorkspaceSvg)
                             .getRenderer()
@@ -1050,7 +1051,7 @@ export abstract class Field implements IASTNodeLocationSvg,
    * @param e Mouse down event.
    */
   protected onMouseDown_(e: Event) {
-    if (!this.sourceBlock_ || this.sourceBlock_.disposed) {
+    if (!this.sourceBlock_ || this.sourceBlock_.isDeadOrDying()) {
       return;
     }
     const gesture = (this.sourceBlock_.workspace as WorkspaceSvg).getGesture(e);

core/field_variable.ts

@@ -377,7 +377,7 @@ export class FieldVariable extends FieldDropdown {
     let variableTypes = this.variableTypes;
     if (variableTypes === null) {
       // If variableTypes is null, return all variable types.
-      if (this.sourceBlock_ && !this.sourceBlock_.disposed) {
+      if (this.sourceBlock_ && !this.sourceBlock_.isDeadOrDying()) {
         return this.sourceBlock_.workspace.getVariableTypes();
       }
     }
@@ -456,7 +456,7 @@ export class FieldVariable extends FieldDropdown {
   protected override onItemSelected_(menu: Menu, menuItem: MenuItem) {
     const id = menuItem.getValue();
     // Handle special cases.
-    if (this.sourceBlock_ && !this.sourceBlock_.disposed) {
+    if (this.sourceBlock_ && !this.sourceBlock_.isDeadOrDying()) {
       if (id === internalConstants.RENAME_VARIABLE_ID) {
         // Rename variable.
         Variables.renameVariable(
@@ -515,7 +515,7 @@ export class FieldVariable extends FieldDropdown {
     }
     const name = this.getText();
     let variableModelList: AnyDuringMigration[] = [];
-    if (this.sourceBlock_ && !this.sourceBlock_.disposed) {
+    if (this.sourceBlock_ && !this.sourceBlock_.isDeadOrDying()) {
       const variableTypes = this.getVariableTypes_();
       // Get a copy of the list, so that adding rename and new variable options
       // doesn't modify the workspace's list.

core/keyboard_nav/ast_node.ts

@@ -275,7 +275,7 @@ export class ASTNode {
     // TODO(#6097): Use instanceof checks to exit early for values of
     // curLocation that don't make sense.
     const curLocationAsBlock = curLocation as Block;
-    if (!curLocationAsBlock || curLocationAsBlock.disposed) {
+    if (!curLocationAsBlock || curLocationAsBlock.isDeadOrDying()) {
       return null;
     }
     const curRoot = curLocationAsBlock.getRootBlock();

tests/mocha/event_test.js

@@ -997,10 +997,6 @@ suite('Events', function() {
             this.eventsFireSpy, 0, Blockly.Events.BlockDelete,
             {oldXml: expectedOldXml, group: ''},
             workspaceSvg.id, expectedId);
-        assertNthCallEventArgEquals(
-            changeListenerSpy, 0, Blockly.Events.BlockDelete,
-            {oldXml: expectedOldXml, group: ''},
-            workspaceSvg.id, expectedId);
 
         // Expect the workspace to not have a variable with ID 'test_block_id'.
         chai.assert.isNull(this.workspace.getVariableById(TEST_BLOCK_ID));

tests/mocha/test_helpers/block_definitions.js

@@ -167,6 +167,7 @@ export function createTestBlock() {
     },
     'renameVarById': Blockly.Block.prototype.renameVarById,
     'updateVarName': Blockly.Block.prototype.updateVarName,
+    'isDeadOrDying': () => false,
   };
 }