toJS() crashes when certain self-referencing objects are stored in an Immutable container

[Methuselah96]

From @mcclure on Sat, 12 Sep 2020 20:26:39 GMT

Summary

The .toJS() function on Immutable containers crashes with a confusing error if the container contains a Preact v10.4.8 JSX.Element object, or possibly any number of other objects containing a self-reference. Nothing in the documentation warns that .toJS() is dangerous.

This appears to be the flipside of already-fixed issue immutable-js#653: There are checks for recursive structures now in fromJS(), but not toJS().

Repro

Here (branch preact-immutable-crash) is a small typescript+webpack+preact app. Test it with

npm install && npx webpack && (cd site && http-server -c-1)

(Assuming http-server previously installed with npm install -g http-server.)

If you build and load commit eee4, it works just fine.
If you build and load commit 2c20, it crashes.
The only difference between the two commits is instead of Preact v10.1.0, it uses 10.4.8.

The crash is an uncaught "RangeError: Maximum call stack size exceeded" exception somewhere deep in the guts of immutable.js.

You can crash Immutable.js with an even simpler test case than this:

const simplerTest = List<h.JSX.Element>().push(<div>One</div>)
const divArray = simplerTest.toJS() // Crash on this line

Literally all we did here was put a JSX.Element into simplerTest and then pull it out again.

In testing I see the same behavior with other Immutable containers, for example OrderedSet (though in retrospect it doesn't make a lot of sense to keep a JSX.Element in a set).

Things that don't crash

If you say .toArray() instead of .toJS() it does not crash. So the problem seems to be with the "deep"/recursive conversion from List to array, not a shallow version. Immutable seems to be descending into the JSX.Element object, perhaps mistaking it for an Immutable.js container.

This doesn't crash:

import { List } from "immutable"

class X {
  x: X
  constructor() {
    this.x = this
  }
}

let demoList = List<X>().push(new X()).push(new X())
let crash = demoList.toJS()

console.log(crash)

So it doesn't do this on all recursive data structures. There must be something special about Preact JSX.Element.

This problem is specific to Preact v10.4.x. I do not see the problem with Preact v10.3.4. I do see it with v10.4.8. Something changed between those two versions.

Is this a bug on Preact?

Because the problem emerged for me after upgrading Preact but without changing Immutable, I initially filed an issue on Preact. Preact's response was:

Sounds like immutable.js is lacking support for circular references. The main differences from our side is that our vnode shape has changed to have an additional self referencing pointer that is used to bail out of render when the vnode is strictly equal.

The maintainer then gives a super-simple example which crashes Immutable without using Preact, using {} instead of class.

It seems Preact is doing something reasonable that they shouldn't be expected to change; as they say "it would set a bad example if we start adding workarounds in Preact for issues in other libraries", and "fixing" the problem at their end would break other design constraints of their library.

Impact

I think this is quite a serious issue. React/Preact are a critical use case for Immutable (because their state objects don't work with mutable state).

The big problem is that it triggers by doing something innocuous, yet is very difficult to debug. Consider all the following code snippets:

1. stringList.map(x => <div>{x}</div>).toJS()
2. stringList.toJS().map(x => <div>{x}</div>)
3. stringList.map(x => <div>{x}</div>).toArray()
4. stringList.toArray().map(x => <div>{x}</div>)

These four all look the same, and the documentation does nothing to suggest they should be different, but the first snippet (and only the first) crashes. And it's a nasty crash. The error message is unhelpful ("RangeError: Maximum call stack size exceeded"?) and at least in Chrome, the stack that is printed along with the error is completely unreadable unless you run in the debugger.

Immutable has already fixed this for the fromJS() crash, but if anything the toJS() crash is if anything more insidious and dangerous because it operates on data you don't think of as "input", ie, non-Immutable objects you stored in the collection. Because the crash is caused by internal details of your data objects, it can even trigger because of changes in code you do not control. (It occurred for me when I upgraded Preact and it took hours to figure out why).

Expected behavior

Any combination of the following would help with the issue:

1. Add the same recursive data structure detection to toJS() as fromJS() has.

2. Make toJS() only recurse on Immutable data structures and not descend into plain JS structures like arrays and dictionaries? (This may be a backward-compatibility breaking change.)

3. Warn in the toJS() documentation that it is dangerous or time-intensive for certain kinds of data.

Copied from original issue: immutable-js#1789