fix: disposing of a workspace that has overwritten shadows

[BeksOmega]

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide
• I ran npm run format and npm run lint

The details

Resolves

Part of #6358

Proposed Changes

Change where we set block.disposed to true. I audited all of the places where disposed is referenced, and I don't think this will break anything. But let's be honest, it probably will break something.

Behavior Before Change

If you disposed of a workspace that had an overwritten shadow block in it, the workspace would throw an error. See #6358 (comment)

No end-user-facing change in behavior.

Behavior After Change

No Error!

No end-user-facing change in behavior.

Reason for Changes

Errors are sad =(

Test Coverage

Manually tested that disposing of a workspace with the following blocks in it did not cause an error to be thrown:

{
  "blocks": {
    "languageVersion": 0,
    "blocks": [
      {
        "type": "math_single",
        "id": "f#E]:l@J;BJ;N(?;KQ5T",
        "x": 138,
        "y": 138,
        "fields": {
          "OP": "ROOT"
        },
        "inputs": {
          "NUM": {
            "shadow": {
              "type": "math_number",
              "id": "wr:jE3$@EZ(/{pNSq92C",
              "fields": {
                "NUM": 9
              }
            },
            "block": {
              "type": "math_number",
              "id": "5A]oz4Gd$y]!BUkH/i~@",
              "fields": {
                "NUM": 123
              }
            }
          }
        }
      }
    ]
  }
}

Documentation

N/A

Additional Information

Caused by https://github.com/google/blockly/pull/6300/files#diff-dd85563e074ebb74e4c34eee01568971430f3cb7a6b846feef8044ad8d5dcd61L566

The issue is that workspace used to be set to null before the blocks were disposed, so that would properly cause createShadowBlock_ to short circuit. But because createShadowBlock_ was now looking at disposed it wasn't short circuiting, because disposed was set after the blocks were disposed.

[BeksOmega]

I think it might actually be safest to add an internal disposing property, and replace all the places where we used to be checking block.workspace == null with that, so that we know the new behavior is 100% consistent.

But on the other hand, I hate using state to define functional logic.

So Imma sleep on it.

[gonfunko]

Do you know where the exception this try/catch is wrapping is thrown? Do we still need this here in case it throws prior to disposed being set to true in the try?

[BeksOmega]

Ha no. Back in 2016 a contributor just wrapped everything between an Events.disable() and an Events.enable() in a try...finally to make sure that events were always re-enabled: #348