Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

object spread operator mutates first argument #25089

Closed
larsthorup opened this issue Dec 17, 2018 · 9 comments
Closed

object spread operator mutates first argument #25089

larsthorup opened this issue Dec 17, 2018 · 9 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@larsthorup
Copy link

larsthorup commented Dec 17, 2018

  • Version: v11.4.0
  • Platform: Windows 10.0.17134, 64-bit
  • Subsystem: don't know

Code to reproduce the issue:

const workspace = JSON.parse('{"orgContentScore":{"41":{"id":"41","contentId":"111","competenceId":"40","scoreTypeId":"6","value":0.25}}}');
const selectedObject = Object.values(workspace.orgContentScore).filter(ocs => ocs.contentId === '111').find(ocs => ocs.competenceId === '40');
console.log(selectedObject.value, 0.25); // Note: not mutated (yet)
console.log({...selectedObject, value: 0.9}.value, 0.9); // Note: This mutates selectedObject on node@11 but not on node@10
console.log(selectedObject.value, 0.25); // Note: selected objects is now mutated!!

On node@11 this outputs this unexpected result:

0.25 0.25
0.9 0.9
0.9 0.25

Where on node@10, it outputs the expected result:

0.25 0.25
0.9 0.9
0.25 0.25

according to my understanding of the description on MDN:

@larsthorup
Copy link
Author

This appears to me to be related to JSON.parse(), as the code works correctly with this line instead of the first line:

const workspace = {"orgContentScore":{"41":{"id":"41","contentId":"111","competenceId":"40","scoreTypeId":"6","value":0.25}}};

@BridgeAR BridgeAR added confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency. labels Dec 17, 2018
@BridgeAR
Copy link
Member

I reduced the test case and reported it to the @nodejs/v8 team:

const weird = JSON.parse('{"a":0,"b":1,"c":2,"d":3,"e":0.1}');
({...weird, e: 666})
console.assert(weird.e === 0.1);

@BridgeAR
Copy link
Member

See https://bugs.chromium.org/p/v8/issues/detail?id=8601. I provided some further details of what I found when looking into this in that issue.

@targos
Copy link
Member

targos commented Dec 17, 2018

