Fix: Move destroy field to shared instance object

[acdlite]

This fixes the "double free" bug illustrated by the regression test added in the previous commit.

The underlying issue is that effect.destroy field is a mutable field but we read it during render. This is a concurrency bug — if we had a borrow checker, it would not allow this.

It's rare in practice today because the field is updated during the commit phase, which takes a lock on the fiber tree until all the effects have fired. But it's still theoretically wrong because you can have multiple Fiber copies each with their own reference to a single destroy function, and indeed we discovered in production a scenario where this happens via our current APIs.

In the future these types of scenarios will be much more common because we will introduce features where effects may run concurrently with the render phase — i.e. an imperative hide method that synchronously hides a React tree and unmounts all its effects without entering the render phase, and without interrupting a render phase that's already in progress.

A future version of React may also be able to run the entire commit phase concurrently with a subsequent render phase. We can't do this now because our data structures are not fully thread safe (see: the Fiber alternate model) but we should be able to do this in the future.

The fix I've introduced in this commit is to move the destroy field to a separate object. The effect "instance" is a shared object that remains the same for the entire lifetime of an effect. In Rust terms, a RefCell. The field is undefined if the effect is unmounted, or if the effect ran but is not stateful. We don't explicitly track whether the effect is mounted or unmounted because that can be inferred by the hiddenness of the fiber in the tree, i.e. whether there is a hidden Offscreen fiber above it.

It's unfortunate that this is stored on a separate object, because it adds more memory per effect instance, but it's conceptually sound. I think there's likely a better data structure we could use for effects; perhaps just one array of effect instances per fiber. But I think this is OK for now despite the additional memory and we can follow up with performance optimizations later.

[react-sizebot]

Comparing: 4a1cc2d...94e2375

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	+0.05%	162.95 kB	163.02 kB	+0.04%	51.54 kB	51.56 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.05%	164.48 kB	164.56 kB	+0.06%	52.03 kB	52.06 kB
facebook-www/ReactDOM-prod.classic.js	+0.03%	553.05 kB	553.24 kB	+0.02%	98.24 kB	98.26 kB
facebook-www/ReactDOM-prod.modern.js	+0.04%	536.90 kB	537.08 kB	+0.03%	95.56 kB	95.59 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 94e2375

[gaearon]

Btw why do we not clear out the destroy field here?

[acdlite]

I guess it doesn't really matter here because the fiber is being deleted but we can for consistency

[gaearon]

Mind adding Jan and Rick to my test commit as coauthors?