Investigate noIterationTypes crash

[rbuckton]

Investigating a crash related to the noIterationTypes sentinel.

Fixes #32953

[rbuckton]

@typescript-bot pack this

[typescript-bot]

Heya @rbuckton, I've started to run the tarball bundle task on this PR at 715c7a1. You can monitor the build here. It should now contribute to this PR's status checks.

[rbuckton]

@dbaeumer, @falsandtru: Once @typescript-bot indicates the tarball for the proposed fix has completed, would you be able to try it out in your local workspace and report back as to whether this has improved stability in the editor?

[typescript-bot]

Hey @rbuckton, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/61494/artifacts?artifactName=tgz&fileId=8CC752B1C9D5115A76D08D4217AB6AEB4CAC71F4E6F9A950CBE4971A628C797302&fileName=/typescript-3.8.0-insiders.20200124.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[falsandtru]

Looks like the problem disappeared 👍

[rbuckton]

I'd like to verify whether this issue was indeed related to nameType being incorrectly set on a Symbol. After the bot finishes packing up the new changes, I'd appreciate if you were able to try the new tarball to see if the issue is still resolved.

@typescript-bot pack this

[typescript-bot]

Heya @rbuckton, I've started to run the tarball bundle task on this PR at 3a775bb. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Hey @rbuckton, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/61500/artifacts?artifactName=tgz&fileId=6DE5F03EC707F2C688D47C8A37406AD3E9DCB5451FA6A4F6F66F0AA52CB6BB5C02&fileName=/typescript-3.8.0-insiders.20200124.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[falsandtru]

I can repro with the reverted version. As I wrote in #32953, I can repro also with yesterday's build.

[rbuckton]

Unfortunately, I still am unable to repro this issue following the steps you provided.

[rbuckton]

@typescript-bot pack this

[typescript-bot]

Heya @rbuckton, I've started to run the tarball bundle task on this PR at 85f00df. You can monitor the build here. It should now contribute to this PR's status checks.

[rbuckton]

@falsandtru, @dbaeumer: Unfortunately I've been unable to repro this using either @falsandtru's repro steps or by manually making changes to the ESLint VSCode extension that @dbaeumer was working on in their issue.

While it looks like we have verified that we can resolve this specific issue by moving noIterationTypes outside of createTypeChecker, the fact that we could end up with multiple different versions of noIterationTypes indicates there is a leak somewhere in the type checker, as this should not be possible.

So far I have identified 3 possible culprits that could leak a TypeChecker (and an older noIterationTypes):

• The nameType property on Symbol, which could leak a type on symbols created by the binder that could be reused during incremental parse.
  • We have already verified that this was not the underlying cause in @falsandtru's case.
• The contextualType property on Node could outlive a TypeChecker if the current semantic operation is canceled (via a cancellation token).
• The combineValueAndTypeSymbols function could possibly assign a SymbolTable created by the binder to a TransientSymbol created by the checker. This could also result in a leak as much of the checker assumes that symbol tables on transient symbols are themselves transient and won't outlive the checker.

I've just pushed up a commit that adds a number of checks to the TypeChecker public API and several internal APIs, as well as addressed the issues mentioned above. The hope is that either one of the above issues was the culprit, or that we will be able to use the additional checks to narrow down the underlying leak. If you are able, once the bot has finished packing a tarball of these changes, I would appreciate if you could use it to test your respective scenarios.

[typescript-bot]

Hey @rbuckton, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/61725/artifacts?artifactName=tgz&fileId=4764E15986F6C2A8655A02E8DF130203E0FFC68E7351FC2349859822FB16204B02&fileName=/typescript-3.8.0-insiders.20200125.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[falsandtru]

Not reproducible as expected.

[rbuckton]

The issue no longer exists as of this build? That indicates the issue was one of the three mentioned above.

[falsandtru]

Yes. The build for debugging should output debug log to discover the problem.

[falsandtru]

Another, the log of #32953 (comment) doesn't help to investigate how to repro?

[dbaeumer]

@rbuckton we have endgame this week so not too much programming. But I will make sure I use the latest build and have an eye on stale problems. From my experience I think I got hit by the cancellation. The traces even showed that a semantic diagnostic operation got canceled before. So may it can be reproduce by setting a break point into the server that results in the client to cancel the operation.

[rbuckton]

Unfortunately I don't have a test that can repro this behavior, so I am hoping at the very least that the guards that I've added will be enough to catch future leaks of this kind.

[rbuckton]

NOTE: I needed to move these after the const checker = {...} declaration as createSymbol now depends on checker to ensure we add the current TypeChecker to all symbols to make sure we are always using the correct checker.

[rbuckton]

@typescript-bot perf test

[typescript-bot]

Heya @rbuckton, I've started to run the perf test suite on this PR at d9d796b. You can monitor the build here. It should now contribute to this PR's status checks.

Update: The results are in!

[typescript-bot]

@rbuckton
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..36388

Metric	master	36388	Delta	Best	Worst
Angular - node (v10.16.3, x64)
Memory used	355,817k (± 0.01%)	360,180k (± 0.03%)	+4,363k (+ 1.23%)	359,934k	360,529k
Parse Time	1.61s (± 0.66%)	1.62s (± 0.61%)	+0.00s (+ 0.25%)	1.60s	1.65s
Bind Time	0.89s (± 0.84%)	0.87s (± 0.78%)	-0.01s (- 1.47%)	0.86s	0.89s
Check Time	4.62s (± 0.50%)	4.59s (± 0.33%)	-0.02s (- 0.48%)	4.57s	4.64s
Emit Time	5.21s (± 0.60%)	5.26s (± 0.87%)	+0.05s (+ 0.94%)	5.13s	5.35s
Total Time	12.32s (± 0.43%)	12.34s (± 0.35%)	+0.02s (+ 0.14%)	12.21s	12.43s
Monaco - node (v10.16.3, x64)
Memory used	364,444k (± 0.01%)	370,036k (± 0.01%)	+5,592k (+ 1.53%)	369,915k	370,172k
Parse Time	1.26s (± 0.84%)	1.25s (± 0.58%)	-0.01s (- 0.64%)	1.23s	1.26s
Bind Time	0.78s (± 0.47%)	0.77s (± 0.58%)	-0.01s (- 1.03%)	0.76s	0.78s
Check Time	4.64s (± 0.54%)	4.70s (± 0.63%)	+0.06s (+ 1.27%)	4.65s	4.78s
Emit Time	2.90s (± 1.66%)	2.95s (± 0.72%)	+0.05s (+ 1.62%)	2.90s	3.00s
Total Time	9.57s (± 0.68%)	9.67s (± 0.48%)	+0.09s (+ 0.95%)	9.57s	9.76s
TFS - node (v10.16.3, x64)
Memory used	323,708k (± 0.02%)	325,199k (± 0.03%)	+1,491k (+ 0.46%)	324,991k	325,346k
Parse Time	0.95s (± 0.68%)	0.94s (± 0.82%)	-0.01s (- 1.16%)	0.92s	0.96s
Bind Time	0.74s (± 1.08%)	0.73s (± 1.47%)	-0.01s (- 1.22%)	0.70s	0.75s
Check Time	4.23s (± 0.54%)	4.19s (± 0.55%)	-0.04s (- 1.04%)	4.14s	4.24s
Emit Time	3.05s (± 0.51%)	3.01s (± 1.16%)	-0.03s (- 1.15%)	2.94s	3.08s
Total Time	8.96s (± 0.34%)	8.87s (± 0.48%)	-0.10s (- 1.06%)	8.77s	8.96s
Angular - node (v12.1.0, x64)
Memory used	331,600k (± 0.03%)	335,757k (± 0.03%)	+4,157k (+ 1.25%)	335,587k	336,003k
Parse Time	1.57s (± 0.63%)	1.57s (± 0.73%)	-0.00s (- 0.13%)	1.54s	1.59s
Bind Time	0.86s (± 0.86%)	0.86s (± 0.67%)	0.00s ( 0.00%)	0.85s	0.87s
Check Time	4.54s (± 0.34%)	4.52s (± 0.68%)	-0.02s (- 0.55%)	4.46s	4.63s
Emit Time	5.43s (± 0.44%)	5.43s (± 0.70%)	+0.00s (+ 0.07%)	5.37s	5.51s
Total Time	12.40s (± 0.17%)	12.38s (± 0.50%)	-0.02s (- 0.19%)	12.28s	12.55s
Monaco - node (v12.1.0, x64)
Memory used	344,282k (± 0.02%)	349,761k (± 0.01%)	+5,480k (+ 1.59%)	349,700k	349,836k
Parse Time	1.22s (± 0.60%)	1.21s (± 0.62%)	-0.00s (- 0.33%)	1.19s	1.22s
Bind Time	0.75s (± 0.94%)	0.74s (± 1.11%)	-0.01s (- 1.60%)	0.72s	0.76s
Check Time	4.52s (± 0.34%)	4.57s (± 0.51%)	+0.05s (+ 1.13%)	4.54s	4.64s
Emit Time	2.95s (± 0.75%)	2.98s (± 0.68%)	+0.03s (+ 1.05%)	2.94s	3.02s
Total Time	9.43s (± 0.19%)	9.50s (± 0.41%)	+0.07s (+ 0.72%)	9.42s	9.59s
TFS - node (v12.1.0, x64)
Memory used	306,002k (± 0.02%)	307,462k (± 0.02%)	+1,460k (+ 0.48%)	307,330k	307,557k
Parse Time	0.94s (± 0.61%)	0.93s (± 0.39%)	-0.01s (- 0.64%)	0.93s	0.94s
Bind Time	0.70s (± 0.42%)	0.69s (± 0.90%)	-0.01s (- 1.85%)	0.68s	0.71s
Check Time	4.16s (± 0.30%)	4.13s (± 0.37%)	-0.03s (- 0.67%)	4.10s	4.17s
Emit Time	3.09s (± 0.55%)	3.06s (± 0.39%)	-0.02s (- 0.68%)	3.04s	3.09s
Total Time	8.89s (± 0.28%)	8.82s (± 0.25%)	-0.07s (- 0.79%)	8.78s	8.87s
Angular - node (v8.9.0, x64)
Memory used	350,857k (± 0.02%)	355,056k (± 0.01%)	+4,198k (+ 1.20%)	354,961k	355,133k
Parse Time	2.09s (± 0.32%)	2.11s (± 0.74%)	+0.02s (+ 0.81%)	2.08s	2.15s
Bind Time	0.93s (± 0.39%)	0.93s (± 0.82%)	0.00s ( 0.00%)	0.91s	0.94s
Check Time	5.42s (± 0.74%)	5.37s (± 0.36%)	-0.06s (- 1.03%)	5.34s	5.42s
Emit Time	6.18s (± 0.80%)	6.25s (± 0.43%)	+0.07s (+ 1.18%)	6.19s	6.30s
Total Time	14.62s (± 0.48%)	14.66s (± 0.22%)	+0.04s (+ 0.25%)	14.57s	14.71s
Monaco - node (v8.9.0, x64)
Memory used	362,765k (± 0.02%)	368,229k (± 0.01%)	+5,464k (+ 1.51%)	368,115k	368,302k
Parse Time	1.56s (± 0.63%)	1.56s (± 0.55%)	0.00s ( 0.00%)	1.55s	1.59s
Bind Time	0.95s (± 0.59%)	0.95s (± 1.06%)	-0.00s (- 0.21%)	0.92s	0.97s
Check Time	5.38s (± 1.42%)	5.44s (± 1.41%)	+0.06s (+ 1.11%)	5.30s	5.57s
Emit Time	3.27s (± 4.24%)	3.22s (± 4.17%)	-0.05s (- 1.47%)	3.01s	3.43s
Total Time	11.16s (± 0.81%)	11.18s (± 0.64%)	+0.01s (+ 0.09%)	11.06s	11.30s
TFS - node (v8.9.0, x64)
Memory used	322,975k (± 0.01%)	324,392k (± 0.01%)	+1,417k (+ 0.44%)	324,321k	324,522k
Parse Time	1.26s (± 0.54%)	1.26s (± 0.47%)	-0.00s (- 0.08%)	1.25s	1.28s
Bind Time	0.76s (± 0.96%)	0.74s (± 0.49%)	-0.01s (- 1.85%)	0.74s	0.75s
Check Time	4.82s (± 0.62%)	4.76s (± 0.49%)	-0.06s (- 1.18%)	4.72s	4.81s
Emit Time	3.20s (± 0.86%)	3.19s (± 0.69%)	-0.01s (- 0.34%)	3.13s	3.23s
Total Time	10.05s (± 0.54%)	9.97s (± 0.27%)	-0.08s (- 0.80%)	9.92s	10.04s
Angular - node (v8.9.0, x86)
Memory used	199,454k (± 0.02%)	201,541k (± 0.02%)	+2,088k (+ 1.05%)	201,455k	201,619k
Parse Time	2.03s (± 0.64%)	2.04s (± 0.65%)	+0.02s (+ 0.89%)	2.00s	2.06s
Bind Time	1.04s (± 1.21%)	1.02s (± 0.69%)	-0.01s (- 1.35%)	1.01s	1.04s
Check Time	4.93s (± 0.51%)	4.88s (± 0.59%)	-0.05s (- 0.95%)	4.82s	4.94s
Emit Time	6.05s (± 1.64%)	6.22s (± 1.98%)	+0.16s (+ 2.69%)	6.05s	6.58s
Total Time	14.04s (± 0.72%)	14.16s (± 0.89%)	+0.12s (+ 0.84%)	13.89s	14.47s
Monaco - node (v8.9.0, x86)
Memory used	203,611k (± 0.02%)	206,376k (± 0.03%)	+2,765k (+ 1.36%)	206,287k	206,527k
Parse Time	1.60s (± 0.46%)	1.61s (± 0.58%)	+0.01s (+ 0.75%)	1.59s	1.63s
Bind Time	0.77s (± 0.62%)	0.76s (± 0.79%)	-0.02s (- 2.33%)	0.74s	0.77s
Check Time	5.12s (± 0.55%)	5.45s (± 1.50%)	+0.33s (+ 6.47%)	5.15s	5.58s
Emit Time	3.20s (± 0.55%)	2.91s (± 3.35%)	-0.29s (- 9.01%)	2.83s	3.29s
Total Time	10.68s (± 0.36%)	10.72s (± 0.51%)	+0.04s (+ 0.35%)	10.60s	10.81s
TFS - node (v8.9.0, x86)
Memory used	182,295k (± 0.02%)	182,951k (± 0.02%)	+656k (+ 0.36%)	182,878k	183,060k
Parse Time	1.32s (± 1.13%)	1.30s (± 0.88%)	-0.01s (- 1.06%)	1.29s	1.34s
Bind Time	0.72s (± 1.26%)	0.71s (± 0.78%)	-0.01s (- 1.80%)	0.70s	0.72s
Check Time	4.62s (± 0.66%)	4.53s (± 0.67%)	-0.09s (- 1.91%)	4.49s	4.63s
Emit Time	2.99s (± 0.92%)	2.97s (± 0.81%)	-0.02s (- 0.60%)	2.92s	3.05s
Total Time	9.64s (± 0.54%)	9.51s (± 0.42%)	-0.14s (- 1.43%)	9.43s	9.60s

