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

indutny · 2015-09-16T17:59:16Z

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

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

bnoordhuis · 2015-09-16T18:09:16Z

deps/v8/src/heap/mark-compact.cc

+
+    // Visit inherited JSObject properties and byte length of ArrayBuffer
+    Address regular_slot =
+        dst->address() + JSArrayBuffer::BodyDescriptor::kStartOffset;


The kStartOffset property is inherited from JSObject, right?

Fishrock123 · 2015-09-16T18:10:35Z

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

bnoordhuis · 2015-09-16T18:12:40Z

deps/v8/src/heap/mark-compact.cc

+      RecordMigratedSlot(Memory::Object_at(internal_field_slot),
+                         internal_field_slot);
+      internal_field_slot += kPointerSize;
+    }


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.)

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

bnoordhuis · 2015-09-16T18:15:10Z

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

indutny · 2015-09-16T18:17:26Z

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

indutny · 2015-09-16T19:39:10Z

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

trevnorris · 2015-09-16T19:48:41Z

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

indutny · 2015-09-16T20:54:13Z

Looks like CI is a bit borked? cc @rvagg

Fishrock123 · 2015-09-16T21:47:31Z

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

indutny · 2015-09-16T21:57:41Z

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

Fishrock123 · 2015-09-16T23:12:48Z

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

indutny · 2015-09-17T00:03:55Z

@Fishrock123 please land the thing!

indutny · 2015-09-17T00:35:16Z

Landed in 2b8a06b, thank you!

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>

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>

Needed for nodejs/node#2912. See #238 and #239.

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>

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>

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>

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>

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 · 2015-11-16T19:13:55Z

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

mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Sep 16, 2015

indutny added the land-on-v4.x label Sep 16, 2015

bnoordhuis reviewed Sep 16, 2015
View reviewed changes

bnoordhuis mentioned this pull request Sep 16, 2015

Upgrade to nan 2.0.x strongloop/strong-oracle#51

Merged

indutny closed this Sep 17, 2015

TooTallNate added a commit to node-ffi/node-ffi that referenced this pull request Sep 18, 2015

travis: test node v4.1

c0b6441

Needed for nodejs/node#2912. See #238 and #239.

rvagg mentioned this pull request Sep 22, 2015

Release proposal: v4.1.1 #2995

Merged

jasnell added the lts-landed-on-v4.x label Oct 7, 2015

MylesBorins removed the land-on-v4.x label Nov 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

indutny commented Sep 16, 2015

bnoordhuis Sep 16, 2015

indutny Sep 16, 2015

Fishrock123 commented Sep 16, 2015

bnoordhuis Sep 16, 2015

indutny Sep 16, 2015

bnoordhuis commented Sep 16, 2015

indutny commented Sep 16, 2015

indutny commented Sep 16, 2015

trevnorris commented Sep 16, 2015

indutny commented Sep 16, 2015

Fishrock123 commented Sep 16, 2015

indutny commented Sep 16, 2015

Fishrock123 commented Sep 16, 2015

indutny commented Sep 17, 2015

indutny commented Sep 17, 2015

MylesBorins commented Nov 16, 2015

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

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

Conversation

indutny commented Sep 16, 2015

bnoordhuis Sep 16, 2015

Choose a reason for hiding this comment

indutny Sep 16, 2015

Choose a reason for hiding this comment

Fishrock123 commented Sep 16, 2015

bnoordhuis Sep 16, 2015

Choose a reason for hiding this comment

indutny Sep 16, 2015

Choose a reason for hiding this comment

bnoordhuis commented Sep 16, 2015

indutny commented Sep 16, 2015

indutny commented Sep 16, 2015

trevnorris commented Sep 16, 2015

indutny commented Sep 16, 2015

Fishrock123 commented Sep 16, 2015

indutny commented Sep 16, 2015

Fishrock123 commented Sep 16, 2015

indutny commented Sep 17, 2015

indutny commented Sep 17, 2015

MylesBorins commented Nov 16, 2015