Chromium Code Reviews
Help | Chromium Project | Sign in
(32)

Issue 2826263002: Make DOMArrayBuffer::Transfer neuter v8::ArrayBuffers

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 days, 20 hours ago by binji
Modified:
2 days, 20 hours ago
Reviewers:
haraken
CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, haraken, hongchan, jbroman+watch_chromium.org, rwlbuis, Raymond Toy, sof
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Make DOMArrayBuffer::Transfer neuter v8::ArrayBuffers This behavior is currently done by ScriptValueSerializer, but it's more convenient if the necessary logic is moved to DOMArrayBuffer::Transfer directly; you'll never want to transfer without also calling Neuter on the V8 ArrayBuffer objects. BUG=711824

Patch Set 1 #

Total comments: 5

Patch Set 2 : merge HEAD and feedback #

Total comments: 1

Patch Set 3 : feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -56 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp View 1 2 3 chunks +11 lines, -38 lines 0 comments Download
M third_party/WebKit/Source/core/dom/DOMArrayBuffer.h View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/DOMArrayBuffer.cpp View 1 2 1 chunk +48 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/DOMArrayBufferBase.h View 1 chunk +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/dom/DOMSharedArrayBuffer.h View 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp View 1 2 2 chunks +13 lines, -12 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 18 (14 generated)
binji
PTAL It seems strange to pass v8::Isolate* to DOMArrayBuffer methods, but I'm not experienced enough ...
3 days, 22 hours ago (2017-04-20 18:44:02 UTC) #7
haraken
https://codereview.chromium.org/2826263002/diff/1/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2826263002/diff/1/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode505 third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:505: exception_state.ThrowDOMException( We're checking IsNeuterable twice. (Transfer calls another IsNeuterable.) ...
3 days, 20 hours ago (2017-04-20 20:07:01 UTC) #8
binji
https://codereview.chromium.org/2826263002/diff/1/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2826263002/diff/1/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode505 third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:505: exception_state.ThrowDOMException( On 2017/04/20 at 20:07:01, haraken wrote: > We're ...
3 days, 17 hours ago (2017-04-21 00:00:23 UTC) #9
haraken
3 days, 5 hours ago (2017-04-21 12:03:39 UTC) #14
LGTM

https://codereview.chromium.org/2826263002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/DOMArrayBuffer.cpp (right):

https://codereview.chromium.org/2826263002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/DOMArrayBuffer.cpp:14: static void
AccumulateArrayBuffersForAllWorlds(
On 2017/04/21 00:00:23, binji wrote:
> On 2017/04/20 at 20:07:01, haraken wrote:
> > I forgot the context but would you help me understand why we need to check
> wrappers of all worlds? It seems like we're doing something strange here.
> 
> I have to admit, I don't understand what a "world" is, and why it would have
its
> own copy of an ArrayBuffer. But if it shares the same backing store as the
> DOMArrayBuffer, it should be neutered on transfer.

Makes sense. Yes, those ArrayBuffer objects are sharing the same backing store.
If you need the semantics that all ArrayBuffers using the backing store must be
neutered, we need that logic.

> 
> I went digging for where this code was originally added, and I was able to
trace
> the basic idea back to the initial implementation of ArrayBuffer transfer:
>
https://chromium.googlesource.com/chromium/src/+/bd67e9585f4acaad1c468eb28633...

https://codereview.chromium.org/2826263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMArrayBuffer.cpp (right):

https://codereview.chromium.org/2826263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMArrayBuffer.cpp:74: }

Can we clean up the code as follows?

- Remove Transfer
- Rename TransferOrCopy to Transfer
- Make IsNeuterable a public method
- Make BaseAudioContext use IsNeuterable & Transfer
- Inline TransferNeuterable and NeuterArrayBuffersInAllWorlds
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46