React: Fiber infinite recursion in mounting lifecycle

[gre]

here is an app https://gist.github.com/gre/52e28f74eda0beccc17e880efe18c250
There is intentionally a bug in this app: the App component returns <Demo setVariables={v=>this.setState(..)} variables={..} /> and Demo calls setVariables in componentDidMount and componentWillReceiveProps (it shouldn't, this is bad user code). What this issue raises is a regression behaviour in React 16 compared to 15.

in React 15 : it renders but you directly get a Uncaught RangeError: Maximum call stack size exceeded.
in React 16 : nothing renders, the app never loads and the browser is spinning forever! I think React is stuck in an infinite recursion.

I presume this is because fiber is by nature async. The fact things never ends makes it very hard to investigate and isolate the source of the problem in a large codebase.
should there be some sort of maximum call stack in Fiber ?

Context: When we migrated our app to latest React Native recently, we had a weird freezing case: the app was completely unresponsive (no button works) but you could still do native actions like scroll a ScrollView. however, even the RN devtool menu action was not working (like the inspector) – probably because it involves JS and React was just stuck in this recursion loop.
In our case, the issue was because calling relay.setVariables in lifecycle but with a wrong logic. for some reason our code used to work in the past but now creates this recursion loop.

[gaearon]

It is funny but we just hit a similar case a few hours ago internally. I think we’ll need to set a hard limit for such “recursion” because what used to be stack overflows in Stack have turned into hangs in Fiber.

[codepodu]

@gaearon How would we go about detecting this in fiber? I suppose we'd have to look for N updates in a given period with circular contexts. ReactFiberScheduler#scheduleUpdate maybe?
CallbackQueue#enqueue if we need something super low-level.

[gaearon]

I was thinking we'd do this somewhere in workLoop if we go through too many iterations but @acdlite might have better ideas.

[acdlite]

Depends on what you want to count and limit. If you want to limit setState, then you put the check in scheduleUpdate. If you want to limit the number of components that are worked on, you put it in the work loop.

But I think what we want to limit is the number of times we commit, so we'd put the check in commitAllWork.

[bgirard]

This seems to be responsible for a lot of hangs on Facebook.com. We've seen several instances where components failed to hit a stable states which now hangs in Fiber. Adding a limit would let developers more easily attach devtools and fix the problem and wont cause a total hang for users.

[gaearon]

Would you be interested in helping out by sending a PR?

[codepodu]

I'm interested. What do we look for as the threshold to raise an error, though? number of times we commit in an interval?

[acdlite]

I think so, yeah. Where an "interval" is the body of performWork.

[acdlite]

I wonder what the limit should be. @sebmarkbage believes that most valid use cases will only have 2-3 iterations, which sounds right. But I'm guessing that would lead to some breakages in existing code. Since we won't ship this until 16, maybe that's ok?

[gaearon]

[acdlite]

@codepodu In the interest of getting this fixed quickly, I'm going to open a PR for this, unless you respond soon-ish and tell me that you've already started :D