Skip to content

Add handleLoopError, an escape hatch for requestAnimationFrame crashes#819

Open
guilhermesimoes wants to merge 1 commit into
lightning-js:mainfrom
guilhermesimoes:feat/protect-raf
Open

Add handleLoopError, an escape hatch for requestAnimationFrame crashes#819
guilhermesimoes wants to merge 1 commit into
lightning-js:mainfrom
guilhermesimoes:feat/protect-raf

Conversation

@guilhermesimoes

@guilhermesimoes guilhermesimoes commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

It's easier to review this PR with whitespace turned off: https://github.com/lightning-js/renderer/pull/819/changes?w=1

Long story short: client code is buggy and can crash Lightning's render loop. When that happens, the raf stops running and effectively the client app is frozen.

Some crashes we've had on Peacock are along the lines of:

animation.on('finish', () => {
  this.foo.x();
    // ^ oops, `foo` does not exist at this point, TypeError: Cannot read property 'x' of undefined
});

This crashes the raf since this code runs synchronously.
In L2, the stack trace is: raf → stage.updateFrame() → emit('frameStart') → AnimationManager.progress() → client code
In L3, the stack trace is: raf → stage.updateAnimations() → AnimationManager.update() → client code

@wouterlucas

Copy link
Copy Markdown
Contributor

can we split this into at least 2 separate PRs to avoid mixing things together:
PR 1 - the reuse platform settings and simplification of platform.ts - those are really nice and easy to merge
PR 2 - the try/catch pattern with handleLoopError

I agree the handleLoopError is a genuine problem we need to address but I'd like to bounce some ideas to see what the impact of this is. My first pass of comments on this:

  • The loop does not seem to reschedule after an error, I guess that's intended?
  • The impact of a try/catch block

The latter requires a bit of a broader discussion, most modern post 2018+ browsers are fine and code within a try/catch block can be optimised/JIT'ed (assuming V8, need to check JSC). However for browsers prior to 2018, like old SpiderMonkey or JSC's will struggle to optimize (if completely bypass it and just base LLint it) with code that sits within a try/catch. Plus generate underwater stacks that they carry along for rollbacks when an issue happens with a memory hit on this. This could, in theory, add a bit of drag on a very hot path in our run loop on browsers around that era. Which is why L2 doesn't do the same thing and chose for the emit pattern instead to create a separated boundary.

In L2:
Frame event -> frameStart event, AnimationManager listens
callback point -> animition event end -> client code
no RAF try/catch wrapping

we could also try to come up with a pattern that would avoid issues on older JavaScript environments and aligns closer to L2. Anyway open for discussion on it 😄

@wouterlucas wouterlucas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please split PRs in two sep ones, and lets have the discussion on the impact of a try/catch block

@guilhermesimoes

Copy link
Copy Markdown
Contributor Author

Opened #829!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants