Fix a memory leak in Error objects

[sokra]

see also https://crbug.com/v8/7142

Summary

Error object keep references to all "context"s in the stack. This will lead to memory leaks if the Error objects are stored somewhere persistent. This is an "optimization" of v8, since constructing the stack property as string can be expensive.
Accessing the stack property will force stringifying of these internal objects and discards references, which allows scopes to be garbage-collected.

A similar fix was made here: mochajs/mocha#3119

These memory leaks are usually very small since very few people store large objects outside of it(). In the webpack test suite the it() is called from the bundle after compiling. Here the bundle and the Compilation is in the stack. This causes memory leaks.

I believe but can't prove yet, that the inability of running too many test suites at once (in webpack) is also caused by this problem. It hangs without error. Let's see...

Test plan

Unsure how to test this.
I tested it manually via memory snapshots in the devtools.

I could construct a test suite that allocates much memory and will lead to memory errors without this fix, but this would probably slow down the CI... Tell me if you want this.

[SimenB]

This would need the same change in jest-circus, btw.

Also, not collecting the stack was on purpose, as we only want the stacks in case of errors, and collecting all stacks when we don't need them is pretty slow. I haven't benchmarked it, though... See #4782 for some discussion (although that one was "skip collecting stack at all", not "eagerly vs lazily evaluate")

[sokra]

This would need the same change in jest-circus, btw.

Do you mean this location?

https://github.com/facebook/jest/blob/e90db16947da1134976eb104255a97896aa7c0ca/packages/jest-circus/src/utils.js#L313-L337

It's not required here, because only the stack is returned, which already evaluates it.

[SimenB]

No, asyncError created here: https://github.com/facebook/jest/blob/36dcb7873e18e4fb059653cfef3bfb35359e6195/packages/jest-circus/src/index.js

[sokra]

No, asynError created here: https://github.com/facebook/jest/blob/36dcb7873e18e4fb059653cfef3bfb35359e6195/packages/jest-circus/src/index.js

done

[SimenB]

@mjesun ideas on why this isn't detected by --detectLeaks?

[codecov-io]

Codecov Report

Merging #6965 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6965   +/-   ##
=======================================
  Coverage   66.97%   66.97%           
=======================================
  Files         250      250           
  Lines       10381    10381           
  Branches        4        4           
=======================================
  Hits         6953     6953           
  Misses       3427     3427           
  Partials        1        1

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e90db16...030c969. Read the comment docs.

[sokra]

ideas on why this isn't detected by --detectLeaks?

just guessing, but maybe these Error objects are eventually disposed when all test suites finished running?

No, asynError created here: https://github.com/facebook/jest/blob/36dcb7873e18e4fb059653cfef3bfb35359e6195/packages/jest-circus/src/index.js

done

I think this isn't even needed in this case, because Error objects are not stored, they are only dispatched as event, which is only temporary.

[sokra]

Also, not collecting the stack was on purpose, as we only want the stacks in case of errors, and collecting all stacks when we don't need them is pretty slow. I haven't benchmarked it, though... See #4782 for some discussion (although that one was "skip collecting stack at all", not "eagerly vs lazily evaluate")

An alternative would be to get rid of references to initError and extraError when the test finishes. I think this is not possible here, because addExpectationResult can be called after the test finished, but you are the experts.

[SimenB]

If it's not a problem in circus, maybe you can switch webpack over to using it? Unless you use jasmine globals it should be a straight swap. I can send a PR.

EDIT: this couples the test run to jasmine, so no circus for now :( https://github.com/webpack/webpack/blob/ee7d9485fa30d1d3d5adbfdfb52c8f57ae8b439e/test/helpers/createLazyTestEnv.js

[sokra]

What is jest-circus? Is there any documentation about that?

[SimenB]

jest-circus will replace Jasmine as the test framework within Jest. It currently has feature parity with the publicly documented API of Jest, and @aaronabramov has ported FB over to use it (I think he's done?). At some point in the future it will be the default and you will have to opt-in to use Jasmine.

We don't really have much docs on it, we probably should...

@cpojer @aaronabramov do we have some docs, or a short pitch, on the what and why of circus?

[mjesun]

@mjesun ideas on why this isn't detected by --detectLeaks?

Nope... that's weird TBH. Jasmine executes inside the container, so it should be detected by the leaker. It might be that the stack traces have weak references, but then there shouldn't be any leak at all...

[sokra]

If this helps here is the retainers path:

[mjesun]

I think the <symbol> in Error is the one relevant; but yeah, it definitely looks like it's caused by the stack trace.

[sokra]

Is there anything else you want me to change in the PR?

[SimenB]

I think we should roll back the changes to circus (which is failing CI), seeing as you say it's not needed. We do stick that error into the state, but perhaps that state is more properly cleaned up than it is with Jasmine?

I'm also a bit afraid that this will cause a performance hit, but maybe not?

[sokra]

I'm also a bit afraid that this will cause a performance hit, but maybe not?

I did some microbenchmarking and figured out that the overhead for eager-evaluation is about 130µs in node 8, a bit lower in node 10.

It's only done a few times per test case, so the overhead is a few seconds for 10,000 test cases.

[SimenB]

Ah, ok. That's not too bad!

[SimenB]

Mind updating the changelog? Then I'll merge 🙂 Do note that we've merged a breaking change to master though, so this won't be released for a bit

[SimenB]

Thanks!

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.