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

Serious Bug in v10.x.x : Function Map.values truncates elements #27882

Closed
Tunga37 opened this issue May 26, 2019 · 1 comment
Closed

Serious Bug in v10.x.x : Function Map.values truncates elements #27882

Tunga37 opened this issue May 26, 2019 · 1 comment
Labels
util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency.

Comments

@Tunga37
Copy link

Tunga37 commented May 26, 2019

Version: v10.15.3 (10.x.x)
Platform: Linux 4.15.0-50-generic #54-Ubuntu SMP Mon May 6 18:46:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

This is the code that demonstrates the problem

const m = new Map();
m.set(1,'a');
m.set(2,'b');
m.set(3,'c');
m.set(4,'d');
m.set(5,'e');
m.set(6,'f');

//before delete [Map Iterator] { 'a', 'b', 'c', 'd', 'e', 'f' }
console.log('before delete',m.values());

m.delete(1);

//after delete [Map Iterator] { 'c', 'd', 'e', 'f' }
console.log('after delete',m.values());

//element 'b' has been truncated

It's probably more v8 problem than node itself (in node v12.x.x this is fixed) but the truth is ... this is very serious bug in node v10.x.x and I had to jump out of version 10.x.x because of this.

@Tunga37 Tunga37 changed the title Function Map.values truncates elements Serious Bug in v10.x.x : Function Map.values truncates elements May 26, 2019
@addaleax addaleax added util Issues and PRs related to the built-in util module. v10.x v8 engine Issues and PRs related to the V8 dependency. labels May 26, 2019
@addaleax
Copy link
Member

I think this was fixed in 0afcb9a (aka v8/v8@88f8fe1). I’ll attempt a backport, that seems doable to me.

I don’t think this is a super-serious bug – to be clear, what is broken is the display of m.values() when using console.log or util.inspect(), not the actual functionality of m.values(), as far as I can tell.

addaleax pushed a commit to addaleax/node that referenced this issue May 26, 2019
Original commit message:

    Fix collection iterator preview with deleted entries

    We used to assume that we know the remaining entries returned by the
    iterator based on the current index. However, that is not accurate,
    since entries skipped by the current index could be deleted.

    In the new approach, we allocate conservatively and shrink the result.

    R=neis@chromium.org

    Bug: v8:8433
    Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8
    Reviewed-on: https://chromium-review.googlesource.com/c/1325966
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57360}

[The backport to v10.x resolves merge conflicts due to a different way
of accessing the “hole” value in V8, different signatures of the
`Handle` constructor and the `Shrink()` method, and neighbouring-line
conflicts in the test file.]

Refs: v8/v8@88f8fe1
Fixes: nodejs#27882
PR-URL: nodejs#24514
Refs: nodejs#24053
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
BethGriggs pushed a commit that referenced this issue Jun 6, 2019
Original commit message:

    Fix collection iterator preview with deleted entries

    We used to assume that we know the remaining entries returned by the
    iterator based on the current index. However, that is not accurate,
    since entries skipped by the current index could be deleted.

    In the new approach, we allocate conservatively and shrink the result.

    R=neis@chromium.org

    Bug: v8:8433
    Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8
    Reviewed-on: https://chromium-review.googlesource.com/c/1325966
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Georg Neis <neis@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57360}

[The backport to v10.x resolves merge conflicts due to a different way
of accessing the “hole” value in V8, different signatures of the
`Handle` constructor and the `Shrink()` method, and neighbouring-line
conflicts in the test file.]

Refs: v8/v8@88f8fe1
Fixes: #27882

Backport-PR-URL: #27894
PR-URL: #24514
Refs: #24053
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@Tunga37 Tunga37 closed this as completed Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

2 participants