System

Machine Name	ts-ci-ubuntu
Platform	linux 4.4.0-166-generic
Architecture	x64
Available Memory	16 GB
Available Memory	5 GB
CPUs	4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz

Hosts

• node (v10.16.3, x64)
• node (v12.1.0, x64)
• node (v8.9.0, x64)
• node (v8.9.0, x86)

Scenarios

• Angular - node (v10.16.3, x64)
• Angular - node (v12.1.0, x64)
• Angular - node (v8.9.0, x64)
• Angular - node (v8.9.0, x86)
• Monaco - node (v10.16.3, x64)
• Monaco - node (v12.1.0, x64)
• Monaco - node (v8.9.0, x64)
• Monaco - node (v8.9.0, x86)
• TFS - node (v10.16.3, x64)
• TFS - node (v12.1.0, x64)
• TFS - node (v8.9.0, x64)
• TFS - node (v8.9.0, x86)

Benchmark	Name	Iterations
Current	36388	10
Baseline	master	10

[ahejlsberg]

Also, let's make sure performance isn't impacted.

[ahejlsberg]

I'm not totally sold on the value of the verifyCheckerOfXXX calls and the checker property always being present. It's overhead and the bug we're fixing wasn't actually caused by anything this is verifying.

[rbuckton]

I can pull the API verification for now, but we really should consider ensuring our public API doesn't allow you to try to mix and match types/signatures/symbols produces by other checkers.

