Fix bug when transferring SharedArrayBuffer to multiple Workers.

Fix bug when transferring SharedArrayBuffer to multiple Workers.

Previously, the serialization code would call Externalize for every transferred
ArrayBuffer or SharedArrayBuffer, but that function can only be called once. If
the buffer is already externalized, we should call GetContents instead.

Also fix use-after-free bug when transferring ArrayBuffers. The transferred
ArrayBuffer must be internalized in the new isolate, or be managed by the
Shell. The current code gives it to the isolate externalized and frees it
immediately afterward when the SerializationData object is destroyed.

BUG=chromium:497295
R=jarin@chromium.org
LOG=n

Committed: https://crrev.com/dd7962bf7838f8379ba776ee6b7b0e4d3bec2140
Cr-Commit-Position: refs/heads/master@{#29499}

[Jarin, 2015-07-02 11:04:32]

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

[Jarin, 2015-07-02 11:04:41]

On 2015/07/01 18:52:54, binji wrote:

Hmm, now I am a bit confused about lifetime management of the shared array
buffer contents. Why does the worker have to do explicit management of the array
buffer contents? I thought there would be some reference counting on buffers
that would magically take care of this. Or is this taking care of the buffers
that were not collected by the GC?

Adding Jochen who might be less confused than me.

[binji, 2015-07-02 17:09:11]

On 2015/07/02 at 11:04:41, jarin wrote:
> On 2015/07/01 18:52:54, binji wrote:
> 
> Hmm, now I am a bit confused about lifetime management of the shared array
buffer contents. Why does the worker have to do explicit management of the array
buffer contents? I thought there would be some reference counting on buffers
that would magically take care of this. Or is this taking care of the buffers
that were not collected by the GC?
> 
> Adding Jochen who might be less confused than me.

The ArrayBuffer (all of this applies to SharedArrayBuffers too) object is always
owned by the isolate that created it. And the memory is always allocated using
the ArrayBuffer::Allocator. But whether the memory is free'd when the object is
garbage collected depends on whether the ArrayBuffer is externalized. AIUI, most
ArrayBuffers start out as non-externalized. When they're transferred to another
isolate, they're externalized which basically means that the isolate that owns
the object shouldn't clean up the memory too. At this point, some higher-level
system has to manage cleaning up the memory that is shared between isolates. In
Blink this is a ref-counted structure, that is attached to the DOM wrapper
objects. In d8, it seemed simpler to manage that memory directly in the Shell
class or the Worker.

Ah, but it looks like the ArrayBuffer::New static constructor can specify that
the array buffer should be created non-externalized. I'll give that a shot,
because it means that the worker won't have to manage the memory.
SharedArrayBuffer will still have to be managed by the Shell, however.

[jochen (gone - plz use gerrit), 2015-07-02 17:52:39]

On 2015/07/02 at 17:09:11, binji wrote:
> On 2015/07/02 at 11:04:41, jarin wrote:
> > On 2015/07/01 18:52:54, binji wrote:
> > 
> > Hmm, now I am a bit confused about lifetime management of the shared array
buffer contents. Why does the worker have to do explicit management of the array
buffer contents? I thought there would be some reference counting on buffers
that would magically take care of this. Or is this taking care of the buffers
that were not collected by the GC?
> > 
> > Adding Jochen who might be less confused than me.
> 
> The ArrayBuffer (all of this applies to SharedArrayBuffers too) object is
always owned by the isolate that created it. And the memory is always allocated
using the ArrayBuffer::Allocator. But whether the memory is free'd when the
object is garbage collected depends on whether the ArrayBuffer is externalized.
AIUI, most ArrayBuffers start out as non-externalized. When they're transferred
to another isolate, they're externalized which basically means that the isolate
that owns the object shouldn't clean up the memory too. At this point, some
higher-level system has to manage cleaning up the memory that is shared between
isolates. In Blink this is a ref-counted structure, that is attached to the DOM
wrapper objects. In d8, it seemed simpler to manage that memory directly in the
Shell class or the Worker.
> 
> Ah, but it looks like the ArrayBuffer::New static constructor can specify that
the array buffer should be created non-externalized. I'll give that a shot,
because it means that the worker won't have to manage the memory.
SharedArrayBuffer will still have to be managed by the Shell, however.

the non-externalized ctor can't be used with shared array buffers, as that would
result in double-frees

[binji, 2015-07-02 17:54:28]

> > Ah, but it looks like the ArrayBuffer::New static constructor can specify
that the array buffer should be created non-externalized. I'll give that a shot,
because it means that the worker won't have to manage the memory.
SharedArrayBuffer will still have to be managed by the Shell, however.
> 
> the non-externalized ctor can't be used with shared array buffers, as that
would result in double-frees

Right, I only use it for transferring ArrayBuffers, not SharedArrayBuffers. SABs
still need to have their memory managed externally.

[jochen (gone - plz use gerrit), 2015-07-03 14:29:38]

https://codereview.chromium.org/1215233004/diff/40001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1215233004/diff/40001/src/d8.cc#newcode1542
src/d8.cc:1542: if (contents.Data()) {
how can data ever be null?

[Jarin, 2015-07-06 05:04:19]

lgtm.

(Even though at some point we might want to implement reference counting for
shared array buffer contents because the current approach leaks memory.)

https://codereview.chromium.org/1215233004/diff/40001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1215233004/diff/40001/src/d8.cc#newcode1542
src/d8.cc:1542: if (contents.Data()) {
On 2015/07/03 14:29:37, jochen wrote:
> how can data ever be null?

As the comment above says, ReadArrayBufferContents can set the contents to empty
(line 1600). No?

[binji, 2015-07-06 06:23:27]

https://codereview.chromium.org/1215233004/diff/40001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1215233004/diff/40001/src/d8.cc#newcode1542
src/d8.cc:1542: if (contents.Data()) {
On 2015/07/06 at 05:04:19, jarin wrote:
> On 2015/07/03 14:29:37, jochen wrote:
> > how can data ever be null?
> 
> As the comment above says, ReadArrayBufferContents can set the contents to
empty (line 1600). No?

Yes, that's correct.

[jochen (gone - plz use gerrit), 2015-07-06 06:32:25]

On 2015/07/06 at 06:23:27, binji wrote:
> https://codereview.chromium.org/1215233004/diff/40001/src/d8.cc
> File src/d8.cc (right):
> 
> https://codereview.chromium.org/1215233004/diff/40001/src/d8.cc#newcode1542
> src/d8.cc:1542: if (contents.Data()) {
> On 2015/07/06 at 05:04:19, jarin wrote:
> > On 2015/07/03 14:29:37, jochen wrote:
> > > how can data ever be null?
> > 
> > As the comment above says, ReadArrayBufferContents can set the contents to
empty (line 1600). No?
> 
> Yes, that's correct.

if the current approach leaks memory, lsan bots will be unhappy, so we'll have
to fix that soonish.

anyway, lgtm

[binji, 2015-07-06 06:34:47]

> if the current approach leaks memory, lsan bots will be unhappy, so we'll have
to fix that soonish.

Well, AFAIK it doesn't leak memory. It just doesn't free the SAB memory until
the d8 shell is destroyed, rather than when there are no remaining references to
the SAB.

[Jarin, 2015-07-06 07:02:30]

On 2015/07/06 06:34:47, binji wrote:
> > if the current approach leaks memory, lsan bots will be unhappy, so we'll
have
> to fix that soonish.
> 
> Well, AFAIK it doesn't leak memory. It just doesn't free the SAB memory until
> the d8 shell is destroyed, rather than when there are no remaining references
to
> the SAB.

What I mean is that the SABs will be only collected when the shell terminates.
So if you create lot of short-lived SABs you can run out of memory even though
the live memory is small. No?

[binji, 2015-07-06 16:53:24]

The CQ bit was checked by binji@chromium.org

[commit-bot: I haz the power, 2015-07-06 16:53:38]

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

[binji, 2015-07-06 16:58:19]

On 2015/07/06 at 07:02:30, jarin wrote:
> On 2015/07/06 06:34:47, binji wrote:
> > > if the current approach leaks memory, lsan bots will be unhappy, so we'll
have
> > to fix that soonish.
> > 
> > Well, AFAIK it doesn't leak memory. It just doesn't free the SAB memory
until
> > the d8 shell is destroyed, rather than when there are no remaining
references to
> > the SAB.
> 
> What I mean is that the SABs will be only collected when the shell terminates.
So if you create lot of short-lived SABs you can run out of memory even though
the live memory is small. No?

Yes, you're right. But those aren't the types of leaks that will be found by
lsan (it only does a leak check at program exit), so we shouldn't have to worry
about those bots.

[commit-bot: I haz the power, 2015-07-06 17:18:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-06 17:18:14]

Patchset 3 (id:??) landed as
https://crrev.com/dd7962bf7838f8379ba776ee6b7b0e4d3bec2140
Cr-Commit-Position: refs/heads/master@{#29499}

[Michael Achenbach, 2015-07-06 20:21:54]

The test is sometimes hanging and timing out, e.g.:
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20isolates/buil...

[binji, 2015-07-06 20:52:52]

On 2015/07/06 at 20:21:54, machenbach wrote:
> The test is sometimes hanging and timing out, e.g.:
>
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20isolates/buil...

I'll take a look.

[Michael Achenbach, 2015-07-07 06:40:25]

On 2015/07/06 20:52:52, binji wrote:
> On 2015/07/06 at 20:21:54, machenbach wrote:
> > The test is sometimes hanging and timing out, e.g.:
> >
>
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20isolates/buil...
> 
> I'll take a look.

I'll revert to keep the tree green.

[Michael Achenbach, 2015-07-07 06:41:07]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1224843008/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Test hangs sometimes and times out
flakily. E.g.:
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosse3/builds....