Skip to content

Commit fdba3bb

Browse files
mcollinamcollina
committed
stream: address review feedback on webstreams C++ helpers
- Collapse the three ArrayBufferView accessors into a single getArrayBufferView() that returns { buffer, byteOffset, byteLength } to cut binding crossings. - Drop AB/SAB type-guards in CanCopyArrayBuffer; SAB handles are interoperable with ArrayBuffer handles in V8. - Use ArrayBuffer::MaybeNew() in CloneAsUint8Array and throw on OOM instead of crashing the process.
1 parent c232ba8 commit fdba3bb

3 files changed

Lines changed: 69 additions & 82 deletions

File tree

‎lib/internal/webstreams/readablestream.js‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,6 @@ const {
9393
} = require('internal/streams/utils');
9494

9595
const {
96-
ArrayBufferViewGetBuffer,
97-
ArrayBufferViewGetByteLength,
98-
ArrayBufferViewGetByteOffset,
9996
AsyncIterator,
10097
canCopyArrayBuffer,
10198
cloneAsUint8Array,
@@ -106,6 +103,7 @@ const {
106103
enqueueValueWithSize,
107104
extractHighWaterMark,
108105
extractSizeAlgorithm,
106+
getArrayBufferView,
109107
getNonWritablePropertyDescriptor,
110108
isBrandCheck,
111109
kState,
@@ -688,8 +686,10 @@ class ReadableStreamBYOBRequest {
688686
'This BYOB request has been invalidated');
689687
}
690688

691-
const viewByteLength = ArrayBufferViewGetByteLength(view);
692-
const viewBuffer = ArrayBufferViewGetBuffer(view);
689+
const {
690+
buffer: viewBuffer,
691+
byteLength: viewByteLength,
692+
} = getArrayBufferView(view);
693693
const viewBufferByteLength = ArrayBufferPrototypeGetByteLength(viewBuffer);
694694

695695
if (ArrayBufferPrototypeGetDetached(viewBuffer)) {
@@ -980,8 +980,10 @@ class ReadableStreamBYOBReader {
980980
}
981981
validateObject(options, 'options', kValidateObjectAllowObjectsAndNull);
982982

983-
const viewByteLength = ArrayBufferViewGetByteLength(view);
984-
const viewBuffer = ArrayBufferViewGetBuffer(view);
983+
const {
984+
buffer: viewBuffer,
985+
byteLength: viewByteLength,
986+
} = getArrayBufferView(view);
985987

986988
if (isSharedArrayBuffer(viewBuffer)) {
987989
throw new ERR_INVALID_ARG_VALUE(
@@ -1198,8 +1200,10 @@ class ReadableByteStreamController {
11981200
if (!isReadableByteStreamController(this))
11991201
throw new ERR_INVALID_THIS('ReadableByteStreamController');
12001202
validateBuffer(chunk);
1201-
const chunkByteLength = ArrayBufferViewGetByteLength(chunk);
1202-
const chunkBuffer = ArrayBufferViewGetBuffer(chunk);
1203+
const {
1204+
buffer: chunkBuffer,
1205+
byteLength: chunkByteLength,
1206+
} = getArrayBufferView(chunk);
12031207

12041208
if (isSharedArrayBuffer(chunkBuffer)) {
12051209
throw new ERR_INVALID_ARG_VALUE(
@@ -2745,9 +2749,7 @@ function readableByteStreamControllerPullInto(
27452749
assert(minimumFill >= elementSize && minimumFill <= view.byteLength);
27462750
assert(minimumFill % elementSize === 0);
27472751

2748-
const buffer = ArrayBufferViewGetBuffer(view);
2749-
const byteOffset = ArrayBufferViewGetByteOffset(view);
2750-
const byteLength = ArrayBufferViewGetByteLength(view);
2752+
const { buffer, byteOffset, byteLength } = getArrayBufferView(view);
27512753
const bufferByteLength = ArrayBufferPrototypeGetByteLength(buffer);
27522754

27532755
let transferredBuffer;
@@ -2888,9 +2890,7 @@ function readableByteStreamControllerEnqueue(controller, chunk) {
28882890
stream,
28892891
} = controller[kState];
28902892

2891-
const buffer = ArrayBufferViewGetBuffer(chunk);
2892-
const byteOffset = ArrayBufferViewGetByteOffset(chunk);
2893-
const byteLength = ArrayBufferViewGetByteLength(chunk);
2893+
const { buffer, byteOffset, byteLength } = getArrayBufferView(chunk);
28942894

28952895
if (closeRequested || stream[kState].state !== 'readable')
28962896
return;
@@ -3183,9 +3183,11 @@ function readableByteStreamControllerRespondWithNewView(controller, view) {
31833183
const desc = pendingPullIntos[0];
31843184
assert(stream[kState].state !== 'errored');
31853185

3186-
const viewByteLength = ArrayBufferViewGetByteLength(view);
3187-
const viewByteOffset = ArrayBufferViewGetByteOffset(view);
3188-
const viewBuffer = ArrayBufferViewGetBuffer(view);
3186+
const {
3187+
buffer: viewBuffer,
3188+
byteOffset: viewByteOffset,
3189+
byteLength: viewByteLength,
3190+
} = getArrayBufferView(view);
31893191
const viewBufferByteLength = ArrayBufferPrototypeGetByteLength(viewBuffer);
31903192

31913193
if (stream[kState].state === 'closed') {

‎lib/internal/webstreams/util.js‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,9 @@ const {
2222
} = internalBinding('buffer');
2323

2424
const {
25-
arrayBufferViewGetBuffer: ArrayBufferViewGetBuffer,
26-
arrayBufferViewGetByteLength: ArrayBufferViewGetByteLength,
27-
arrayBufferViewGetByteOffset: ArrayBufferViewGetByteOffset,
2825
canCopyArrayBuffer,
2926
cloneAsUint8Array,
27+
getArrayBufferView,
3028
} = internalBinding('webstreams');
3129

3230
const {
@@ -90,11 +88,11 @@ function customInspect(depth, options, name, data) {
9088
return `${name} ${inspect(data, opts)}`;
9189
}
9290

93-
// ArrayBufferViewGetBuffer/ByteLength/ByteOffset, canCopyArrayBuffer, and
94-
// cloneAsUint8Array are implemented in src/node_webstreams.cc via direct V8
95-
// API calls. They are immune to user tampering of typed-array prototypes
96-
// (matching the defensive behavior of the previous Reflect.get-based JS
97-
// implementation) and faster on hot byte-stream paths.
91+
// getArrayBufferView, canCopyArrayBuffer, and cloneAsUint8Array are
92+
// implemented in src/node_webstreams.cc via direct V8 API calls. They are
93+
// immune to user tampering of typed-array prototypes (matching the defensive
94+
// behavior of the previous Reflect.get-based JS implementation) and faster on
95+
// hot byte-stream paths.
9896

9997
function isBrandCheck(brand) {
10098
return (value) => {
@@ -182,9 +180,6 @@ function lazyTransfer() {
182180
}
183181

184182
module.exports = {
185-
ArrayBufferViewGetBuffer,
186-
ArrayBufferViewGetByteLength,
187-
ArrayBufferViewGetByteOffset,
188183
AsyncIterator,
189184
canCopyArrayBuffer,
190185
cloneAsUint8Array,
@@ -195,6 +190,7 @@ module.exports = {
195190
enqueueValueWithSize,
196191
extractHighWaterMark,
197192
extractSizeAlgorithm,
193+
getArrayBufferView,
198194
getNonWritablePropertyDescriptor,
199195
isBrandCheck,
200196
isPromisePending,

‎src/node_webstreams.cc‎

Lines changed: 42 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1+
#include "env-inl.h"
12
#include "node_binding.h"
3+
#include "node_errors.h"
24
#include "node_external_reference.h"
35
#include "util-inl.h"
46
#include "v8.h"
@@ -12,50 +14,35 @@ using v8::Context;
1214
using v8::FunctionCallbackInfo;
1315
using v8::Isolate;
1416
using v8::Local;
17+
using v8::Name;
18+
using v8::Null;
19+
using v8::Number;
1520
using v8::Object;
16-
using v8::SharedArrayBuffer;
1721
using v8::Uint32;
1822
using v8::Uint8Array;
1923
using v8::Value;
2024

21-
namespace {
22-
23-
inline bool BufferIsDetached(Local<Value> buffer) {
24-
if (buffer->IsArrayBuffer()) {
25-
return buffer.As<ArrayBuffer>()->WasDetached();
26-
}
27-
// SharedArrayBuffers cannot be detached.
28-
return false;
29-
}
30-
31-
inline size_t BufferByteLength(Local<Value> buffer) {
32-
if (buffer->IsArrayBuffer()) {
33-
return buffer.As<ArrayBuffer>()->ByteLength();
34-
}
35-
return buffer.As<SharedArrayBuffer>()->ByteLength();
36-
}
37-
38-
} // namespace
39-
40-
// Equivalent to:
41-
// Reflect.get(view.constructor.prototype, 'buffer', view)
42-
// but uses the V8 API directly so it is immune to prototype tampering and
43-
// avoids the JS-side overhead of the defensive accessor in lib/internal/.
44-
void ArrayBufferViewGetBuffer(const FunctionCallbackInfo<Value>& args) {
45-
CHECK(args[0]->IsArrayBufferView());
46-
args.GetReturnValue().Set(args[0].As<ArrayBufferView>()->Buffer());
47-
}
48-
49-
void ArrayBufferViewGetByteLength(const FunctionCallbackInfo<Value>& args) {
50-
CHECK(args[0]->IsArrayBufferView());
51-
Local<ArrayBufferView> view = args[0].As<ArrayBufferView>();
52-
args.GetReturnValue().Set(static_cast<double>(view->ByteLength()));
53-
}
54-
55-
void ArrayBufferViewGetByteOffset(const FunctionCallbackInfo<Value>& args) {
25+
// Returns { buffer, byteOffset, byteLength } in a single binding crossing,
26+
// equivalent to reading the three properties via
27+
// Reflect.get(view.constructor.prototype, ..., view). Uses the V8 API
28+
// directly so it is immune to prototype tampering and avoids the JS-side
29+
// overhead of the defensive accessors in lib/internal/.
30+
void GetArrayBufferView(const FunctionCallbackInfo<Value>& args) {
31+
Isolate* isolate = args.GetIsolate();
5632
CHECK(args[0]->IsArrayBufferView());
5733
Local<ArrayBufferView> view = args[0].As<ArrayBufferView>();
58-
args.GetReturnValue().Set(static_cast<double>(view->ByteOffset()));
34+
Local<Name> names[] = {
35+
FIXED_ONE_BYTE_STRING(isolate, "buffer"),
36+
FIXED_ONE_BYTE_STRING(isolate, "byteOffset"),
37+
FIXED_ONE_BYTE_STRING(isolate, "byteLength"),
38+
};
39+
Local<Value> values[] = {
40+
view->Buffer(),
41+
Number::New(isolate, static_cast<double>(view->ByteOffset())),
42+
Number::New(isolate, static_cast<double>(view->ByteLength())),
43+
};
44+
args.GetReturnValue().Set(Object::New(
45+
isolate, Null(isolate), names, values, arraysize(names)));
5946
}
6047

6148
// Returns true iff bytes can be safely copied between the buffers given the
@@ -72,14 +59,17 @@ void CanCopyArrayBuffer(const FunctionCallbackInfo<Value>& args) {
7259
CHECK(args[3]->IsUint32());
7360
CHECK(args[4]->IsUint32());
7461

75-
Local<Object> to_buffer = args[0].As<Object>();
76-
Local<Object> from_buffer = args[2].As<Object>();
62+
// SharedArrayBuffer handles are interoperable with ArrayBuffer handles in
63+
// V8, so we can use the ArrayBuffer accessors uniformly. WasDetached()
64+
// always returns false on a SAB.
65+
Local<ArrayBuffer> to_buffer = args[0].As<ArrayBuffer>();
66+
Local<ArrayBuffer> from_buffer = args[2].As<ArrayBuffer>();
7767

7868
if (to_buffer->StrictEquals(from_buffer)) {
7969
args.GetReturnValue().Set(false);
8070
return;
8171
}
82-
if (BufferIsDetached(args[0]) || BufferIsDetached(args[2])) {
72+
if (to_buffer->WasDetached() || from_buffer->WasDetached()) {
8373
args.GetReturnValue().Set(false);
8474
return;
8575
}
@@ -88,8 +78,8 @@ void CanCopyArrayBuffer(const FunctionCallbackInfo<Value>& args) {
8878
uint32_t from_index = args[3].As<Uint32>()->Value();
8979
uint32_t count = args[4].As<Uint32>()->Value();
9080

91-
size_t to_byte_length = BufferByteLength(args[0]);
92-
size_t from_byte_length = BufferByteLength(args[2]);
81+
size_t to_byte_length = to_buffer->ByteLength();
82+
size_t from_byte_length = from_buffer->ByteLength();
9383

9484
bool ok = static_cast<uint64_t>(to_index) + count <= to_byte_length &&
9585
static_cast<uint64_t>(from_index) + count <= from_byte_length;
@@ -103,11 +93,17 @@ void CanCopyArrayBuffer(const FunctionCallbackInfo<Value>& args) {
10393
// returns a Uint8Array over the full new buffer. Avoids the JS-side
10494
// Reflect.get + slice round-trip.
10595
void CloneAsUint8Array(const FunctionCallbackInfo<Value>& args) {
106-
Isolate* isolate = args.GetIsolate();
96+
Environment* env = Environment::GetCurrent(args);
97+
Isolate* isolate = env->isolate();
10798
CHECK(args[0]->IsArrayBufferView());
10899
Local<ArrayBufferView> view = args[0].As<ArrayBufferView>();
109100
size_t byte_length = view->ByteLength();
110-
Local<ArrayBuffer> new_buffer = ArrayBuffer::New(isolate, byte_length);
101+
Local<ArrayBuffer> new_buffer;
102+
if (!ArrayBuffer::MaybeNew(isolate, byte_length).ToLocal(&new_buffer)) {
103+
// MaybeNew does not schedule an exception on allocation failure.
104+
env->ThrowRangeError("Array buffer allocation failed");
105+
return;
106+
}
111107
if (byte_length > 0) {
112108
size_t copied = view->CopyContents(new_buffer->Data(), byte_length);
113109
CHECK_EQ(copied, byte_length);
@@ -119,20 +115,13 @@ void Initialize(Local<Object> target,
119115
Local<Value> unused,
120116
Local<Context> context,
121117
void* priv) {
122-
SetMethod(context, target, "arrayBufferViewGetBuffer",
123-
ArrayBufferViewGetBuffer);
124-
SetMethod(context, target, "arrayBufferViewGetByteLength",
125-
ArrayBufferViewGetByteLength);
126-
SetMethod(context, target, "arrayBufferViewGetByteOffset",
127-
ArrayBufferViewGetByteOffset);
118+
SetMethod(context, target, "getArrayBufferView", GetArrayBufferView);
128119
SetMethod(context, target, "canCopyArrayBuffer", CanCopyArrayBuffer);
129120
SetMethod(context, target, "cloneAsUint8Array", CloneAsUint8Array);
130121
}
131122

132123
void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
133-
registry->Register(ArrayBufferViewGetBuffer);
134-
registry->Register(ArrayBufferViewGetByteLength);
135-
registry->Register(ArrayBufferViewGetByteOffset);
124+
registry->Register(GetArrayBufferView);
136125
registry->Register(CanCopyArrayBuffer);
137126
registry->Register(CloneAsUint8Array);
138127
}

0 commit comments

Comments
 (0)