[rbuckton]

@ahejlsberg: Any other comments after having reverted the verifyX changes for now?

[ahejlsberg]

Looks good, but curious about my question below.

[ahejlsberg]

Why do we need to clone here?

[weswigham]

if typeSymbol or valueSymbol are not TransietSymbols, then their symbol tables consist only of non-transient symbols and persist through multiple checks. If we only copy the reference to the table into a new TransientSymbol, when we later stuff more things into the table (as is possible for transient symbols), we'll pollute the supposedly "immutable" symbol table from the original non-transient symbol with new transient members (which will then outlive the checker they were created within).

[rbuckton]

This was one of 3 possible leaks. We assume elsewhere in the checker that any SymbolTable attached to a TransientSymbol is safe to mutate, and sometimes add new symbols to it. combineValueAndTypeSymbols currently just blindly assigns the reference to the symbol table to result (which is a TransientSymbol), ignoring whether typeSymbol or valueSymbol were created by the binder. As a result, there is the possibility of some other function in checker later mutating the SymbolTable.

[rbuckton]

Need to reopen this on master since I used release-3.8 primarily for providing a stable drop for testers.

@typescript-bot cherry-pick this to master

[typescript-bot]

Hey @rbuckton, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into master manually.

[rbuckton]

Closing in favor of #36491, which ports this change to master instead.

[rbuckton]

This has been merged into master.