It is fixed in V8 7.2.502.4 (from #24875)

@caitp
Copy link
Contributor

caitp commented Dec 18, 2018

Relevant commits which fixed this:
v8/v8@bf84766,
v8/v8@3e010af

@BridgeAR
Copy link
Member

@caitp it seems like the change depends on some other commits. Do you think it would be possible to backport your change?

@caitp
Copy link
Contributor

caitp commented Dec 18, 2018

I could, but I’m on holidays — but, if you feel like taking on fixing it yourself, I’m happy to answer questions.

Otherwise, the folks assigned on the v8 bug will likely get to it before I do.

@hashseed
Copy link
Member

@GeorgNeis helpfully provided a patch to V8 7.1 and resolved merge conflicts here. Would anybody be willing to apply this patch to the right branch in Node.js?

@BridgeAR
Copy link
Member

BridgeAR commented Dec 20, 2018

@hashseed I updated V8 to the mentioned V8 version first and applied the patch afterwards but it still can't compile :/

I missed something when comparing the patch.

@danbev danbev closed this as completed in a981214 Dec 21, 2018
danbev pushed a commit that referenced this issue Dec 21, 2018
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers instead of referencing them

    Adds a helper macro "CloneIfMutablePrimitive", which tests if the
    operand is a MutableHeapNumber, and if so, clones it, otherwise
    returning the original value.

    Also modifies the signature of "CopyPropertyArrayValues" to take a
    "DestroySource" enum, indicating whether or not the resulting object is
    supplanting the source object or not, and removes all default
    parameters from that macro (which were not used anyways).

    This corrects the issue reported in chromium:901301, where
    StaNamedOwnProperty was replacing the value of a MutableHeapNumber
    referenced by both the cloned object and the source object.

    BUG=chromium:901301, v8:7611
    [email protected], [email protected]

    Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629
    Reviewed-on: https://chromium-review.googlesource.com/c/1318096
    Commit-Queue: Caitlin Potter <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#57304}

PR-URL: #25101
Refs: v8/v8@bf84766
Fixes: #25089
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
danbev pushed a commit that referenced this issue Dec 21, 2018
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers only if !FLAG_unbox_double_fields

    Change the macros added in bf84766a2cd3e09070adcd6228a3a487c8dc4bbd to
    only do the hard work if FLAG_unbox_double_fields is unset (otherwise,
    they will attempt to dereference raw float64s, which is bad!)

    Also adds a write barrier in CopyPropertyArrayValues for each store if
    it's possible that a MutableHeapNumber is cloned.

    BUG=chromium:901301, chromium:902965, chromium:903070, v8:7611
    [email protected], [email protected], [email protected]

    Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb
    Reviewed-on: https://chromium-review.googlesource.com/c/1323911
    Commit-Queue: Caitlin Potter <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Igor Sheludko <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#57368}

PR-URL: #25101
Refs: v8/v8@3e010af
Fixes: #25089
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
@targos targos added the v11.x label Jan 1, 2019
targos pushed a commit that referenced this issue Jan 1, 2019
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers instead of referencing them

    Adds a helper macro "CloneIfMutablePrimitive", which tests if the
    operand is a MutableHeapNumber, and if so, clones it, otherwise
    returning the original value.

    Also modifies the signature of "CopyPropertyArrayValues" to take a
    "DestroySource" enum, indicating whether or not the resulting object is
    supplanting the source object or not, and removes all default
    parameters from that macro (which were not used anyways).

    This corrects the issue reported in chromium:901301, where
    StaNamedOwnProperty was replacing the value of a MutableHeapNumber
    referenced by both the cloned object and the source object.

    BUG=chromium:901301, v8:7611
    [email protected], [email protected]

    Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629
    Reviewed-on: https://chromium-review.googlesource.com/c/1318096
    Commit-Queue: Caitlin Potter <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#57304}

PR-URL: #25101
Refs: v8/v8@bf84766
Fixes: #25089
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
targos pushed a commit that referenced this issue Jan 1, 2019
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers only if !FLAG_unbox_double_fields

    Change the macros added in bf84766a2cd3e09070adcd6228a3a487c8dc4bbd to
    only do the hard work if FLAG_unbox_double_fields is unset (otherwise,
    they will attempt to dereference raw float64s, which is bad!)

    Also adds a write barrier in CopyPropertyArrayValues for each store if
    it's possible that a MutableHeapNumber is cloned.

    BUG=chromium:901301, chromium:902965, chromium:903070, v8:7611
    [email protected], [email protected], [email protected]

    Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb
    Reviewed-on: https://chromium-review.googlesource.com/c/1323911
    Commit-Queue: Caitlin Potter <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Igor Sheludko <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#57368}

PR-URL: #25101
Refs: v8/v8@3e010af
Fixes: #25089
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
PR-URL: nodejs#25101
Refs: v8/v8@7.1.302.28...7.1.302.33
Fixes: nodejs#25089
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers instead of referencing them

    Adds a helper macro "CloneIfMutablePrimitive", which tests if the
    operand is a MutableHeapNumber, and if so, clones it, otherwise
    returning the original value.

    Also modifies the signature of "CopyPropertyArrayValues" to take a
    "DestroySource" enum, indicating whether or not the resulting object is
    supplanting the source object or not, and removes all default
    parameters from that macro (which were not used anyways).

    This corrects the issue reported in chromium:901301, where
    StaNamedOwnProperty was replacing the value of a MutableHeapNumber
    referenced by both the cloned object and the source object.

    BUG=chromium:901301, v8:7611
    [email protected], [email protected]

    Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629
    Reviewed-on: https://chromium-review.googlesource.com/c/1318096
    Commit-Queue: Caitlin Potter <[email protected]>
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#57304}

PR-URL: nodejs#25101
Refs: v8/v8@bf84766
Fixes: nodejs#25089
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers only if !FLAG_unbox_double_fields

    Change the macros added in bf84766a2cd3e09070adcd6228a3a487c8dc4bbd to
    only do the hard work if FLAG_unbox_double_fields is unset (otherwise,
    they will attempt to dereference raw float64s, which is bad!)

    Also adds a write barrier in CopyPropertyArrayValues for each store if
    it's possible that a MutableHeapNumber is cloned.

    BUG=chromium:901301, chromium:902965, chromium:903070, v8:7611
    [email protected], [email protected], [email protected]

    Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb
    Reviewed-on: https://chromium-review.googlesource.com/c/1323911
    Commit-Queue: Caitlin Potter <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Igor Sheludko <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#57368}

PR-URL: nodejs#25101
Refs: v8/v8@3e010af
Fixes: nodejs#25089
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants