Skip to content

Commit

Permalink
deps: V8: cherry-pick 93b1a74cbc9b
Browse files Browse the repository at this point in the history
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<v8::Data>
    > 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<v8::External>()
    >
    > needs to be updated to cast the value to v8::Value first:
    >
    > object->GetInternalField(index).As<v8::Value>().As<v8::External>()
    >
    > Bug: v8:14120
    > Change-Id: I731c958d1756b9d5ee4a3e78813416cd60d1b7ca
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4707972
    > Reviewed-by: Michael Lippautz <[email protected]>
    > Commit-Queue: Joyee Cheung <[email protected]>
    > 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 <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#89824}

Refs: v8/v8@93b1a74
PR-URL: #49639
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
  • Loading branch information
targos committed Oct 10, 2023
1 parent 4ad3479 commit 867586c
Show file tree
Hide file tree
Showing 8 changed files with 104 additions and 35 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -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 #####

Expand Down
23 changes: 16 additions & 7 deletions deps/v8/include/v8-object.h
Original file line number Diff line number Diff line change
Expand Up @@ -483,11 +483,20 @@ class V8_EXPORT Object : public Value {
return object.template value<Object>()->InternalFieldCount();
}

/** Gets the value from an internal field. */
V8_INLINE Local<Value> 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<v8::Value>().As<v8::External>();
*
* The embedder should make sure that the internal field being retrieved
* using this method has already been set with SetInternalField() before.
**/
V8_INLINE Local<Data> GetInternalField(int index);

/** Sets the value in an internal field. */
void SetInternalField(int index, Local<Value> value);
/** Sets the data in an internal field. */
void SetInternalField(int index, Local<Data> data);

/**
* Gets a 2-byte-aligned native pointer from an internal field. This field
Expand Down Expand Up @@ -725,13 +734,13 @@ class V8_EXPORT Object : public Value {
private:
Object();
static void CheckCast(Value* obj);
Local<Value> SlowGetInternalField(int index);
Local<Data> SlowGetInternalField(int index);
void* SlowGetAlignedPointerFromInternalField(int index);
};

// --- Implementation ---

Local<Value> Object::GetInternalField(int index) {
Local<Data> Object::GetInternalField(int index) {
#ifndef V8_ENABLE_CHECKS
using A = internal::Address;
using I = internal::Internals;
Expand All @@ -750,7 +759,7 @@ Local<Value> Object::GetInternalField(int index) {

auto isolate = reinterpret_cast<v8::Isolate*>(
internal::IsolateFromNeverReadOnlySpaceObject(obj));
return Local<Value>::New(isolate, value);
return Local<Data>::New(isolate, value);
}
#endif
return SlowGetInternalField(index);
Expand Down
4 changes: 2 additions & 2 deletions deps/v8/samples/process.cc
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ Local<Object> JsHttpRequestProcessor::WrapMap(map<string, string>* obj) {
// Utility function that extracts the C++ map pointer from a wrapper
// object.
map<string, string>* JsHttpRequestProcessor::UnwrapMap(Local<Object> obj) {
Local<External> field = obj->GetInternalField(0).As<External>();
Local<External> field = obj->GetInternalField(0).As<Value>().As<External>();
void* ptr = field->Value();
return static_cast<map<string, string>*>(ptr);
}
Expand Down Expand Up @@ -502,7 +502,7 @@ Local<Object> JsHttpRequestProcessor::WrapRequest(HttpRequest* request) {
* wrapper object.
*/
HttpRequest* JsHttpRequestProcessor::UnwrapRequest(Local<Object> obj) {
Local<External> field = obj->GetInternalField(0).As<External>();
Local<External> field = obj->GetInternalField(0).As<Value>().As<External>();
void* ptr = field->Value();
return static_cast<HttpRequest*>(ptr);
}
Expand Down
6 changes: 3 additions & 3 deletions deps/v8/src/api/api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6246,16 +6246,16 @@ static bool InternalFieldOK(i::Handle<i::JSReceiver> obj, int index,
location, "Internal field out of bounds");
}

Local<Value> v8::Object::SlowGetInternalField(int index) {
Local<Data> v8::Object::SlowGetInternalField(int index) {
i::Handle<i::JSReceiver> obj = Utils::OpenHandle(this);
const char* location = "v8::Object::GetInternalField()";
if (!InternalFieldOK(obj, index, location)) return Local<Value>();
i::Handle<i::Object> value(i::JSObject::cast(*obj)->GetEmbedderField(index),
obj->GetIsolate());
return Utils::ToLocal(value);
return ToApiHandle<Data>(value);
}

void v8::Object::SetInternalField(int index, v8::Local<Value> value) {
void v8::Object::SetInternalField(int index, v8::Local<Data> value) {
i::Handle<i::JSReceiver> obj = Utils::OpenHandle(this);
const char* location = "v8::Object::SetInternalField()";
if (!InternalFieldOK(obj, index, location)) return;
Expand Down
66 changes: 57 additions & 9 deletions deps/v8/test/cctest/test-api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::FunctionTemplate> templ = v8::FunctionTemplate::New(isolate);
Local<v8::ObjectTemplate> instance_templ = templ->InstanceTemplate();
instance_templ->SetInternalFieldCount(1);
Local<v8::Object> obj = templ->GetFunction(env.local())
.ToLocalChecked()
->NewInstance(env.local())
.ToLocalChecked();
CHECK_EQ(1, obj->InternalFieldCount());
Local<v8::Data> data = obj->GetInternalField(0);
CHECK(data->IsValue() && data.As<v8::Value>()->IsUndefined());
Local<v8::Private> sym = v8::Private::New(isolate, v8_str("Foo"));
obj->SetInternalField(0, sym);
Local<v8::Data> 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<v8::Value>());
// If it's never called this would fail.
CHECK(internal_field_check_called);
#endif
}

THREADED_TEST(InternalFields) {
LocalContext env;
v8::Isolate* isolate = env->GetIsolate();
Expand All @@ -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<v8::Value>()->IsUndefined());
obj->SetInternalField(0, v8_num(17));
CHECK_EQ(17, obj->GetInternalField(0)->Int32Value(env.local()).FromJust());
CHECK_EQ(17, obj->GetInternalField(0)
.As<v8::Value>()
->Int32Value(env.local())
.FromJust());
}

TEST(InternalFieldsSubclassing) {
Expand All @@ -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<v8::Value>()->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<v8::Value>()
->Int32Value(env.local())
.FromJust());
}
CHECK(env->Global()
->Set(env.local(), v8_str("BaseClass"), constructor)
Expand Down Expand Up @@ -3032,9 +3076,12 @@ THREADED_TEST(GlobalObjectInternalFields) {
v8::Local<v8::Object> global_proxy = env->Global();
v8::Local<v8::Object> global = global_proxy->GetPrototype().As<v8::Object>();
CHECK_EQ(1, global->InternalFieldCount());
CHECK(global->GetInternalField(0)->IsUndefined());
CHECK(global->GetInternalField(0).As<v8::Value>()->IsUndefined());
global->SetInternalField(0, v8_num(17));
CHECK_EQ(17, global->GetInternalField(0)->Int32Value(env.local()).FromJust());
CHECK_EQ(17, global->GetInternalField(0)
.As<v8::Value>()
->Int32Value(env.local())
.FromJust());
}


Expand Down Expand Up @@ -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<v8::Value>()->IsUndefined());
t1 = new Trivial(42);
t2 = new Trivial2(103, 9);

Expand Down Expand Up @@ -29699,7 +29746,8 @@ class HiddenDataDelegate : public v8::Context::DeepFreezeDelegate {
std::vector<v8::Local<v8::Object>>& children_out) override {
int fields = obj->InternalFieldCount();
for (int idx = 0; idx < fields; idx++) {
v8::Local<v8::Value> child_value = obj->GetInternalField(idx);
v8::Local<v8::Value> child_value =
obj->GetInternalField(idx).As<v8::Value>();
if (child_value->IsExternal()) {
if (!FreezeExternal(v8::Local<v8::External>::Cast(child_value),
children_out)) {
Expand Down
8 changes: 5 additions & 3 deletions deps/v8/test/cctest/test-api.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,11 @@ template <typename T>
static void CheckInternalFieldsAreZero(v8::Local<T> 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<v8::Value> field =
value->GetInternalField(i).template As<v8::Value>();
CHECK_EQ(
0,
field->Int32Value(CcTest::isolate()->GetCurrentContext()).FromJust());
}
}

Expand Down
20 changes: 12 additions & 8 deletions deps/v8/test/cctest/test-serialize.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3667,23 +3667,27 @@ UNINITIALIZED_TEST(SnapshotCreatorTemplates) {
.ToLocalChecked()
->ToObject(context)
.ToLocalChecked();
v8::Local<v8::Object> b =
a->GetInternalField(0)->ToObject(context).ToLocalChecked();
v8::Local<v8::Object> c =
b->GetInternalField(0)->ToObject(context).ToLocalChecked();
v8::Local<v8::Object> b = a->GetInternalField(0)
.As<v8::Value>()
->ToObject(context)
.ToLocalChecked();
v8::Local<v8::Object> c = b->GetInternalField(0)
.As<v8::Value>()
->ToObject(context)
.ToLocalChecked();

InternalFieldData* a1 = reinterpret_cast<InternalFieldData*>(
a->GetAlignedPointerFromInternalField(1));
v8::Local<v8::Value> a2 = a->GetInternalField(2);
v8::Local<v8::Value> a2 = a->GetInternalField(2).As<v8::Value>();

InternalFieldData* b1 = reinterpret_cast<InternalFieldData*>(
b->GetAlignedPointerFromInternalField(1));
v8::Local<v8::Value> b2 = b->GetInternalField(2);
v8::Local<v8::Value> b2 = b->GetInternalField(2).As<v8::Value>();

v8::Local<v8::Value> c0 = c->GetInternalField(0);
v8::Local<v8::Value> c0 = c->GetInternalField(0).As<v8::Value>();
InternalFieldData* c1 = reinterpret_cast<InternalFieldData*>(
c->GetAlignedPointerFromInternalField(1));
v8::Local<v8::Value> c2 = c->GetInternalField(2);
v8::Local<v8::Value> c2 = c->GetInternalField(2).As<v8::Value>();

CHECK(c0->IsUndefined());

Expand Down
10 changes: 8 additions & 2 deletions deps/v8/test/unittests/objects/value-serializer-unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,15 @@ class ValueSerializerTest : public TestWithIsolate {
StringFromUtf8("value"),
[](Local<String> property, const PropertyCallbackInfo<Value>& info) {
CHECK(i::ValidateCallbackInfo(info));
info.GetReturnValue().Set(info.Holder()->GetInternalField(0));
info.GetReturnValue().Set(
info.Holder()->GetInternalField(0).As<v8::Value>());
});
function_template->InstanceTemplate()->SetAccessor(
StringFromUtf8("value2"),
[](Local<String> property, const PropertyCallbackInfo<Value>& info) {
CHECK(i::ValidateCallbackInfo(info));
info.GetReturnValue().Set(info.Holder()->GetInternalField(1));
info.GetReturnValue().Set(
info.Holder()->GetInternalField(1).As<v8::Value>());
});
for (Local<Context> context :
{serialization_context_, deserialization_context_}) {
Expand Down Expand Up @@ -2884,6 +2886,7 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripUint32) {
.WillRepeatedly(Invoke([this](Isolate*, Local<Object> object) {
uint32_t value = 0;
EXPECT_TRUE(object->GetInternalField(0)
.As<v8::Value>()
->Uint32Value(serialization_context())
.To(&value));
WriteExampleHostObjectTag();
Expand Down Expand Up @@ -2915,9 +2918,11 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripUint64) {
.WillRepeatedly(Invoke([this](Isolate*, Local<Object> object) {
uint32_t value = 0, value2 = 0;
EXPECT_TRUE(object->GetInternalField(0)
.As<v8::Value>()
->Uint32Value(serialization_context())
.To(&value));
EXPECT_TRUE(object->GetInternalField(1)
.As<v8::Value>()
->Uint32Value(serialization_context())
.To(&value2));
WriteExampleHostObjectTag();
Expand Down Expand Up @@ -2955,6 +2960,7 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripDouble) {
.WillRepeatedly(Invoke([this](Isolate*, Local<Object> object) {
double value = 0;
EXPECT_TRUE(object->GetInternalField(0)
.As<v8::Value>()
->NumberValue(serialization_context())
.To(&value));
WriteExampleHostObjectTag();
Expand Down

0 comments on commit 867586c

Please sign in to comment.