Adds support for ArrayBufferContents with external buffer.

Adds support for ArrayBufferContents with external buffer.

The current implementation of WTF::ArrayBuffer assumes that underlying
buffer is always owned by the ArrayBuffer. This patch teaches
ArrayBufferContents about external buffers that are not owned by the
ArrayBuffer.

This support is need to implement JS bindings for mojo shared buffer
where a buffer owned by mojo needs to be exposed as an ArrayBuffer. More
details here: https://codereview.chromium.org/2720873002/

BUG=647036
Review-Url: https://codereview.chromium.org/2719883004
Cr-Commit-Position: refs/heads/master@{#453861}
Committed: https://chromium.googlesource.com/chromium/src/+/cce921f5c9a04fa1d7aa70075c70678e3504ef49

[alokp, 2017-02-27 23:58:23]

alokp@chromium.org changed reviewers:
+ jbroman@chromium.org

[alokp, 2017-02-28 00:19:50]

Description was changed from

==========
Adds support for ArrayBufferContents with external buffer.

The current implementation of WTF::ArrayBuffer assumes that underlying
buffers are always owned by the buffer. This patch teaches
ArrayBufferContents about external buffers that are not owned by the
ArrayBuffer.

This support is need to implement JS bindings for mojo shared buffer
where a buffer owned by mojo needs to be exposed as an ArrayBuffer. More
details here: https://codereview.chromium.org/2720873002/

BUG=647036
==========

to

==========
Adds support for ArrayBufferContents with external buffer.

The current implementation of WTF::ArrayBuffer assumes that underlying
buffer is always owned by the ArrayBuffer. This patch teaches
ArrayBufferContents about external buffers that are not owned by the
ArrayBuffer.

This support is need to implement JS bindings for mojo shared buffer
where a buffer owned by mojo needs to be exposed as an ArrayBuffer. More
details here: https://codereview.chromium.org/2720873002/

BUG=647036
==========

[alokp, 2017-02-28 01:00:28]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 01:01:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-02-28 01:28:50]

Huh, not as bad as I'd feared it would be. You'll want an OK from someone who
understands both the WTF and V8 array buffer stuff -- jochen@ would probably be
a good person.

Since you're apparently relying on other stuff to synchronize, a regular
ArrayBuffer (as opposed to SharedArrayBuffer) seems like it would be fine. Also,
double-checking: buffers mapped in this way are always both readable and
writable, right?

There's no way to make a "read-only" array buffer (AFAIK), so if that's
something you'd need in the future, then we'd have to look at some other way of
exposing this concept.

https://codereview.chromium.org/2719883004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):

