search

scratchfoundation / scratch-vm

Virtual Machine used to represent, run, and maintain the state of programs for Scratch 3.0
http://scratchfoundation.github.io/scratch-vm/
BSD 3-Clause "New" or "Revised" License
1.21k stars 1.51k forks source link

Creating and deleting a clone in the same When I start as a clone stack crashes Scratch #2282

Open BryceLTaylor opened 5 years ago

BryceLTaylor commented 5 years ago

Expected Behavior

If you build this stack: Screen Shot 2019-09-23 at 11 40 22 AM And click the green flag it should not crash Scratch.

Actual Behavior

Scratch freezes and nothing you click is responsive. After a minute or so it pops up with a "page not responsive" dialog: Screen Shot 2019-09-23 at 11 46 53 AM It may require attempting to interact with Scratch to get the dialog.

Steps to Reproduce

Build this stack: when I start as a clone create a clone of myself delete this clone

Run a 'create a clone of myself' block Note: Scratch becomes unresponsive and after a minute a dialog appears.

Interestingly, if you put the create a clone of myself block in a forever loop and then have the stack when I start as a clone delete this clone the application does not crash. The create and delete blocks must be in the same stack.
Screen Shot 2019-09-23 at 11 51 55 AM

Similarly if you build two stacks one that is When I start as a clone create a clone of myself and a second stack that is When I start as a clone delete this clone it doesn't crash either. Further evidence that the two must be in the same stack.

Screen Shot 2019-09-23 at 11 53 41 AM

Here is my repro project: https://scratch.mit.edu/projects/330894575/editor It is a simplified version of one that came in from the community: https://scratch.mit.edu/projects/314578144

Operating System and Browser

Mac Chrome

apple502j commented 5 years ago

This is a known bug since 2.0 😜 Aaaand why did you remove [help wanted]?

apple502j commented 5 years ago

My thoughts on it:

1) Clone 1 (for short, C1) produced 2) C1 makes C2 3) C1 dies and C2 makes C3 4) C2 dies and C3 makes C4 ... (causes high CPU usage)

Kenny2github commented 5 years ago

Interestingly, if you put the create a clone of myself block in a forever loop and then have the stack when I start as a clone delete this clone the application does not crash.

Well of course, a forever loop has a 1/FPS delay, giving plenty of time for the clone to delete itself. What is interesting is that: image image doesn't crash it, suggesting that, indeed

The create and delete blocks must be in the same stack.

AmazingMech2418 commented 3 years ago

I'll have to investigate where tomorrow probably, but I think the issue here is an infinite loop. Probably somewhere in either sequencer or runtime, but I still have to investigate that. It seems like what's happening is that every clone is being created and destroyed in one single iteration of the sequencer, but finding the actual place in the code where this happens is a bit trickier, especially since this would be my first time looking at the sequencer code.

AmazingMech2418 commented 3 years ago

It clearly isn't this loop that's running the infinite loop: https://github.com/LLK/scratch-vm/blob/607cd2d392b0d16eb14e3512de9ac6741f2dfe3d/src/engine/sequencer.js#L88-L91

Here, the timer should take over and prevent a crash, like with run without screen refresh forever loops.

It could potentially be this thread: https://github.com/LLK/scratch-vm/blob/607cd2d392b0d16eb14e3512de9ac6741f2dfe3d/src/engine/sequencer.js#L103

If this line of code were activated every iteration for some reason, it could certainly cause an infinite loop: https://github.com/LLK/scratch-vm/blob/607cd2d392b0d16eb14e3512de9ac6741f2dfe3d/src/engine/sequencer.js#L130-L132

One thing I just noticed too is that the clone automatically starts the "when I start as clone" hat block when executed, and doesn't wait until the next thread. This may be our problem. The only thing though is, why does it delete the clones before the next iteration? It seems like both the creating the clone and deleting the clone are for some reason executed at the same time. https://github.com/LLK/scratch-vm/blob/607cd2d392b0d16eb14e3512de9ac6741f2dfe3d/src/sprites/rendered-target.js#L973 https://github.com/LLK/scratch-vm/blob/f405e59d01a8f9c0e3e986fb5276667a8a3c7d40/src/sprites/rendered-target.js#L170-L180

Also, when the layering is edited, so is the execution order. I'm not sure if this does anything yet though... https://github.com/LLK/scratch-vm/blob/607cd2d392b0d16eb14e3512de9ac6741f2dfe3d/src/sprites/rendered-target.js#L897-L906

Does anyone know of any reason why both the creation of clones and deletion of clones in these scripts would be run in the same iteration? It does follow what @apple502j guessed, but for some reason does all of this in one iteration, creating an infinite loop that hangs the VM, and the editor.

Also, another interesting thing that proves these are run in the same iteration is, if you put a wait 0 seconds block in between the creation and deletion of the clones, it doesn't crash.

One thing though is for sure: this bug is in the sequencer or runtime.

AmazingMech2418 commented 3 years ago

By the way, if you use a loop 1 time block around the create clone thing, it also doesn't crash. It seems like any control structure will keep it from crashing.

Edit: The reason for this is that the control structure in stepThread will cause the block to not be executed yet.

AmazingMech2418 commented 3 years ago

The strange thing with this though, is that all scripts are run at once as long as there isn't a control structure. And it's only when the clones stop being created that everything stops.

AmazingMech2418 commented 3 years ago

I think this is where the issue is: https://github.com/LLK/scratch-vm/blob/607cd2d392b0d16eb14e3512de9ac6741f2dfe3d/src/engine/sequencer.js#L192

While this is great for optimization, with clones, it causes a phenomena sort of similar to infinite tail-end recursion, creating an infinite loop and hanging the editor.

I believe this code essentially speeds things up so that all scripts that don't involve control structures will run instantly, and therefore optimizing runtime performance. However, with clones, this is what happens:

  1. Clone 1 is created and thread 1 starts
  2. Thread 1 has no control structures, so clone 2 is created and clone 1 is deleted, both instantly
  3. Thread 1 is complete, so now thread 2 is run for clone 2, based on the sequencer
  4. Thread 2 also has no control structures, so clone 3 is created and clone 2 is deleted, both instantly
  5. ...

This all occurs without exiting the sequencer stepThreads function.

The reason not deleting the clone will not cause this issue is because the 301st clone won't be created due to clone limits, but deleting the clones immediately will mean that it never reaches the clone limit, and creates this infinite loop.

This can be fixed in two ways:

  1. Treat create clone creation as a control structure (this does create a performance issue though)
  2. Wait until next iteration to start clone threads (could possibly be done through an event (especially since the runtime extends EventEmitter) or a stack of hat blocks (in an array) to be fired on the end of the iteration)

However, whether or not this should be fixed is also a valid question. While it can crash the tab and does hang the editor, this can be useful, if implemented correctly, in that it could dramatically increase performance by replacing normal iterative methods with tail-end recursion using this method, running at the same speed as native JavaScript, even with loops that might hang the editor more than is allowed by the "warp timer". However, this benefit likely does not outweigh the risks of hanging the editor, given that very few applications will need to run faster than allowed by "run without screen refresh".

@ericrosenbaum What do you think of this? I would be willing to submit a PR, but it would also probably be best to discuss which solution option (if any) to use. I personally think solution 2 would be the best since it doesn't restrict performance.