Skip to content

Fix stack overflow in merge symbol#24134

Merged
sandersn merged 3 commits into
masterfrom
fix-stack-overflow-in-mergeSymbol
May 15, 2018
Merged

Fix stack overflow in merge symbol#24134
sandersn merged 3 commits into
masterfrom
fix-stack-overflow-in-mergeSymbol

Conversation

@sandersn
Copy link
Copy Markdown
Member

Fixes #24131

  1. targetInitializer needs to be merged before comparing to target. Transient symbols are never equal to non-transient symbols.
  2. Target's valueDeclaration mutates while mergeSymbol is running, but getJSInitializerSymbol depends on it. mergeSymbol needs to save the original value of target.valueDeclaration before looking for a js initializer.

sandersn added 3 commits May 14, 2018 14:58
target's valueDeclaration is set to the source's if it is not present.
This makes it incorrect to use getJSInitializerSymbol because it relies
on the symbol's valueDeclaration.

This fix just saves the original valueDeclaration before mutating and
uses that.
Instead of the unmerged one
@sandersn sandersn requested review from RyanCavanaugh and mhegazy May 15, 2018 18:40
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented May 15, 2018

can we merge this in release-2.8

@sandersn
Copy link
Copy Markdown
Member Author

It is now in 2.8.

@sandersn sandersn merged commit 0ba8998 into master May 15, 2018
@sandersn sandersn deleted the fix-stack-overflow-in-mergeSymbol branch May 15, 2018 19:49
@billti
Copy link
Copy Markdown
Member

billti commented May 16, 2018

2.9 will need it also (if not planning to merge 'master' into it again).

sandersn added a commit that referenced this pull request May 16, 2018
* JS initializers use original valueDecl, not mutated

target's valueDeclaration is set to the source's if it is not present.
This makes it incorrect to use getJSInitializerSymbol because it relies
on the symbol's valueDeclaration.

This fix just saves the original valueDeclaration before mutating and
uses that.

* Compare merged targetInitializer to target

Instead of the unmerged one

* Add test of stack overflow
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants