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 message loop behavior when host callback is cancelled #16407

Merged
merged 3 commits into from Aug 19, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 15, 2019

c2a1a77

New test.

This works on master but would have been broken by #16145.
I verified the test fails on that branch.

3b70481

This is the fix. Again, it doesn't affect master because we don't use cancelHostCallback yet. But it fixes the previously added test when combined with #16145. The actual problem isn't related to #16145, we just didn't execute the code path that triggers the bad case until that PR.

The problem itself is that cancelHostCallback would clear out the callback. Then in the message loop we would see that it's null, and exit early. But we wouldn't schedule another one, so the loop would stop. However, the boolean itself would stay true, so next time we schedule something, we'd think the loop was still running. And fail to schedule it. The fix is to reset the boolean.

The rAF codepath handles this earlier — in onAnimate. But we don't have that entry point in the message loop implementation. So we forgot to do it.

22a1287

Just some more tests from #16271 since we're not doing it for now.

@gaearon gaearon changed the title Add a regression test for cancelCallback with message loop Fix message loop behavior when host callback is cancelled Aug 15, 2019
@sizebot
Copy link

sizebot commented Aug 15, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@gaearon gaearon merged commit 2d68bd0 into facebook:master Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants