From 1a92c884185bc3e01c6d64aae55ea5a947d82840 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 8 Nov 2019 20:40:46 +0200 Subject: [PATCH] src: migrate off ArrayBuffer::GetContents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit V8 deprecates `GetContents()` in favour of `GetBackingStore()`. Update our code to reflect that. V8 also deprecates `Externalize()` and `IsExternal()`; we should be able to remove all usage of this once V8 8.0 is there. PR-URL: https://github.com/nodejs/node/pull/30339 Refs: https://github.com/v8/v8/commit/bfe3d6bce734e596e312465e207bcfd55a59fe34 Reviewed-By: Gus Caplan Reviewed-By: Michaël Zasso Reviewed-By: Colin Ihrig Reviewed-By: Jiawen Geng Reviewed-By: David Carlier --- src/aliased_buffer.h | 4 ++-- src/js_native_api_v8.cc | 31 ++++++++++++++++++-------- src/node_buffer.cc | 13 +++++------ src/node_contextify.cc | 8 +++---- src/node_messaging.cc | 21 ++++++++++++----- src/node_os.cc | 2 +- src/node_process_methods.cc | 10 ++++----- src/node_worker.cc | 4 +++- src/node_zlib.cc | 3 ++- src/util-inl.h | 2 +- src/util.h | 5 +++-- test/addons/openssl-binding/binding.cc | 4 ++-- test/addons/zlib-binding/binding.cc | 4 ++-- 13 files changed, 67 insertions(+), 44 deletions(-) diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h index 5083ae9a1f47b5..b083fb68e69bd2 100644 --- a/src/aliased_buffer.h +++ b/src/aliased_buffer.h @@ -42,7 +42,7 @@ class AliasedBufferBase { // allocate v8 ArrayBuffer v8::Local ab = v8::ArrayBuffer::New( isolate_, size_in_bytes); - buffer_ = static_cast(ab->GetContents().Data()); + buffer_ = static_cast(ab->GetBackingStore()->Data()); // allocate v8 TypedArray v8::Local js_array = V8T::New(ab, byte_offset_, count); @@ -228,7 +228,7 @@ class AliasedBufferBase { isolate_, new_size_in_bytes); // allocate new native buffer - NativeT* new_buffer = static_cast(ab->GetContents().Data()); + NativeT* new_buffer = static_cast(ab->GetBackingStore()->Data()); // copy old content memcpy(new_buffer, buffer_, old_size_in_bytes); diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 1f5b6c012f7975..ef1edb92eed6fa 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -2562,7 +2562,7 @@ napi_status napi_create_arraybuffer(napi_env env, // Optionally return a pointer to the buffer's data, to avoid another call to // retrieve it. if (data != nullptr) { - *data = buffer->GetContents().Data(); + *data = buffer->GetBackingStore()->Data(); } *result = v8impl::JsValueFromV8LocalValue(buffer); @@ -2608,15 +2608,15 @@ napi_status napi_get_arraybuffer_info(napi_env env, v8::Local value = v8impl::V8LocalValueFromJsValue(arraybuffer); RETURN_STATUS_IF_FALSE(env, value->IsArrayBuffer(), napi_invalid_arg); - v8::ArrayBuffer::Contents contents = - value.As()->GetContents(); + std::shared_ptr backing_store = + value.As()->GetBackingStore(); if (data != nullptr) { - *data = contents.Data(); + *data = backing_store->Data(); } if (byte_length != nullptr) { - *byte_length = contents.ByteLength(); + *byte_length = backing_store->ByteLength(); } return napi_clear_last_error(env); @@ -2747,9 +2747,15 @@ napi_status napi_get_typedarray_info(napi_env env, *length = array->Length(); } - v8::Local buffer = array->Buffer(); + v8::Local buffer; + if (data != nullptr || arraybuffer != nullptr) { + // Calling Buffer() may have the side effect of allocating the buffer, + // so only do this when it’s needed. + buffer = array->Buffer(); + } + if (data != nullptr) { - *data = static_cast(buffer->GetContents().Data()) + + *data = static_cast(buffer->GetBackingStore()->Data()) + array->ByteOffset(); } @@ -2821,9 +2827,15 @@ napi_status napi_get_dataview_info(napi_env env, *byte_length = array->ByteLength(); } - v8::Local buffer = array->Buffer(); + v8::Local buffer; + if (data != nullptr || arraybuffer != nullptr) { + // Calling Buffer() may have the side effect of allocating the buffer, + // so only do this when it’s needed. + buffer = array->Buffer(); + } + if (data != nullptr) { - *data = static_cast(buffer->GetContents().Data()) + + *data = static_cast(buffer->GetBackingStore()->Data()) + array->ByteOffset(); } @@ -3015,6 +3027,7 @@ napi_status napi_detach_arraybuffer(napi_env env, napi_value arraybuffer) { env, value->IsArrayBuffer(), napi_arraybuffer_expected); v8::Local it = value.As(); + // TODO(addaleax): Remove the first condition once we have V8 8.0. RETURN_STATUS_IF_FALSE( env, it->IsExternal(), napi_detachable_arraybuffer_expected); RETURN_STATUS_IF_FALSE( diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 3aa1ea2535a47d..2dbdd61923f026 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -192,16 +192,13 @@ bool HasInstance(Local obj) { char* Data(Local val) { CHECK(val->IsArrayBufferView()); Local ui = val.As(); - ArrayBuffer::Contents ab_c = ui->Buffer()->GetContents(); - return static_cast(ab_c.Data()) + ui->ByteOffset(); + return static_cast(ui->Buffer()->GetBackingStore()->Data()) + + ui->ByteOffset(); } char* Data(Local obj) { - CHECK(obj->IsArrayBufferView()); - Local ui = obj.As(); - ArrayBuffer::Contents ab_c = ui->Buffer()->GetContents(); - return static_cast(ab_c.Data()) + ui->ByteOffset(); + return Data(obj.As()); } @@ -1060,13 +1057,13 @@ static void EncodeInto(const FunctionCallbackInfo& args) { Local dest = args[1].As(); Local buf = dest->Buffer(); char* write_result = - static_cast(buf->GetContents().Data()) + dest->ByteOffset(); + static_cast(buf->GetBackingStore()->Data()) + dest->ByteOffset(); size_t dest_length = dest->ByteLength(); // results = [ read, written ] Local result_arr = args[2].As(); uint32_t* results = reinterpret_cast( - static_cast(result_arr->Buffer()->GetContents().Data()) + + static_cast(result_arr->Buffer()->GetBackingStore()->Data()) + result_arr->ByteOffset()); int nchars; diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 2d30e0b8038ce4..46a1d7c8ef0691 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -697,8 +697,8 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { ScriptCompiler::CachedData* cached_data = nullptr; if (!cached_data_buf.IsEmpty()) { - ArrayBuffer::Contents contents = cached_data_buf->Buffer()->GetContents(); - uint8_t* data = static_cast(contents.Data()); + uint8_t* data = static_cast( + cached_data_buf->Buffer()->GetBackingStore()->Data()); cached_data = new ScriptCompiler::CachedData( data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength()); } @@ -1044,8 +1044,8 @@ void ContextifyContext::CompileFunction( // Read cache from cached data buffer ScriptCompiler::CachedData* cached_data = nullptr; if (!cached_data_buf.IsEmpty()) { - ArrayBuffer::Contents contents = cached_data_buf->Buffer()->GetContents(); - uint8_t* data = static_cast(contents.Data()); + uint8_t* data = static_cast( + cached_data_buf->Buffer()->GetBackingStore()->Data()); cached_data = new ScriptCompiler::CachedData( data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength()); } diff --git a/src/node_messaging.cc b/src/node_messaging.cc index c2a2063381ddf1..61714d7ba4c795 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -297,12 +297,19 @@ Maybe Message::Serialize(Environment* env, // Currently, we support ArrayBuffers and MessagePorts. if (entry->IsArrayBuffer()) { Local ab = entry.As(); - // If we cannot render the ArrayBuffer unusable in this Isolate and - // take ownership of its memory, copying the buffer will have to do. - if (!ab->IsDetachable() || ab->IsExternal() || - !env->isolate_data()->uses_node_allocator()) { + // If we cannot render the ArrayBuffer unusable in this Isolate, + // copying the buffer will have to do. + // Note that we can currently transfer ArrayBuffers even if they were + // not allocated by Node’s ArrayBufferAllocator in the first place, + // because we pass the underlying v8::BackingStore around rather than + // raw data *and* an Isolate with a non-default ArrayBuffer allocator + // is always going to outlive any Workers it creates, and so will its + // allocator along with it. + // TODO(addaleax): Eventually remove the IsExternal() condition, + // see https://github.com/nodejs/node/pull/30339#issuecomment-552225353 + // for details. + if (!ab->IsDetachable() || ab->IsExternal()) continue; - } if (std::find(array_buffers.begin(), array_buffers.end(), ab) != array_buffers.end()) { ThrowDataCloneException( @@ -362,7 +369,9 @@ Maybe Message::Serialize(Environment* env, for (Local ab : array_buffers) { // If serialization succeeded, we render it inaccessible in this Isolate. std::shared_ptr backing_store = ab->GetBackingStore(); - ab->Externalize(backing_store); + // TODO(addaleax): This can/should be dropped once we have V8 8.0. + if (!ab->IsExternal()) + ab->Externalize(backing_store); ab->Detach(); array_buffers_.emplace_back(std::move(backing_store)); diff --git a/src/node_os.cc b/src/node_os.cc index 131e38055685c2..12a4ec3551a4db 100644 --- a/src/node_os.cc +++ b/src/node_os.cc @@ -171,7 +171,7 @@ static void GetLoadAvg(const FunctionCallbackInfo& args) { Local array = args[0].As(); CHECK_EQ(array->Length(), 3); Local ab = array->Buffer(); - double* loadavg = static_cast(ab->GetContents().Data()); + double* loadavg = static_cast(ab->GetBackingStore()->Data()); uv_loadavg(loadavg); } diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 7e2af379079c4c..7efe8efb9b9e6d 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -109,7 +109,7 @@ static void CPUUsage(const FunctionCallbackInfo& args) { Local array = args[0].As(); CHECK_EQ(array->Length(), 2); Local ab = array->Buffer(); - double* fields = static_cast(ab->GetContents().Data()); + double* fields = static_cast(ab->GetBackingStore()->Data()); // Set the Float64Array elements to be user / system values in microseconds. fields[0] = MICROS_PER_SEC * rusage.ru_utime.tv_sec + rusage.ru_utime.tv_usec; @@ -148,7 +148,7 @@ static void Hrtime(const FunctionCallbackInfo& args) { uint64_t t = uv_hrtime(); Local ab = args[0].As()->Buffer(); - uint32_t* fields = static_cast(ab->GetContents().Data()); + uint32_t* fields = static_cast(ab->GetBackingStore()->Data()); fields[0] = (t / NANOS_PER_SEC) >> 32; fields[1] = (t / NANOS_PER_SEC) & 0xffffffff; @@ -157,7 +157,7 @@ static void Hrtime(const FunctionCallbackInfo& args) { static void HrtimeBigInt(const FunctionCallbackInfo& args) { Local ab = args[0].As()->Buffer(); - uint64_t* fields = static_cast(ab->GetContents().Data()); + uint64_t* fields = static_cast(ab->GetBackingStore()->Data()); fields[0] = uv_hrtime(); } @@ -204,7 +204,7 @@ static void MemoryUsage(const FunctionCallbackInfo& args) { Local array = args[0].As(); CHECK_EQ(array->Length(), 4); Local ab = array->Buffer(); - double* fields = static_cast(ab->GetContents().Data()); + double* fields = static_cast(ab->GetBackingStore()->Data()); fields[0] = rss; fields[1] = v8_heap_stats.total_heap_size(); @@ -301,7 +301,7 @@ static void ResourceUsage(const FunctionCallbackInfo& args) { Local array = args[0].As(); CHECK_EQ(array->Length(), 16); Local ab = array->Buffer(); - double* fields = static_cast(ab->GetContents().Data()); + double* fields = static_cast(ab->GetBackingStore()->Data()); fields[0] = MICROS_PER_SEC * rusage.ru_utime.tv_sec + rusage.ru_utime.tv_usec; fields[1] = MICROS_PER_SEC * rusage.ru_stime.tv_sec + rusage.ru_stime.tv_usec; diff --git a/src/node_worker.cc b/src/node_worker.cc index 3d3838d7969729..676b70a1fa90c1 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -623,7 +623,9 @@ void Worker::GetResourceLimits(const FunctionCallbackInfo& args) { Local Worker::GetResourceLimits(Isolate* isolate) const { Local ab = ArrayBuffer::New(isolate, sizeof(resource_limits_)); - memcpy(ab->GetContents().Data(), resource_limits_, sizeof(resource_limits_)); + memcpy(ab->GetBackingStore()->Data(), + resource_limits_, + sizeof(resource_limits_)); return Float64Array::New(ab, 0, kTotalResourceLimitCount); } diff --git a/src/node_zlib.cc b/src/node_zlib.cc index fdcf685caf2e5b..739e36d69911b2 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -590,7 +590,8 @@ class ZlibStream : public CompressionStream { CHECK(args[4]->IsUint32Array()); Local array = args[4].As(); Local ab = array->Buffer(); - uint32_t* write_result = static_cast(ab->GetContents().Data()); + uint32_t* write_result = static_cast( + ab->GetBackingStore()->Data()); CHECK(args[5]->IsFunction()); Local write_js_callback = args[5].As(); diff --git a/src/util-inl.h b/src/util-inl.h index c06a0ae84c88f5..d44ee09fefbb3a 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -514,7 +514,7 @@ void ArrayBufferViewContents::Read(v8::Local abv) { static_assert(sizeof(T) == 1, "Only supports one-byte data at the moment"); length_ = abv->ByteLength(); if (length_ > sizeof(stack_storage_) || abv->HasBuffer()) { - data_ = static_cast(abv->Buffer()->GetContents().Data()) + + data_ = static_cast(abv->Buffer()->GetBackingStore()->Data()) + abv->ByteOffset(); } else { abv->CopyContents(stack_storage_, sizeof(stack_storage_)); diff --git a/src/util.h b/src/util.h index f2d3f355f9f713..2f6c17fc321a67 100644 --- a/src/util.h +++ b/src/util.h @@ -488,11 +488,12 @@ class BufferValue : public MaybeStackBuffer { #define SPREAD_BUFFER_ARG(val, name) \ CHECK((val)->IsArrayBufferView()); \ v8::Local name = (val).As(); \ - v8::ArrayBuffer::Contents name##_c = name->Buffer()->GetContents(); \ + std::shared_ptr name##_bs = \ + name->Buffer()->GetBackingStore(); \ const size_t name##_offset = name->ByteOffset(); \ const size_t name##_length = name->ByteLength(); \ char* const name##_data = \ - static_cast(name##_c.Data()) + name##_offset; \ + static_cast(name##_bs->Data()) + name##_offset; \ if (name##_length > 0) \ CHECK_NE(name##_data, nullptr); diff --git a/test/addons/openssl-binding/binding.cc b/test/addons/openssl-binding/binding.cc index ecda40f4cb50a3..6cfecc4505421e 100644 --- a/test/addons/openssl-binding/binding.cc +++ b/test/addons/openssl-binding/binding.cc @@ -12,8 +12,8 @@ inline void RandomBytes(const v8::FunctionCallbackInfo& info) { auto byte_length = view->ByteLength(); assert(view->HasBuffer()); auto buffer = view->Buffer(); - auto contents = buffer->GetContents(); - auto data = static_cast(contents.Data()) + byte_offset; + auto contents = buffer->GetBackingStore(); + auto data = static_cast(contents->Data()) + byte_offset; assert(RAND_poll()); auto rval = RAND_bytes(data, static_cast(byte_length)); info.GetReturnValue().Set(rval > 0); diff --git a/test/addons/zlib-binding/binding.cc b/test/addons/zlib-binding/binding.cc index 0b82de211a8430..abfb0615842594 100644 --- a/test/addons/zlib-binding/binding.cc +++ b/test/addons/zlib-binding/binding.cc @@ -12,8 +12,8 @@ inline void CompressBytes(const v8::FunctionCallbackInfo& info) { auto byte_length = view->ByteLength(); assert(view->HasBuffer()); auto buffer = view->Buffer(); - auto contents = buffer->GetContents(); - auto data = static_cast(contents.Data()) + byte_offset; + auto contents = buffer->GetBackingStore(); + auto data = static_cast(contents->Data()) + byte_offset; Bytef buf[1024];