From 867586ce953fdd807162faba24af3dd0afafaa60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Fri, 15 Sep 2023 14:24:43 +0200 Subject: [PATCH] deps: V8: cherry-pick 93b1a74cbc9b Original commit message: Reland "[api] allow v8::Data as internal field" This is a reland of commit 0aa622e12893e9921c01a34ce9507b544e599c4a The original patch tried to run a test that calls exit() in the fatal error handler in parallel, which would not work. This marked the test with TEST() to avoid running it in parallel. Original change's description: > [api] allow v8::Data as internal field > > Previously only v8::Value can be stored as internal fields. > In some cases, however, it's necessary for the embedder to > tie the lifetime of a v8::Data with the lifetime of a > JS object, and that v8::Data may not be a v8::Value, as > it can be something returned from the V8 API. One way to > keep the v8::Data alive may be to use a v8::Persistent > but that can easily lead to leaks. > > This patch changes v8::Object::GetInternalField() and > v8::Object::SetInernalField() to accept v8::Data instead of just > v8::Value, so that v8::Data can kept alive by a JS object in > a way that the GC can be aware of to address this problem. > This is a breaking change for embedders > using v8::Object::GetInternalField() as it changes the return > type. Since most v8::Value subtypes only support direct casts > from v8::Value but not v8::Data, calls like > > object->GetInternalField(index).As() > > needs to be updated to cast the value to v8::Value first: > > object->GetInternalField(index).As().As() > > Bug: v8:14120 > Change-Id: I731c958d1756b9d5ee4a3e78813416cd60d1b7ca > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4707972 > Reviewed-by: Michael Lippautz > Commit-Queue: Joyee Cheung > Cr-Commit-Position: refs/heads/main@{#89718} Bug: v8:14120 Change-Id: I3e45d09b5c300d5eefc73e380ef21ac2bd61760c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4834471 Commit-Queue: Joyee Cheung Reviewed-by: Camillo Bruni Cr-Commit-Position: refs/heads/main@{#89824} Refs: https://github.com/v8/v8/commit/93b1a74cbc9b5382a145051b9475433bc6107f1e PR-URL: https://github.com/nodejs/node/pull/49639 Reviewed-By: Jiawen Geng Reviewed-By: Rafael Gonzaga Reviewed-By: Antoine du Hamel --- common.gypi | 2 +- deps/v8/include/v8-object.h | 23 +++++-- deps/v8/samples/process.cc | 4 +- deps/v8/src/api/api.cc | 6 +- deps/v8/test/cctest/test-api.cc | 66 ++++++++++++++++--- deps/v8/test/cctest/test-api.h | 8 ++- deps/v8/test/cctest/test-serialize.cc | 20 +++--- .../objects/value-serializer-unittest.cc | 10 ++- 8 files changed, 104 insertions(+), 35 deletions(-) diff --git a/common.gypi b/common.gypi index f2d4663e0bbc17..c51f2d5ec72d2b 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.7', + 'v8_embedder_string': '-node.8', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/include/v8-object.h b/deps/v8/include/v8-object.h index 3b50651f2eb968..45660924dd055a 100644 --- a/deps/v8/include/v8-object.h +++ b/deps/v8/include/v8-object.h @@ -483,11 +483,20 @@ class V8_EXPORT Object : public Value { return object.template value()->InternalFieldCount(); } - /** Gets the value from an internal field. */ - V8_INLINE Local GetInternalField(int index); + /** + * Gets the data from an internal field. + * To cast the return value into v8::Value subtypes, it needs to be + * casted to a v8::Value first. For example, to cast it into v8::External: + * + * object->GetInternalField(index).As().As(); + * + * The embedder should make sure that the internal field being retrieved + * using this method has already been set with SetInternalField() before. + **/ + V8_INLINE Local GetInternalField(int index); - /** Sets the value in an internal field. */ - void SetInternalField(int index, Local value); + /** Sets the data in an internal field. */ + void SetInternalField(int index, Local data); /** * Gets a 2-byte-aligned native pointer from an internal field. This field @@ -725,13 +734,13 @@ class V8_EXPORT Object : public Value { private: Object(); static void CheckCast(Value* obj); - Local SlowGetInternalField(int index); + Local SlowGetInternalField(int index); void* SlowGetAlignedPointerFromInternalField(int index); }; // --- Implementation --- -Local Object::GetInternalField(int index) { +Local Object::GetInternalField(int index) { #ifndef V8_ENABLE_CHECKS using A = internal::Address; using I = internal::Internals; @@ -750,7 +759,7 @@ Local Object::GetInternalField(int index) { auto isolate = reinterpret_cast( internal::IsolateFromNeverReadOnlySpaceObject(obj)); - return Local::New(isolate, value); + return Local::New(isolate, value); } #endif return SlowGetInternalField(index); diff --git a/deps/v8/samples/process.cc b/deps/v8/samples/process.cc index a58033936dd8d3..0af49e304ab7b3 100644 --- a/deps/v8/samples/process.cc +++ b/deps/v8/samples/process.cc @@ -386,7 +386,7 @@ Local JsHttpRequestProcessor::WrapMap(map* obj) { // Utility function that extracts the C++ map pointer from a wrapper // object. map* JsHttpRequestProcessor::UnwrapMap(Local obj) { - Local field = obj->GetInternalField(0).As(); + Local field = obj->GetInternalField(0).As().As(); void* ptr = field->Value(); return static_cast*>(ptr); } @@ -502,7 +502,7 @@ Local JsHttpRequestProcessor::WrapRequest(HttpRequest* request) { * wrapper object. */ HttpRequest* JsHttpRequestProcessor::UnwrapRequest(Local obj) { - Local field = obj->GetInternalField(0).As(); + Local field = obj->GetInternalField(0).As().As(); void* ptr = field->Value(); return static_cast(ptr); } diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc index f6505e6820bf4d..ddf93efc1bd8da 100644 --- a/deps/v8/src/api/api.cc +++ b/deps/v8/src/api/api.cc @@ -6246,16 +6246,16 @@ static bool InternalFieldOK(i::Handle obj, int index, location, "Internal field out of bounds"); } -Local v8::Object::SlowGetInternalField(int index) { +Local v8::Object::SlowGetInternalField(int index) { i::Handle obj = Utils::OpenHandle(this); const char* location = "v8::Object::GetInternalField()"; if (!InternalFieldOK(obj, index, location)) return Local(); i::Handle value(i::JSObject::cast(*obj)->GetEmbedderField(index), obj->GetIsolate()); - return Utils::ToLocal(value); + return ToApiHandle(value); } -void v8::Object::SetInternalField(int index, v8::Local value) { +void v8::Object::SetInternalField(int index, v8::Local value) { i::Handle obj = Utils::OpenHandle(this); const char* location = "v8::Object::SetInternalField()"; if (!InternalFieldOK(obj, index, location)) return; diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index adb4b4277fb6fb..6c80599b571793 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -2884,6 +2884,45 @@ THREADED_TEST(FunctionPrototype) { CHECK_EQ(v8_run_int32value(script), 321); } +bool internal_field_check_called = false; +void OnInternalFieldCheck(const char* location, const char* message) { + internal_field_check_called = true; + exit(strcmp(location, "v8::Value::Cast") + + strcmp(message, "Data is not a Value")); +} + +// The fatal error handler would call exit() so this should not be run in +// parallel. +TEST(InternalDataFields) { + LocalContext env; + v8::Isolate* isolate = env->GetIsolate(); + v8::HandleScope scope(isolate); + + Local templ = v8::FunctionTemplate::New(isolate); + Local instance_templ = templ->InstanceTemplate(); + instance_templ->SetInternalFieldCount(1); + Local obj = templ->GetFunction(env.local()) + .ToLocalChecked() + ->NewInstance(env.local()) + .ToLocalChecked(); + CHECK_EQ(1, obj->InternalFieldCount()); + Local data = obj->GetInternalField(0); + CHECK(data->IsValue() && data.As()->IsUndefined()); + Local sym = v8::Private::New(isolate, v8_str("Foo")); + obj->SetInternalField(0, sym); + Local field = obj->GetInternalField(0); + CHECK(!field->IsValue()); + CHECK(field->IsPrivate()); + CHECK_EQ(sym, field); + +#ifdef V8_ENABLE_CHECKS + isolate->SetFatalErrorHandler(OnInternalFieldCheck); + USE(obj->GetInternalField(0).As()); + // If it's never called this would fail. + CHECK(internal_field_check_called); +#endif +} + THREADED_TEST(InternalFields) { LocalContext env; v8::Isolate* isolate = env->GetIsolate(); @@ -2897,9 +2936,12 @@ THREADED_TEST(InternalFields) { ->NewInstance(env.local()) .ToLocalChecked(); CHECK_EQ(1, obj->InternalFieldCount()); - CHECK(obj->GetInternalField(0)->IsUndefined()); + CHECK(obj->GetInternalField(0).As()->IsUndefined()); obj->SetInternalField(0, v8_num(17)); - CHECK_EQ(17, obj->GetInternalField(0)->Int32Value(env.local()).FromJust()); + CHECK_EQ(17, obj->GetInternalField(0) + .As() + ->Int32Value(env.local()) + .FromJust()); } TEST(InternalFieldsSubclassing) { @@ -2925,14 +2967,16 @@ TEST(InternalFieldsSubclassing) { CHECK_EQ(0, i_obj->map()->GetInObjectProperties()); // Check writing and reading internal fields. for (int j = 0; j < nof_embedder_fields; j++) { - CHECK(obj->GetInternalField(j)->IsUndefined()); + CHECK(obj->GetInternalField(j).As()->IsUndefined()); int value = 17 + j; obj->SetInternalField(j, v8_num(value)); } for (int j = 0; j < nof_embedder_fields; j++) { int value = 17 + j; - CHECK_EQ(value, - obj->GetInternalField(j)->Int32Value(env.local()).FromJust()); + CHECK_EQ(value, obj->GetInternalField(j) + .As() + ->Int32Value(env.local()) + .FromJust()); } CHECK(env->Global() ->Set(env.local(), v8_str("BaseClass"), constructor) @@ -3032,9 +3076,12 @@ THREADED_TEST(GlobalObjectInternalFields) { v8::Local global_proxy = env->Global(); v8::Local global = global_proxy->GetPrototype().As(); CHECK_EQ(1, global->InternalFieldCount()); - CHECK(global->GetInternalField(0)->IsUndefined()); + CHECK(global->GetInternalField(0).As()->IsUndefined()); global->SetInternalField(0, v8_num(17)); - CHECK_EQ(17, global->GetInternalField(0)->Int32Value(env.local()).FromJust()); + CHECK_EQ(17, global->GetInternalField(0) + .As() + ->Int32Value(env.local()) + .FromJust()); } @@ -7789,7 +7836,7 @@ void InternalFieldCallback(bool global_gc) { .ToLocalChecked(); handle.Reset(isolate, obj); CHECK_EQ(2, obj->InternalFieldCount()); - CHECK(obj->GetInternalField(0)->IsUndefined()); + CHECK(obj->GetInternalField(0).As()->IsUndefined()); t1 = new Trivial(42); t2 = new Trivial2(103, 9); @@ -29699,7 +29746,8 @@ class HiddenDataDelegate : public v8::Context::DeepFreezeDelegate { std::vector>& children_out) override { int fields = obj->InternalFieldCount(); for (int idx = 0; idx < fields; idx++) { - v8::Local child_value = obj->GetInternalField(idx); + v8::Local child_value = + obj->GetInternalField(idx).As(); if (child_value->IsExternal()) { if (!FreezeExternal(v8::Local::Cast(child_value), children_out)) { diff --git a/deps/v8/test/cctest/test-api.h b/deps/v8/test/cctest/test-api.h index 7c4d0ece4e4cbf..5ecf1e3ba97295 100644 --- a/deps/v8/test/cctest/test-api.h +++ b/deps/v8/test/cctest/test-api.h @@ -46,9 +46,11 @@ template static void CheckInternalFieldsAreZero(v8::Local value) { CHECK_EQ(T::kInternalFieldCount, value->InternalFieldCount()); for (int i = 0; i < value->InternalFieldCount(); i++) { - CHECK_EQ(0, value->GetInternalField(i) - ->Int32Value(CcTest::isolate()->GetCurrentContext()) - .FromJust()); + v8::Local field = + value->GetInternalField(i).template As(); + CHECK_EQ( + 0, + field->Int32Value(CcTest::isolate()->GetCurrentContext()).FromJust()); } } diff --git a/deps/v8/test/cctest/test-serialize.cc b/deps/v8/test/cctest/test-serialize.cc index 4a7e5e84e88414..36f131630d6fd4 100644 --- a/deps/v8/test/cctest/test-serialize.cc +++ b/deps/v8/test/cctest/test-serialize.cc @@ -3667,23 +3667,27 @@ UNINITIALIZED_TEST(SnapshotCreatorTemplates) { .ToLocalChecked() ->ToObject(context) .ToLocalChecked(); - v8::Local b = - a->GetInternalField(0)->ToObject(context).ToLocalChecked(); - v8::Local c = - b->GetInternalField(0)->ToObject(context).ToLocalChecked(); + v8::Local b = a->GetInternalField(0) + .As() + ->ToObject(context) + .ToLocalChecked(); + v8::Local c = b->GetInternalField(0) + .As() + ->ToObject(context) + .ToLocalChecked(); InternalFieldData* a1 = reinterpret_cast( a->GetAlignedPointerFromInternalField(1)); - v8::Local a2 = a->GetInternalField(2); + v8::Local a2 = a->GetInternalField(2).As(); InternalFieldData* b1 = reinterpret_cast( b->GetAlignedPointerFromInternalField(1)); - v8::Local b2 = b->GetInternalField(2); + v8::Local b2 = b->GetInternalField(2).As(); - v8::Local c0 = c->GetInternalField(0); + v8::Local c0 = c->GetInternalField(0).As(); InternalFieldData* c1 = reinterpret_cast( c->GetAlignedPointerFromInternalField(1)); - v8::Local c2 = c->GetInternalField(2); + v8::Local c2 = c->GetInternalField(2).As(); CHECK(c0->IsUndefined()); diff --git a/deps/v8/test/unittests/objects/value-serializer-unittest.cc b/deps/v8/test/unittests/objects/value-serializer-unittest.cc index fc86e6e3031fb4..deed350e31430b 100644 --- a/deps/v8/test/unittests/objects/value-serializer-unittest.cc +++ b/deps/v8/test/unittests/objects/value-serializer-unittest.cc @@ -63,13 +63,15 @@ class ValueSerializerTest : public TestWithIsolate { StringFromUtf8("value"), [](Local property, const PropertyCallbackInfo& info) { CHECK(i::ValidateCallbackInfo(info)); - info.GetReturnValue().Set(info.Holder()->GetInternalField(0)); + info.GetReturnValue().Set( + info.Holder()->GetInternalField(0).As()); }); function_template->InstanceTemplate()->SetAccessor( StringFromUtf8("value2"), [](Local property, const PropertyCallbackInfo& info) { CHECK(i::ValidateCallbackInfo(info)); - info.GetReturnValue().Set(info.Holder()->GetInternalField(1)); + info.GetReturnValue().Set( + info.Holder()->GetInternalField(1).As()); }); for (Local context : {serialization_context_, deserialization_context_}) { @@ -2884,6 +2886,7 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripUint32) { .WillRepeatedly(Invoke([this](Isolate*, Local object) { uint32_t value = 0; EXPECT_TRUE(object->GetInternalField(0) + .As() ->Uint32Value(serialization_context()) .To(&value)); WriteExampleHostObjectTag(); @@ -2915,9 +2918,11 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripUint64) { .WillRepeatedly(Invoke([this](Isolate*, Local object) { uint32_t value = 0, value2 = 0; EXPECT_TRUE(object->GetInternalField(0) + .As() ->Uint32Value(serialization_context()) .To(&value)); EXPECT_TRUE(object->GetInternalField(1) + .As() ->Uint32Value(serialization_context()) .To(&value2)); WriteExampleHostObjectTag(); @@ -2955,6 +2960,7 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripDouble) { .WillRepeatedly(Invoke([this](Isolate*, Local object) { double value = 0; EXPECT_TRUE(object->GetInternalField(0) + .As() ->NumberValue(serialization_context()) .To(&value)); WriteExampleHostObjectTag();