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

[Fresh] Fix an infinite loop in an edge case #17414

Merged
merged 2 commits into from Nov 21, 2019
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 21, 2019

A good old mutation-while-iterating kind of bug. These maps and sets can be mutated during a commit, so we can't just forEach here. Otherwise we risk running into an infinite loop where processing each next item causes more items to be added to the list.

Added a regression test. Verified this fixes a problem on WWW.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 21, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9fe214a:

Sandbox Source
quizzical-beaver-q28gu Configuration

@sizebot
Copy link

sizebot commented Nov 21, 2019

Details of bundled changes.

Comparing: a7d07ff...9fe214a

react-refresh

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-refresh-runtime.development.js +4.8% +5.2% 16.77 KB 17.58 KB 5.13 KB 5.4 KB NODE_DEV
react-refresh-runtime.production.min.js 0.0% 🔺+0.4% 368 B 368 B 264 B 265 B NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against 9fe214a

@sizebot
Copy link

sizebot commented Nov 21, 2019

Details of bundled changes.

Comparing: a7d07ff...9fe214a

react-refresh

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFreshRuntime-dev.js +4.8% +5.3% 16.81 KB 17.61 KB 5.11 KB 5.38 KB FB_WWW_DEV
react-refresh-runtime.development.js +4.8% +5.2% 16.78 KB 17.59 KB 5.14 KB 5.41 KB NODE_DEV

Size changes (experimental)

Generated by 🚫 dangerJS against 9fe214a

// If we don't do this, there is a risk they will be mutated while
// we iterate over them. For example, trying to recover a failed root
// may cause another root to be added to the failed list -- an infinite loop.
let failedRootsSnapshot = new Map(failedRoots);
Copy link
Contributor

Choose a reason for hiding this comment

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

is it supposed that new Map([iterable]) is polyfilled in react-refresh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would IE11 be the only one that can’t do it? I guess we could use something longer but tbh I don’t know if we care about supporting IE11 for a developer productivity feature like this.

@threepointone
Copy link
Contributor

Ouch, a classic bug.

@gaearon gaearon merged commit 237a966 into facebook:master Nov 21, 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

5 participants