React-modal: Huge memory leak with Jest/Enzyme after upgrading to react-modal 3.X

[ryami333]

Summary:

For a while now, I've been unable to upgrade to react-modal 3.X in my React 16 project with Jest/Enzyme because when I run tests Node itself actually crashes and I usually have to take some fairly nuclear steps such as force-quitting the process in my activity monitor app in order to use it again. The latest 2.X release of react-modal is totally fine.

Steps to reproduce:

1. Upgrade react-modal from 2.4.1 to 3.X
2. Run npm run jest
3. First few tests may run, but eventually the process hangs, and eventually these errors start to spill out:

<--- Last few GCs --->

[46548:0x102802400]    43615 ms: Mark-sweep 1402.9 (1437.6) -> 1402.9 (1436.6) MB, 1477.5 / 0.0 ms  (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 1477 ms) last resort GC in old space requested
[46548:0x102802400]    45072 ms: Mark-sweep 1402.9 (1436.6) -> 1402.9 (1436.6) MB, 1456.0 / 0.0 ms  last resort GC in old space requested


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x38a049725ec1 <JSObject>
    1: read [/path/to/project/node_modules/react-dom/cjs/react-dom-server.node.development.js:~2206] [pc=0x181429965313](this=0x38a08a383211 <ReactDOMServerRenderer map = 0x38a00fd8cc29>,bytes=0x38a0125029a9 <Number inf>)
    3: renderToStaticMarkup [/path/to/project/node_modules/react-dom/cjs/react-dom-server.node.development.js:2512] [bytecode=0x38a0...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: node::Abort() [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 3: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 4: v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 5: v8::internal::Runtime_AllocateInTargetSpace(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/username/.nvm/versions/node/v8.9.0/bin/node]
 6: 0x18142970463d

*Note that I've replaced the actual path and username in this snippet.

[diasbruno]

Hi @ryami333, can you give more information about this tests?...or is there anything unusual about how are you using react-modal like keep reference of the modal children?

[enapupe]

This seems to be an issue with createPortal.. not sure if enzyme has proper support for it?

[ryami333]

@diasbruno I'm not doing anything like that, no. I'm using enzyme and enzyme-to-json. Most of my tests follow this kind of pattern:

import React from 'react';
import { render } from 'enzyme'; // 3.3.0 (latest)
import toJson from 'enzyme-to-json'; // 3.3.0 (latest)

describe('FooComponent', () => {
    let wrapper;

    it('renders without crashing', () => {
        wrapper = render(
            <FooComponent />,
        );
    });

    it('matches existing snapshot', () => {
        expect(toJson(wrapper)).toMatchSnapshot();
    });
});

Early on, the tests are all appearing to pass, but they just get slower and slower, and eventually the test runner hangs and those node heap errors start spewing.

@enapupe yeah portals seem the most likely candidate don't they?

[diasbruno]

@ryami333 That's a place to look at. Thanks, @enapupe.

[ryami333]

@diasbruno have you had any success replicating this bug? I'm still unable to upgrade at all. Using latest versions of enzyme, enzyme-adapter-react-16, react-test-renderer and of course react-modal.

I've managed to find that it works with Enzyme's 'mount' but not with 'render'.

[diasbruno]

This can be related to the environment the modal is been rendered src/components/Modal.js#L187.

It will just return null if it can not use the DOM.

[ryami333]

But why would returning null result in a big memory exception? Returning null is a perfectly valid render return value.

[diasbruno]

It's possible that the modal is not clean up resources properly. Can you write a small app with test? It would be better to debug.

[ryami333]

Here you go @diasbruno: https://github.com/ryami333/react-modal-memory-exception

This is a 'Create React App' web-app, where I've done the following:

• Installed enzyme, react-modal and enzyme-adapter-react-16.
• Added a <Modal> to App.js.
• Tried to render the 'App' component in App.test.js.

Simply run yarn install && yarn test (or npm install && npm test I suppose) to replicate.

[diasbruno]

The modal seems to be rendered. I'll try to see what is going on.

./node_modules/.bin/react-scripts --inspect-brk test --env=jsdom --runInBand

[rharriso]

This could be an issue with jsdom I was seeing a similar bug when running integration tests (no enzyme render). This problem went away when I removed jsdom from the configuration.

[diasbruno]

@rharriso Thanks for the info. I'll try to dig in.

[rharriso]

@diasbruno some more info WRT jsdom.

This issue remained with the latest version 11.6.2.
I didn't see any issues in their backlog that looked related.

[diasbruno]

Some resources to help tracking this issue:

https://github.com/felixge/node-memory-leak-tutorial
https://github.com/jsdom/jsdom/issues/1705

[diasbruno]

It seems to be stuck in an infinity loop on:

ReactDOMServerRenderer.prototype.read = function read(bytes) { 
  // bytes = Infinity
  ...

  var child = frame.children[frame.childIndex++]; // always $$typeof: Symbol(react.portal)
  ...
}

[diasbruno]

Well...your test has passed. :)

```shell
PASS src/App.test.js (347.669s)
â renders without crashing (342764ms)

Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Snapshots: 0 total
Time: 348.978s, estimated 1404s
Ran all test suites related to changed files.

Watch Usage
⺠Press a to run all tests.
⺠Press p to filter by a filename regex pattern.
⺠Press t to filter by a test name regex pattern.
⺠Press q to quit watch mode.
⺠Press Enter to trigger a test run.

[diasbruno]

Try to apply this patch on your project and tell me your results.

diff --git a/react-dom-server.node.development.js b/dd.js
index 2039700..d4832da 100644
--- a/react-dom-server.node.development.js
+++ b/dd.js
@@ -2227,7 +2227,8 @@ var ReactDOMServerRenderer = function () {
         }
         continue;
       }
-      var child = frame.children[frame.childIndex++];
+      frame.childIndex++;
+      var child = frame.children[frame.childIndex];
       {
         setCurrentDebugStack(this.stack);
       }

patch -p1 < patch.diff

node_modules/react-dom/cjs/react-dom-server.node.development.js

[diasbruno]

It's not a proper fix, I still need to investigate more, it's just to have an idea of what is going on...

[diasbruno]

cc @ryami333 Please let me know when you get a chance to try this patch.

[rharriso]

@diasbruno I tried this out on my test suite. And it works!

[diasbruno]

@rharriso sorry, forgot to mention you. Thanks for trying it.

[diasbruno]

After some investigation, I reached the same state as facebook/react/issues/11565.

[ryami333]

@diasbruno that patch works for me too! Does this mean you will create a PR for react itself?

[diasbruno]

Actually, it's not a fix. The things I wrote here are just for logging. That (patch) is really incorrect since it will skip the frame.children[0]. So, I'll continue debugging this issue.

WARNING: it is not known the side-effects of this patch. :)

[diasbruno]

@ryami333 @rharriso Did you applied the patch on a real project?

[ryami333]

Yes I did, one with over 200 render tests.

[diasbruno]

Interesting. I'll continue investigating to find the problem (if possible).

[ryami333]

Sorry, I should qualify my response a little bit - the tests don't all pass, but they no longer crash.

[rharriso]

@diasbruno, yes. It's a private company project with 600 tests

[rharriso]

However, the error was see in our integration test suite, which renders with ReactDOM, not enzyme

[diasbruno]

@rharriso so you are using some kind of browser?

[rharriso]

No, these tests just make sure the page responds with 200.
we inject a request into the Hapi server, and looking at the response code.

[diasbruno]

There is a second patch I made, it should work with react-test-render (I still need to test it).

I'll publish on a fork of react.

[rharriso]

@diasbruno Let me know what you want me to test out.

[diasbruno]

The potential bug is:

while (React.isValidElement(child)) {. If the child has type react.portal, React.isValidElement(child) will return false. Then it will return {child, context};.

So, it will end up in an infinity loop ({child: nextChild, context} = resolve(child, context));.

Note that in the current revision, it seems to have a fix $$typeof !== REACT_PORTAL_TYPE.

I ended up with this code before I looked the current version of react:

function isPortalElement(object) {
  return typeof object === 'object' && object !== null && object.$$typeof === REACT_PORTAL_TYPE;
}

  if (React.isPortalElement(child)) {
    child = null;
  }

  return { child: child, context: context };

@rharriso @ryami333 If you can't upgrade your react version, let me know so I can provide the patch to this version.

[rharriso]

@diasbruno I probably can't ~this~ until after next week. But probably soon after, I'll get back to you next week though!

[ryami333]

@diasbruno - what do you mean by upgrading React? I've been experiencing this on the latesr version of React (16.2.0)

[diasbruno]

You are right. Totally forgot I was on the master branch.

I'm trying to finish the patch to submit to react.

The patch is available here.

[diasbruno]

Sorry for dumping a lot of information that might be confusing or not organized (that's because I was starting to get confused by the many packages involved in this issue and I was starting to look into the react-test-render issue).

[asborisov]

@diasbruno Can you advice the best way to avoid this problem for now (until fix merged into next version of react)?

[rharriso]

@asborisov I avoided this by removing jsdom from our integration tests. That may not be an option for unit tests though.

[diasbruno]

@asborisov @rharriso Follow the react issue I've mention here. Many ideas/workaround have been publish there.

[asborisov]

I downgrade to 2.4.2 and looks it work now. Will wait until correct fix is merged