[objects] do not visit ArrayBuffer's backing store

[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

Committed: https://crrev.com/0d017282d32ce634f364461aa79ee996108f8b9d
Cr-Commit-Position: refs/heads/master@{#30771}

[fedor.indutny, 2015-09-10 21:25:17]

Hello again!

One more CL from me. I hope you don't mind! ;)

I'm not sure if this patch covers all edge cases, especially if the backing
store could still be visited from the layout descriptor.

Does ArrayBuffer have LayoutDescriptor?

Thanks!

[Michael Lippautz, 2015-09-11 11:05:22]

mlippautz@chromium.org changed reviewers:
+ jochen@chromium.org

[Michael Lippautz, 2015-09-11 11:05:22]

+jochen

I added a tracking bug for this issue. Will maybe have a look today or starting
Monday.

-Michael

[Michael Lippautz, 2015-09-11 14:36:24]

Thanks for digging into this, but I think there are some problems.

The main problem I see is that we also have to deal with non-externalized
buffers. This is completely missing from the specialized visitors.

I leave the main review to Jochen, as he knows more about ArrayBuffers.

[fedor.indutny, 2015-09-11 20:35:05]

Oh yeah, I forgot about it. Looking forward for full review!

Thanks.

[fedor.indutny, 2015-09-14 08:57:34]

Hello!

Just wanted to let you know that this is very critical for node.js . There are
many C++ modules that are currently crashing because of this.

Thank you very much,
Fedor.

[jochen (gone - plz use gerrit), 2015-09-14 10:04:36]

Object::ContentType needs to return kMixedValues for array buffers, and all
call-sites of content type need to be updated to deal with array buffers.

https://codereview.chromium.org/1327403002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1327403002/diff/1/src/objects-inl.h#newcode6594
src/objects-inl.h:6594: HeapObject::RawField(obj,
JSArrayBuffer::BodyDescriptor::kStartOffset),
I wouldn't use constants here, just visit the length and the internal fields

https://codereview.chromium.org/1327403002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1327403002/diff/1/src/objects.h#newcode9679
src/objects.h:9679: class BodyDescriptor {
instead of having a body description, just add two methods called
JSArrayBufferIterateBody() to JSArrayBuffer (similar to what FixedTypedArrayBase
has)

[fedor.indutny, 2015-09-14 17:08:16]

All fixed. Thanks!

https://codereview.chromium.org/1327403002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1327403002/diff/1/src/objects-inl.h#newcode6594
src/objects-inl.h:6594: HeapObject::RawField(obj,
JSArrayBuffer::BodyDescriptor::kStartOffset),
On 2015/09/14 10:04:36, jochen wrote:
> I wouldn't use constants here, just visit the length and the internal fields

Acknowledged.

https://codereview.chromium.org/1327403002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1327403002/diff/1/src/objects.h#newcode9679
src/objects.h:9679: class BodyDescriptor {
On 2015/09/14 10:04:36, jochen wrote:
> instead of having a body description, just add two methods called
> JSArrayBufferIterateBody() to JSArrayBuffer (similar to what
FixedTypedArrayBase
> has)

Acknowledged.

[jochen (gone - plz use gerrit), 2015-09-15 07:41:20]

lgtm

[fedor.indutny, 2015-09-15 17:32:26]

The CQ bit was checked by fedor@indutny.com

[commit-bot: I haz the power, 2015-09-15 17:32:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327403002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327403002/20001

[commit-bot: I haz the power, 2015-09-15 17:36:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-15 17:36:10]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5806)

[fedor.indutny, 2015-09-15 17:40:21]

Jochen,

Looks like it is missing some more LGTMs:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    src/heap/mark-compact.cc
    src/heap/objects-visiting-inl.h
    src/heap/objects-visiting.cc
    src/heap/store-buffer.cc

[fedor.indutny, 2015-09-15 17:47:07]

Going to try it one more time with removed unused variable, just in case...

[fedor.indutny, 2015-09-15 17:47:31]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-09-15 17:47:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1327403002/#ps40001
(title: "remove unused var in test")

[commit-bot: I haz the power, 2015-09-15 17:47:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327403002/40001

[commit-bot: I haz the power, 2015-09-15 17:50:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-15 17:50:43]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5808)

[fedor.indutny, 2015-09-15 17:52:45]

Yeah, the same. We need more LGTMs :) Thank you!

[fedor.indutny, 2015-09-15 22:25:31]

Looks like there was a debug build failure because I didn't visit all the
JSObject fields. This is fixed now, running CQ just to verify that it no longer
fails on a buildbot (it should not commit the change anyway, so I assume that it
is safe to trigger the CQ).

[fedor.indutny, 2015-09-15 22:25:36]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-09-15 22:25:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1327403002/#ps60001
(title: "visit all fields in array buffer, reorder fields for simplicity")

[commit-bot: I haz the power, 2015-09-15 22:25:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327403002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327403002/60001

[commit-bot: I haz the power, 2015-09-15 22:29:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-15 22:29:34]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5812)

[fedor.indutny, 2015-09-15 23:14:44]

All green now, please take a look. Thanks!

[jochen (gone - plz use gerrit), 2015-09-16 11:22:48]

still lgtm

[fedor.indutny, 2015-09-16 17:25:44]

The CQ bit was checked by fedor@indutny.com

[commit-bot: I haz the power, 2015-09-16 17:25:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327403002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327403002/60001

[commit-bot: I haz the power, 2015-09-16 17:27:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-16 17:28:17]

Patchset 4 (id:??) landed as
https://crrev.com/0d017282d32ce634f364461aa79ee996108f8b9d
Cr-Commit-Position: refs/heads/master@{#30771}

[fedor.indutny, 2015-09-16 17:34:31]

Not sure why it worked now, but hooray! :) Thank you.