Skip to content

deps: backport 0d01728 from v8's upstream #2912

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

deps: backport 0d01728 from v8's upstream #2912

wants to merge 1 commit into from

Conversation

@indutny
Copy link
Member

@indutny indutny commented Sep 16, 2015

Original commit message:

[objects] do not visit ArrayBuffer's backing store

ArrayBuffer's backing store is a pointer to external heap, and
can't be treated as a heap object. Doing so will result in
crashes, when the backing store is unaligned.

See: https://github.com/nodejs/node/issues/2791

BUG=chromium:530531
R=mlippautz@chromium.org
LOG=N

Review URL: https://codereview.chromium.org/1327403002

Cr-Commit-Position: refs/heads/master@{#30771}

Fix: #2791

cc @Fishrock123 @trevnorris @nodejs/v8

@indutny
deps: backport 0d01728 from v8's upstream
58e0127 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: nodejs#2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{nodejs#30771}

Fix: nodejs#2791
@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Sep 16, 2015
bnoordhuis
bnoordhuis reviewed Sep 16, 2015
View reviewed changes
deps/v8/src/heap/mark-compact.cc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kStartOffset property is inherited from JSObject, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

@Fishrock123
Copy link
Contributor

I don't feel qualified to review this, but Rubberstamp-LGTM if others sign off.

bnoordhuis
bnoordhuis reviewed Sep 16, 2015
View reviewed changes
deps/v8/src/heap/mark-compact.cc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I read this right, it basically iterates over the arraybuffer's properties skipping the kBitFieldSlot and kBitFieldOffset fields? Seems awfully implicit. (EDIT: And evidently it results in redundancy.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis yeah, this is what v8 team advised me to do...

@bnoordhuis
Copy link
Member

LGTM although it's not the approach I would have taken. The test is clever though.

@indutny
Copy link
Member Author

indutny commented Sep 16, 2015

@bnoordhuis I'd rather move the backing store pointer down, but idk

@bnoordhuis bnoordhuis mentioned this pull request Sep 16, 2015
Merged
@indutny
Copy link
Member Author

indutny commented Sep 16, 2015

CI: https://ci.nodejs.org/job/node-test-pull-request/323/

@trevnorris
Copy link
Contributor

Change looks good, but I'm not qualified to fully sign off.

@indutny
Copy link
Member Author

indutny commented Sep 16, 2015

Looks like CI is a bit borked? cc @rvagg

@Fishrock123
Copy link
Contributor

New run: https://ci.nodejs.org/job/node-test-pull-request/328/

@indutny
Copy link
Member Author

indutny commented Sep 16, 2015

One more CI: https://ci.nodejs.org/job/node-test-pull-request/331/

@Fishrock123
Copy link
Contributor

@indutny CI also seems more or less fine. Maybe wait for armv7.

@indutny
Copy link
Member Author

indutny commented Sep 17, 2015

@Fishrock123 please land the thing!

@indutny
Copy link
Member Author

indutny commented Sep 17, 2015

Landed in 2b8a06b, thank you!

indutny added a commit that referenced this pull request Sep 17, 2015
@indutny
deps: backport 0d01728 from v8's upstream
2b8a06b 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: #2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{#30771}

Fix: #2791
PR-URL: #2912
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@indutny indutny closed this Sep 17, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 17, 2015
@indutny @Fishrock123
deps: backport 0d01728 from v8's upstream
94972d5 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: nodejs#2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{nodejs#30771}

Fix: nodejs#2791
PR-URL: nodejs#2912
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
TooTallNate added a commit to node-ffi/node-ffi that referenced this pull request Sep 18, 2015
@TooTallNate
travis: test node v4.1
c0b6441 
Needed for nodejs/node#2912.
See #238 and #239.
@rvagg rvagg mentioned this pull request Sep 22, 2015
Merged
indutny added a commit that referenced this pull request Sep 29, 2015
@indutny
deps: backport 0d01728 from v8's upstream
9eabb0a 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: #2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{#30771}

Fix: #2791
PR-URL: #2912
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit to targos/node that referenced this pull request Oct 5, 2015
@indutny @targos
deps: backport 0d01728 from v8's upstream
9c43d99 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: nodejs#2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{nodejs#30771}

Fix: nodejs#2791
PR-URL: nodejs#2912
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit to targos/node that referenced this pull request Oct 6, 2015
@indutny @targos
deps: backport 0d01728 from v8's upstream
bdfe354 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: nodejs#2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{nodejs#30771}

Fix: nodejs#2791
PR-URL: nodejs#2912
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
indutny added a commit that referenced this pull request Oct 8, 2015
@indutny @targos
deps: backport 0d01728 from v8's upstream
9c6fd4f 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: #2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{#30771}

Fix: #2791
PR-URL: #2912
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
indutny added a commit that referenced this pull request Oct 11, 2015
@indutny @targos
deps: backport 0d01728 from v8's upstream
d0514dc 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: #2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{#30771}

Fix: #2791
PR-URL: #2912
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Oct 14, 2015
@indutny @ofrobots
deps: backport 0d01728 from v8's upstream
dcfa6aa 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: nodejs#2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{nodejs#30771}

Ref: nodejs#2791
Ref: nodejs#2912
PR-URL: nodejs#3351
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
indutny added a commit that referenced this pull request Oct 14, 2015
@indutny @ofrobots
deps: backport 0d01728 from v8's upstream
972a0c8 
Original commit message:

    [objects] do not visit ArrayBuffer's backing store

    ArrayBuffer's backing store is a pointer to external heap, and
    can't be treated as a heap object. Doing so will result in
    crashes, when the backing store is unaligned.

    See: #2791

    BUG=chromium:530531
    R=mlippautz@chromium.org
    LOG=N

    Review URL: https://codereview.chromium.org/1327403002

    Cr-Commit-Position: refs/heads/master@{#30771}

Ref: #2791
Ref: #2912
PR-URL: #3351
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

landed in lts-v4.x-staging as 94972d5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@indutny @Fishrock123 @bnoordhuis @trevnorris @MylesBorins @mscdex @jasnell