https://codereview.chromium.org/2719883004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:68: // -
ArrayBufferContents will not free it upon destruction.
Hmm, so if JavaScript drops the reference on the floor (e.g. the context shuts
down and we don't unmap it first), then the mapping just stays forever?

I think I'd rather have a "free" function by provided here. Something like this:

using FreeCallback = void(*)(void*);

where the default is:
FreeCallback defaultFreeCallback = [](void* data) {
  PartitionFreeGeneric(Partitions::arrayBufferPartition(), data);
};

But for Mojo might be something along the lines of:

FreeCallback mojoSharedBufferFreeCallback = [](void* data) {
  MojoResult result = MojoUnmapBuffer(data);
  DCHECK_EQ(MOJO_RESULT_OK, result);
};

This way, the buffer will be unmapped if either it is explicitly unmapped (I can
see why you'd want the ability to do that), or if the array buffer is no longer
being held by the script.

[commit-bot: I haz the power, 2017-02-28 03:16:18]

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

[commit-bot: I haz the power, 2017-02-28 03:16:19]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-02-28 03:27:34]

Hmm.

Would you help me understand why normal JS bindings don't need to externalize
array buffers but Mojo bindings need to? I'm wondering where the difference
comes from.

[alokp, 2017-02-28 06:28:23]

Mojo supports read-only shared buffer, but the current js bindings use
v8::ArrayBuffer, which as you mention is always read-write. I need to study the
current implementation to check how read-only is being enforced.

https://codereview.chromium.org/2719883004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):

https://codereview.chromium.org/2719883004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:68: // -
ArrayBufferContents will not free it upon destruction.
On 2017/02/28 01:28:50, jbroman wrote:
> Hmm, so if JavaScript drops the reference on the floor (e.g. the context shuts
> down and we don't unmap it first), then the mapping just stays forever?

I was thinking of unmapping the buffer upon destruction of
MojoSharedBufferMapping. See:
https://codereview.chromium.org/2720873002/diff/40001/third_party/WebKit/Sour...

That said, I think I like your proposal better. Injecting a free function would
allow us to return an ArrayBuffer directly instead of wrapping it into a
MojoSharedBufferMapping. I do not think an Unmap function is particularly useful
in JS.

> I think I'd rather have a "free" function by provided here. Something like
this:
> 
> using FreeCallback = void(*)(void*);
> 
> where the default is:
> FreeCallback defaultFreeCallback = [](void* data) {
>   PartitionFreeGeneric(Partitions::arrayBufferPartition(), data);
> };
> 
> But for Mojo might be something along the lines of:
> 
> FreeCallback mojoSharedBufferFreeCallback = [](void* data) {
>   MojoResult result = MojoUnmapBuffer(data);
>   DCHECK_EQ(MOJO_RESULT_OK, result);
> };
> 
> This way, the buffer will be unmapped if either it is explicitly unmapped (I
can
> see why you'd want the ability to do that), or if the array buffer is no
longer
> being held by the script.

[alokp, 2017-02-28 06:32:43]

On 2017/02/28 03:27:34, haraken wrote:
> Hmm.
> 
> Would you help me understand why normal JS bindings don't need to externalize
> array buffers but Mojo bindings need to? I'm wondering where the difference
> comes from.

We need to implement JS bindings for mojo shared buffer functions in
mojo/public/c/system/buffer.h.

MojoMapBuffer returns a handle to a buffer that needs to be wrapped into a
DOMArrayBuffer. This buffer is owned by Mojo, where as DOMArrayBuffer assumes
that the underlying buffer is always internal.

[haraken, 2017-02-28 08:30:30]

On 2017/02/28 06:32:43, alokp wrote:
> On 2017/02/28 03:27:34, haraken wrote:
> > Hmm.
> > 
> > Would you help me understand why normal JS bindings don't need to
externalize
> > array buffers but Mojo bindings need to? I'm wondering where the difference
> > comes from.
> 
> We need to implement JS bindings for mojo shared buffer functions in
> mojo/public/c/system/buffer.h.
> 
> MojoMapBuffer returns a handle to a buffer that needs to be wrapped into a
> DOMArrayBuffer. This buffer is owned by Mojo, where as DOMArrayBuffer assumes
> that the underlying buffer is always internal.

Why does the buffer need to be owned by Mojo? In other words, would it be hard
to make MojoMapBuffer create the underlying buffer using the DOMArrayBuffer (so
that the buffer can be owned by DOMArrayBuffer)?

[jbroman, 2017-02-28 19:05:47]

On 2017/02/28 at 08:30:30, haraken wrote:
> On 2017/02/28 06:32:43, alokp wrote:
> > On 2017/02/28 03:27:34, haraken wrote:
> > > Hmm.
> > > 
> > > Would you help me understand why normal JS bindings don't need to
externalize
> > > array buffers but Mojo bindings need to? I'm wondering where the
difference
> > > comes from.
> > 
> > We need to implement JS bindings for mojo shared buffer functions in
> > mojo/public/c/system/buffer.h.
> > 
> > MojoMapBuffer returns a handle to a buffer that needs to be wrapped into a
> > DOMArrayBuffer. This buffer is owned by Mojo, where as DOMArrayBuffer
assumes
> > that the underlying buffer is always internal.
> 
> Why does the buffer need to be owned by Mojo? In other words, would it be hard
to make MojoMapBuffer create the underlying buffer using the DOMArrayBuffer (so
that the buffer can be owned by DOMArrayBuffer)?

Because this is an IPC primitive, the memory has to be allocated in such a way
that it can be unmapped and sent to another process, or received from another
process and mapped into memory. This means it has to be backed by shared memory,
which PartitionAlloc is (intentionally) not.

The alternatives here are, from my perspective:
(a) allow these exotic array buffers
(b) implement some other JS object which provided a sufficient view into mapped
buffers, but not ArrayBuffer (e.g. methods to copy ranges between an ordinary
ArrayBuffer and a shared memory mapping)

The former is less code and should be more efficient, but it does add this
nuance to our ArrayBuffer code.

One follow-up question I have: is it legal to unmap a Mojo buffer on a different
thread than the one it was originally mapped in? My intuition is yes, but if no
we'll have to ensure that these buffers are not sent to workers.

[alokp, 2017-02-28 19:37:32]

On 2017/02/28 19:05:47, jbroman wrote:
> On 2017/02/28 at 08:30:30, haraken wrote:
> > On 2017/02/28 06:32:43, alokp wrote:
> > > On 2017/02/28 03:27:34, haraken wrote:
> > > > Hmm.
> > > > 
> > > > Would you help me understand why normal JS bindings don't need to
> externalize
> > > > array buffers but Mojo bindings need to? I'm wondering where the
> difference
> > > > comes from.
> > > 
> > > We need to implement JS bindings for mojo shared buffer functions in
> > > mojo/public/c/system/buffer.h.
> > > 
> > > MojoMapBuffer returns a handle to a buffer that needs to be wrapped into a
> > > DOMArrayBuffer. This buffer is owned by Mojo, where as DOMArrayBuffer
> assumes
> > > that the underlying buffer is always internal.
> > 
> > Why does the buffer need to be owned by Mojo? In other words, would it be
hard
> to make MojoMapBuffer create the underlying buffer using the DOMArrayBuffer
(so
> that the buffer can be owned by DOMArrayBuffer)?
> 
> Because this is an IPC primitive, the memory has to be allocated in such a way
> that it can be unmapped and sent to another process, or received from another
> process and mapped into memory. This means it has to be backed by shared
memory,
> which PartitionAlloc is (intentionally) not.
> 
> The alternatives here are, from my perspective:
> (a) allow these exotic array buffers
> (b) implement some other JS object which provided a sufficient view into
mapped
> buffers, but not ArrayBuffer (e.g. methods to copy ranges between an ordinary
> ArrayBuffer and a shared memory mapping)
> 
> The former is less code and should be more efficient, but it does add this
> nuance to our ArrayBuffer code.
> 
> One follow-up question I have: is it legal to unmap a Mojo buffer on a
different
> thread than the one it was originally mapped in? My intuition is yes, but if
no
> we'll have to ensure that these buffers are not sent to workers.

Yes it is OK to unmap on a different thread - mojo core APIs are thread-safe:
https://cs.chromium.org/chromium/src/mojo/edk/system/core.h?dr=CSs&l=39
https://cs.chromium.org/chromium/src/mojo/edk/system/core.cc?dr=CSs&l=995

jbroman@: The latest patch follows your advice of injecting a "free" function.
We now pass a unique_ptr<void>, which allows passing the deleter as well as
makes it very clear that data is owned by ArrayBuffer. We do not need the
OwnershipType flag anymore.

[jbroman, 2017-02-28 20:01:19]

Thanks. And I like using std::unique_ptr to make sure that lifetime is managed
correctly on all paths.

I think the only question that needs to be settled is haraken@'s question about
whether exposing Mojo shared buffers this way is the right decision. It is, at
least, equivalent to what we do for the C++ and Java bindings to Mojo. We also
do the same thing in the existing Mojo JS bindings (though that implementation
does it in a way that I think will crash if you ever supply it to a Blink API).

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp:152:
m_data.reset(ArrayBufferContents::allocateMemory(sizeInBytes, policy));
While I'm pretty sure the last deleter set here will be the right one, I'd
rather be explicit about which deleter is expected here:

m_data = ScopedData(ArrayBuffer::allocateMemory(source.sizeInBytes(),
DontInitialize), freeMemory);

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp:179:
m_data.reset(ArrayBufferContents::allocateMemory(source.sizeInBytes(),
While I'm pretty sure the last deleter set here will be the right one, I'd
rather be explicit about which deleter is expected here:

m_data = ScopedData(ArrayBuffer::allocateMemory(source.sizeInBytes(),
DontInitialize), freeMemory);

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:47: using
ScopedData = std::unique_ptr<void, DataDeleter>;
A brief comment about the expected usage of this (namely, that most callers will
want to provide WTF::ArrayBufferContents::freeMemory as the deleter) would be
useful.

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:87: static
void* allocateMemory(size_t, InitializationPolicy);
Possible bikeshed: but for most callers it might actually be nice this returned
a ScopedData with the right deleter. The couple users that don't could always
use ScopedData::release to get the raw void* out anyhow, and it would make this
less error-prone for callers other than V8.

[alokp, 2017-02-28 20:56:09]

I think this patch improves the ArrayBufferContents API independent of mojo
consideration.

We can discuss haraken's concerns in the patch that implements mojo-js bindings:
https://codereview.chromium.org/2720873002/

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp:152:
m_data.reset(ArrayBufferContents::allocateMemory(sizeInBytes, policy));
On 2017/02/28 20:01:19, jbroman wrote:
> While I'm pretty sure the last deleter set here will be the right one, I'd
> rather be explicit about which deleter is expected here:
> 
> m_data = ScopedData(ArrayBuffer::allocateMemory(source.sizeInBytes(),
> DontInitialize), freeMemory);

Replaced with createData.

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.cpp:179:
m_data.reset(ArrayBufferContents::allocateMemory(source.sizeInBytes(),
On 2017/02/28 20:01:19, jbroman wrote:
> While I'm pretty sure the last deleter set here will be the right one, I'd
> rather be explicit about which deleter is expected here:
> 
> m_data = ScopedData(ArrayBuffer::allocateMemory(source.sizeInBytes(),
> DontInitialize), freeMemory);

Replaced with createData.

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:47: using
ScopedData = std::unique_ptr<void, DataDeleter>;
On 2017/02/28 20:01:19, jbroman wrote:
> A brief comment about the expected usage of this (namely, that most callers
will
> want to provide WTF::ArrayBufferContents::freeMemory as the deleter) would be
> useful.

Done.

https://codereview.chromium.org/2719883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:87: static
void* allocateMemory(size_t, InitializationPolicy);
On 2017/02/28 20:01:19, jbroman wrote:
> Possible bikeshed: but for most callers it might actually be nice this
returned
> a ScopedData with the right deleter. The couple users that don't could always
> use ScopedData::release to get the raw void* out anyhow, and it would make
this
> less error-prone for callers other than V8.

Good idea. Although it would be a bit weird to not expose allocateMemory but
still expose freeMemory (which would be needed by V8Initializer). So I create a
helper createData function instead. I was also able to get rid of allocateMemory
since all usage has been replaced by createData.

[alokp, 2017-02-28 20:56:29]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 20:57:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-02-28 22:31:47]

Well, except that the DataDeleter only has one implementation if we don't need a
separate one for Mojo, at which point the unique_ptr can have a deleter which
doesn't store a function pointer.

[jbroman, 2017-02-28 22:32:34]

On 2017/02/28 at 22:31:47, jbroman wrote:
> Well, except that the DataDeleter only has one implementation if we don't need
a separate one for Mojo, at which point the unique_ptr can have a deleter which
doesn't store a function pointer.

But yes, agreed that the question about whether we should use ArrayBuffer for
this belongs on the Mojo CL. Still, I'd like to block this CL landing (in its
current form, at least) on a resolution to that question.

[commit-bot: I haz the power, 2017-02-28 23:23:00]

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

[commit-bot: I haz the power, 2017-02-28 23:23:00]

Dry run: This issue passed the CQ dry run.

[alokp, 2017-03-01 00:50:26]

alokp@chromium.org changed reviewers:
+ haraken@chromium.org

[alokp, 2017-03-01 00:50:27]

+haraken

[haraken, 2017-03-01 01:46:42]

On 2017/03/01 00:50:27, alokp wrote:
> +haraken

Thanks for all the discussion. Yeah, this looks weird from the perspective of
ArrayBuffers, but from the perspective of how to implement the Mojo-JS bindings,
I agree that this will be a reasonable option.

[haraken, 2017-03-01 01:55:36]

LGTM

https://codereview.chromium.org/2719883004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):

https://codereview.chromium.org/2719883004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:54: using
ScopedData = std::unique_ptr<void, DataDeleter>;

Nit: I'd rename this to Handle.

https://codereview.chromium.org/2719883004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:96: static
ScopedData createData(size_t, InitializationPolicy);

createHandle ?

[alokp, 2017-03-01 02:01:51]

https://codereview.chromium.org/2719883004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):

https://codereview.chromium.org/2719883004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:54: using
ScopedData = std::unique_ptr<void, DataDeleter>;
On 2017/03/01 01:55:36, haraken wrote:
> 
> Nit: I'd rename this to Handle.

Handle Seems too generic. How about DataHandle?

[haraken, 2017-03-01 02:02:44]

On 2017/03/01 02:01:51, alokp wrote:
>
https://codereview.chromium.org/2719883004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):
> 
>
https://codereview.chromium.org/2719883004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:54: using
> ScopedData = std::unique_ptr<void, DataDeleter>;
> On 2017/03/01 01:55:36, haraken wrote:
> > 
> > Nit: I'd rename this to Handle.
> 
> Handle Seems too generic. How about DataHandle?

Sounds good, or ArrayBufferHandle?

[jbroman, 2017-03-01 02:59:30]

lgtm 2 (and I'm fine with whatever name you settle on for the unique_ptr)

[alokp, 2017-03-01 04:24:25]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-03-01 04:24:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2719883004/#ps60001
(title: "renames ScopedData -> DataHandle")

[commit-bot: I haz the power, 2017-03-01 04:24:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 06:10:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488342265508530,
"parent_rev": "8b8542139c3680bd68c6fa07b3df6cfd8706b3c9", "commit_rev":
"cce921f5c9a04fa1d7aa70075c70678e3504ef49"}

[commit-bot: I haz the power, 2017-03-01 06:11:40]

Description was changed from

==========
Adds support for ArrayBufferContents with external buffer.

The current implementation of WTF::ArrayBuffer assumes that underlying
buffer is always owned by the ArrayBuffer. This patch teaches
ArrayBufferContents about external buffers that are not owned by the
ArrayBuffer.

This support is need to implement JS bindings for mojo shared buffer
where a buffer owned by mojo needs to be exposed as an ArrayBuffer. More
details here: https://codereview.chromium.org/2720873002/

BUG=647036
==========

to

==========
Adds support for ArrayBufferContents with external buffer.

The current implementation of WTF::ArrayBuffer assumes that underlying
buffer is always owned by the ArrayBuffer. This patch teaches
ArrayBufferContents about external buffers that are not owned by the
ArrayBuffer.

This support is need to implement JS bindings for mojo shared buffer
where a buffer owned by mojo needs to be exposed as an ArrayBuffer. More
details here: https://codereview.chromium.org/2720873002/

BUG=647036

Review-Url: https://codereview.chromium.org/2719883004
Cr-Commit-Position: refs/heads/master@{#453861}
Committed:
https://chromium.googlesource.com/chromium/src/+/cce921f5c9a04fa1d7aa70075c70...
==========

[commit-bot: I haz the power, 2017-03-01 06:11:41]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/cce921f5c9a04fa1d7aa70075c70...