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

Crash if a field click deletes a block #7587

Closed
1 task
maribethb opened this issue Oct 11, 2023 · 8 comments · Fixed by #7621
Closed
1 task

Crash if a field click deletes a block #7587

maribethb opened this issue Oct 11, 2023 · 8 comments · Fixed by #7621
Assignees
Labels
good first issue issue: bug Describes why the code or behaviour is wrong size: small Bugs that can be picked up and completed in 1-3 days

Comments

@maribethb
Copy link
Contributor

maribethb commented Oct 11, 2023

Check for duplicates

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

Description

If you have a block with a field that deletes the block*, Blockly will crash because the gesture handling code assumes the block continues to exist.

We can fix this in several ways:

  1. Move bringBlockToFront to the beginning of doFieldClick (so that it happens before the field has a chance to handle the click)
  2. Have bringBlockToFront make sure disposed is false on the block before calling the bringToFront method. This would prevent trying to move a dead/dying block.

Reproduction steps

  1. Add the following block definition to the playground above the start call.
Blockly.Blocks['test'] = {
        init: function() {
          this.setStyle('math_blocks');
          this.appendDummyInput()
              .appendField(new Blockly.FieldImage(
                  'media/arrow.png',
                  30,
                  30,
                  'test',
                  function() {
                    this.getSourceBlock().dispose();
                  }
              ));
        }
  1. Run npm run start to start the playground.
  2. Load the following JSON into the playground:
{
  "blocks": {
    "languageVersion": 0,
    "blocks": [
      {
        "type": "test"
      }
    ]
  }
}
  1. Open the browser.
  2. Click the arrow on the block.
  3. Observe the error in the console.

Stack trace

Uncaught TypeError: Cannot read properties of null (reading 'childNodes')
    at BlockSvg$$module$build$src$core$block_svg.bringToFront (block_svg.ts:1227:34)
    at Gesture$$module$build$src$core$gesture.bringBlockToFront_ (gesture.ts:940:25)
    at Gesture$$module$build$src$core$gesture.doFieldClick_ (gesture.ts:880:10)
    at Gesture$$module$build$src$core$gesture.handleUp (gesture.ts:591:14)
    at HTMLDocument.g (browser_events.ts:70:9)

Additional Info

This should also be fixed for icons since, #7588 is fixed.

To Fix

  1. Run the reproduction steps above to make sure you can reproduce the issue.
  2. Add a check in bringToFront that checks if this.isDeadOrDying(), and returns early if so.
  3. Run the reproduction steps again to make sure the issue doesn't reproduce.
@maribethb maribethb added issue: bug Describes why the code or behaviour is wrong issue: triage Issues awaiting triage by a Blockly team member size: small Bugs that can be picked up and completed in 1-3 days labels Oct 11, 2023
@rachel-fenichel rachel-fenichel removed the issue: triage Issues awaiting triage by a Blockly team member label Oct 18, 2023
@Apoorvgarg-creator
Copy link
Contributor

Interested to solve the issue

@maribethb
Copy link
Contributor Author

Great, I'll assign you @Apoorvgarg-creator ! If you have any questions please feel free to ask here.

@Apoorvgarg-creator
Copy link
Contributor

Great, I'll assign you @Apoorvgarg-creator ! If you have any questions please feel free to ask here.

Thank you !

@Apoorvgarg-creator
Copy link
Contributor

@maribethb I am working on this issue, I cloned the repo and ran npm i after that I want to build and reproduce the issue, can you please help me !

@Apoorvgarg-creator
Copy link
Contributor

Getting this on running npm run start

Screenshot 2023-10-28 at 10 06 22 PM

@BeksOmega
Copy link
Collaborator

Getting this on running npm run start

Looks like you're probably using an old version of node. Can you upgrade to Node v20 @Apoorvgarg-creator and see if that fixes the issue?

@Apoorvgarg-creator
Copy link
Contributor

Apoorvgarg-creator commented Oct 31, 2023

@BeksOmega Thank you, It worked !

I was able to reproduce the error. Solving the issue. Thank you !!

Apoorvgarg-creator added a commit to Apoorvgarg-creator/blockly that referenced this issue Oct 31, 2023
@Apoorvgarg-creator
Copy link
Contributor

Apoorvgarg-creator commented Oct 31, 2023

@maribethb @BeksOmega I have solved the bug with the solution proposed in the issue.
But I wanted to know if this solution is also logically correct or not -
if I add parent !== null check before accessing properties of parent ?

BeksOmega pushed a commit that referenced this issue Nov 1, 2023
* Fix: #7587

* Fix: Lint error

* Fix: Move expression out of loop

* Fix: No need to use temp variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue issue: bug Describes why the code or behaviour is wrong size: small Bugs that can be picked up and completed in 1-3 days
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants