Restrict DeepMerger mutable object reuse to fix subtle production-only bug
#9742
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjamn
commented
May 19, 2022
Comment on lines
-249
to
-257
| merge(a, b) { | ||
| // We use the same DeepMerger instance throughout the read, so any | ||
| // merged objects created during this read can be updated later in the | ||
| // read using in-place/destructive property assignments. Once the read | ||
| // is finished, these objects will be frozen, but in the meantime it's | ||
| // good for performance and memory usage if we avoid allocating a new | ||
| // object for every merged property. | ||
| return merger.merge(a, b); | ||
| }, |
There was a problem hiding this comment.
This comment might have been right about it being "good for performance and memory usage if we avoid allocating a new object for every merged property," in theory, but you have to be prepared to abandon any optimization that causes bugs, as this one did.
benjamn
added a commit
that referenced
this pull request
May 19, 2022
benjamn
force-pushed
the
issue-9735-DeepMerger-prod-only-mismerge
branch
from
9fc3818
to
916f974
Compare
|
LGTM. I've checked and confirmed that this does resolve the bug in the sample reproduction in #9735. |
If you revert the previous commit and run `npm test`, you'll see all the tests this dynamic Object.isFrozen check has been silently protecting from failing, but only (and this is the important part) in development. Since we only actually freeze objects with Object.freeze in development, this Object.isFrozen check does not help in production, so an object that would have been frozen in development gets reused as a mutable copy, potentially acquiring properties it should not acquire (a bug fixed by the previous commit, first reported in issue #9735).
benjamn
force-pushed
the
issue-9735-DeepMerger-prod-only-mismerge
branch
from
916f974
to
570b7f3
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #9735 appears to be due to a bug I introduced in my commit 756ab87 in PR #8734. This PR takes us (partially) back to the way things worked before that commit.
I believe I've found two complementary ways to fix this problem:
Thanks very much to @akallem for some deep debugging and an excellent reproduction in #9735.