Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix exception when disposing of a workspace with a variable block obscuring a shadow block. #8619

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Oct 11, 2024

The basics

The details

Resolves

Fixes gonfunko/scratch-blocks#210

Proposed Changes

This PR fixes a tricky bug related to workspace disposal. When a WorkspaceSvg instance A is disposed:

  1. It sets this.rendered = false
  2. It disposes of its flyout
  3. The flyout disposes of its flyout workspace B
  4. The flyout workspace B, as part of disposing itself, clears its variable map, which is the same as the variable map of workspace A, because when a flyout is created the parent workspace shares its variable map with the flyout
  5. When the variable map clears itself, it calls VariableMap.deleteVariable for each variable it contains
  6. VariableMap.deleteVariable calls VariableMap.getVariableUsesById to identify references to the variable
  7. VariableMap.getVariableUsesById calls Variables.getVariableUsesById, passing its workspace, which, because this variable map was shared with the flyout by Workspace A, is Workspace A, which is in the process of being torn down
  8. Variables.getVariableUsesById retrieves the blocks of Workspace A, which in this (mildly) contrived example includes a variable block obscuring a shadow block
  9. VariableMap.deleteVariable calls dispose on the variable block obscuring a shadow block on Workspace A, in order to clean up references to the being-deleted variable
  10. Because the variable block was obscuring a shadow block, the connections attempt to create a new shadow block to restore it
  11. When the respawned shadow block gets created on workspace A, which is being disposed, the BlockSvg constructor checks to ensure that its workspace is rendered (to prevent creating a BlockSvg on a regular Workspace), but because the workspace is being disposed, rendered is false, so it throws
  12. @gonfunko spends the better part of two days tracking this down

This PR only clears the variable map when the workspace being disposed is not a flyout workspace, which resolves the issue.

@gonfunko gonfunko requested a review from a team as a code owner October 11, 2024 21:15
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Oct 11, 2024
@gonfunko gonfunko marked this pull request as draft October 11, 2024 21:19
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Oct 11, 2024
@gonfunko gonfunko marked this pull request as ready for review October 11, 2024 21:33
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a wonderfully simple fix for a complex problem. Thanks for the write-up.

@gonfunko gonfunko merged commit 089179b into google:rc/v12.0.0 Oct 15, 2024
8 checks passed
@gonfunko gonfunko deleted the workspace-disposal branch October 15, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants