Skip to content

Commit 0aa622e

Browse files
joyeecheungV8 LUCI CQ
joyeecheung
authored and
V8 LUCI CQ
committed
[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}
1 parent aa58acd commit 0aa622e

File tree

7 files changed

+101
-34
lines changed

7 files changed

+101
-34
lines changed

‎include/v8-object.h

+16-7
Original file line numberDiff line numberDiff line change
@@ -483,11 +483,20 @@ class V8_EXPORT Object : public Value {
483483
return object.template value<Object>()->InternalFieldCount();
484484
}
485485

486-
/** Gets the value from an internal field. */
487-
V8_INLINE Local<Value> GetInternalField(int index);
486+
/**
487+
* Gets the data from an internal field.
488+
* To cast the return value into v8::Value subtypes, it needs to be
489+
* casted to a v8::Value first. For example, to cast it into v8::External:
490+
*
491+
* object->GetInternalField(index).As<v8::Value>().As<v8::External>();
492+
*
493+
* The embedder should make sure that the internal field being retrieved
494+
* using this method has already been set with SetInternalField() before.
495+
**/
496+
V8_INLINE Local<Data> GetInternalField(int index);
488497

489-
/** Sets the value in an internal field. */
490-
void SetInternalField(int index, Local<Value> value);
498+
/** Sets the data in an internal field. */
499+
void SetInternalField(int index, Local<Data> data);
491500

492501
/**
493502
* Gets a 2-byte-aligned native pointer from an internal field. This field
@@ -725,13 +734,13 @@ class V8_EXPORT Object : public Value {
725734
private:
726735
Object();
727736
static void CheckCast(Value* obj);
728-
Local<Value> SlowGetInternalField(int index);
737+
Local<Data> SlowGetInternalField(int index);
729738
void* SlowGetAlignedPointerFromInternalField(int index);
730739
};
731740

732741
// --- Implementation ---
733742

734-
Local<Value> Object::GetInternalField(int index) {
743+
Local<Data> Object::GetInternalField(int index) {
735744
#ifndef V8_ENABLE_CHECKS
736745
using A = internal::Address;
737746
using I = internal::Internals;
@@ -750,7 +759,7 @@ Local<Value> Object::GetInternalField(int index) {
750759

751760
auto isolate = reinterpret_cast<v8::Isolate*>(
752761
internal::IsolateFromNeverReadOnlySpaceObject(obj));
753-
return Local<Value>::New(isolate, value);
762+
return Local<Data>::New(isolate, value);
754763
}
755764
#endif
756765
return SlowGetInternalField(index);

‎samples/process.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@ Local<Object> JsHttpRequestProcessor::WrapMap(map<string, string>* obj) {
386386
// Utility function that extracts the C++ map pointer from a wrapper
387387
// object.
388388
map<string, string>* JsHttpRequestProcessor::UnwrapMap(Local<Object> obj) {
389-
Local<External> field = obj->GetInternalField(0).As<External>();
389+
Local<External> field = obj->GetInternalField(0).As<Value>().As<External>();
390390
void* ptr = field->Value();
391391
return static_cast<map<string, string>*>(ptr);
392392
}
@@ -502,7 +502,7 @@ Local<Object> JsHttpRequestProcessor::WrapRequest(HttpRequest* request) {
502502
* wrapper object.
503503
*/
504504
HttpRequest* JsHttpRequestProcessor::UnwrapRequest(Local<Object> obj) {
505-
Local<External> field = obj->GetInternalField(0).As<External>();
505+
Local<External> field = obj->GetInternalField(0).As<Value>().As<External>();
506506
void* ptr = field->Value();
507507
return static_cast<HttpRequest*>(ptr);
508508
}

‎src/api/api.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -6230,16 +6230,16 @@ static bool InternalFieldOK(i::Handle<i::JSReceiver> obj, int index,
62306230
location, "Internal field out of bounds");
62316231
}
62326232

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

6242-
void v8::Object::SetInternalField(int index, v8::Local<Value> value) {
6242+
void v8::Object::SetInternalField(int index, v8::Local<Data> value) {
62436243
i::Handle<i::JSReceiver> obj = Utils::OpenHandle(this);
62446244
const char* location = "v8::Object::SetInternalField()";
62456245
if (!InternalFieldOK(obj, index, location)) return;

‎test/cctest/test-api.cc

+55-9
Original file line numberDiff line numberDiff line change
@@ -2884,6 +2884,43 @@ THREADED_TEST(FunctionPrototype) {
28842884
CHECK_EQ(v8_run_int32value(script), 321);
28852885
}
28862886

2887+
bool internal_field_check_called = false;
2888+
void OnInternalFieldCheck(const char* location, const char* message) {
2889+
internal_field_check_called = true;
2890+
exit(strcmp(location, "v8::Value::Cast") +
2891+
strcmp(message, "Data is not a Value"));
2892+
}
2893+
2894+
THREADED_TEST(InternalDataFields) {
2895+
LocalContext env;
2896+
v8::Isolate* isolate = env->GetIsolate();
2897+
v8::HandleScope scope(isolate);
2898+
2899+
Local<v8::FunctionTemplate> templ = v8::FunctionTemplate::New(isolate);
2900+
Local<v8::ObjectTemplate> instance_templ = templ->InstanceTemplate();
2901+
instance_templ->SetInternalFieldCount(1);
2902+
Local<v8::Object> obj = templ->GetFunction(env.local())
2903+
.ToLocalChecked()
2904+
->NewInstance(env.local())
2905+
.ToLocalChecked();
2906+
CHECK_EQ(1, obj->InternalFieldCount());
2907+
Local<v8::Data> data = obj->GetInternalField(0);
2908+
CHECK(data->IsValue() && data.As<v8::Value>()->IsUndefined());
2909+
Local<v8::Private> sym = v8::Private::New(isolate, v8_str("Foo"));
2910+
obj->SetInternalField(0, sym);
2911+
Local<v8::Data> field = obj->GetInternalField(0);
2912+
CHECK(!field->IsValue());
2913+
CHECK(field->IsPrivate());
2914+
CHECK_EQ(sym, field);
2915+
2916+
#ifdef V8_ENABLE_CHECKS
2917+
isolate->SetFatalErrorHandler(OnInternalFieldCheck);
2918+
USE(obj->GetInternalField(0).As<v8::Value>());
2919+
// If it's never called this would fail.
2920+
CHECK(internal_field_check_called);
2921+
#endif
2922+
}
2923+
28872924
THREADED_TEST(InternalFields) {
28882925
LocalContext env;
28892926
v8::Isolate* isolate = env->GetIsolate();
@@ -2897,9 +2934,12 @@ THREADED_TEST(InternalFields) {
28972934
->NewInstance(env.local())
28982935
.ToLocalChecked();
28992936
CHECK_EQ(1, obj->InternalFieldCount());
2900-
CHECK(obj->GetInternalField(0)->IsUndefined());
2937+
CHECK(obj->GetInternalField(0).As<v8::Value>()->IsUndefined());
29012938
obj->SetInternalField(0, v8_num(17));
2902-
CHECK_EQ(17, obj->GetInternalField(0)->Int32Value(env.local()).FromJust());
2939+
CHECK_EQ(17, obj->GetInternalField(0)
2940+
.As<v8::Value>()
2941+
->Int32Value(env.local())
2942+
.FromJust());
29032943
}
29042944

29052945
TEST(InternalFieldsSubclassing) {
@@ -2925,14 +2965,16 @@ TEST(InternalFieldsSubclassing) {
29252965
CHECK_EQ(0, i_obj->map()->GetInObjectProperties());
29262966
// Check writing and reading internal fields.
29272967
for (int j = 0; j < nof_embedder_fields; j++) {
2928-
CHECK(obj->GetInternalField(j)->IsUndefined());
2968+
CHECK(obj->GetInternalField(j).As<v8::Value>()->IsUndefined());
29292969
int value = 17 + j;
29302970
obj->SetInternalField(j, v8_num(value));
29312971
}
29322972
for (int j = 0; j < nof_embedder_fields; j++) {
29332973
int value = 17 + j;
2934-
CHECK_EQ(value,
2935-
obj->GetInternalField(j)->Int32Value(env.local()).FromJust());
2974+
CHECK_EQ(value, obj->GetInternalField(j)
2975+
.As<v8::Value>()
2976+
->Int32Value(env.local())
2977+
.FromJust());
29362978
}
29372979
CHECK(env->Global()
29382980
->Set(env.local(), v8_str("BaseClass"), constructor)
@@ -3032,9 +3074,12 @@ THREADED_TEST(GlobalObjectInternalFields) {
30323074
v8::Local<v8::Object> global_proxy = env->Global();
30333075
v8::Local<v8::Object> global = global_proxy->GetPrototype().As<v8::Object>();
30343076
CHECK_EQ(1, global->InternalFieldCount());
3035-
CHECK(global->GetInternalField(0)->IsUndefined());
3077+
CHECK(global->GetInternalField(0).As<v8::Value>()->IsUndefined());
30363078
global->SetInternalField(0, v8_num(17));
3037-
CHECK_EQ(17, global->GetInternalField(0)->Int32Value(env.local()).FromJust());
3079+
CHECK_EQ(17, global->GetInternalField(0)
3080+
.As<v8::Value>()
3081+
->Int32Value(env.local())
3082+
.FromJust());
30383083
}
30393084

30403085

@@ -7789,7 +7834,7 @@ void InternalFieldCallback(bool global_gc) {
77897834
.ToLocalChecked();
77907835
handle.Reset(isolate, obj);
77917836
CHECK_EQ(2, obj->InternalFieldCount());
7792-
CHECK(obj->GetInternalField(0)->IsUndefined());
7837+
CHECK(obj->GetInternalField(0).As<v8::Value>()->IsUndefined());
77937838
t1 = new Trivial(42);
77947839
t2 = new Trivial2(103, 9);
77957840

@@ -29697,7 +29742,8 @@ class HiddenDataDelegate : public v8::Context::DeepFreezeDelegate {
2969729742
std::vector<v8::Local<v8::Object>>& children_out) override {
2969829743
int fields = obj->InternalFieldCount();
2969929744
for (int idx = 0; idx < fields; idx++) {
29700-
v8::Local<v8::Value> child_value = obj->GetInternalField(idx);
29745+
v8::Local<v8::Value> child_value =
29746+
obj->GetInternalField(idx).As<v8::Value>();
2970129747
if (child_value->IsExternal()) {
2970229748
if (!FreezeExternal(v8::Local<v8::External>::Cast(child_value),
2970329749
children_out)) {

‎test/cctest/test-api.h

+5-3
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,11 @@ template <typename T>
4646
static void CheckInternalFieldsAreZero(v8::Local<T> value) {
4747
CHECK_EQ(T::kInternalFieldCount, value->InternalFieldCount());
4848
for (int i = 0; i < value->InternalFieldCount(); i++) {
49-
CHECK_EQ(0, value->GetInternalField(i)
50-
->Int32Value(CcTest::isolate()->GetCurrentContext())
51-
.FromJust());
49+
v8::Local<v8::Value> field =
50+
value->GetInternalField(i).template As<v8::Value>();
51+
CHECK_EQ(
52+
0,
53+
field->Int32Value(CcTest::isolate()->GetCurrentContext()).FromJust());
5254
}
5355
}
5456

‎test/cctest/test-serialize.cc

+12-8
Original file line numberDiff line numberDiff line change
@@ -3667,23 +3667,27 @@ UNINITIALIZED_TEST(SnapshotCreatorTemplates) {
36673667
.ToLocalChecked()
36683668
->ToObject(context)
36693669
.ToLocalChecked();
3670-
v8::Local<v8::Object> b =
3671-
a->GetInternalField(0)->ToObject(context).ToLocalChecked();
3672-
v8::Local<v8::Object> c =
3673-
b->GetInternalField(0)->ToObject(context).ToLocalChecked();
3670+
v8::Local<v8::Object> b = a->GetInternalField(0)
3671+
.As<v8::Value>()
3672+
->ToObject(context)
3673+
.ToLocalChecked();
3674+
v8::Local<v8::Object> c = b->GetInternalField(0)
3675+
.As<v8::Value>()
3676+
->ToObject(context)
3677+
.ToLocalChecked();
36743678

36753679
InternalFieldData* a1 = reinterpret_cast<InternalFieldData*>(
36763680
a->GetAlignedPointerFromInternalField(1));
3677-
v8::Local<v8::Value> a2 = a->GetInternalField(2);
3681+
v8::Local<v8::Value> a2 = a->GetInternalField(2).As<v8::Value>();
36783682

36793683
InternalFieldData* b1 = reinterpret_cast<InternalFieldData*>(
36803684
b->GetAlignedPointerFromInternalField(1));
3681-
v8::Local<v8::Value> b2 = b->GetInternalField(2);
3685+
v8::Local<v8::Value> b2 = b->GetInternalField(2).As<v8::Value>();
36823686

3683-
v8::Local<v8::Value> c0 = c->GetInternalField(0);
3687+
v8::Local<v8::Value> c0 = c->GetInternalField(0).As<v8::Value>();
36843688
InternalFieldData* c1 = reinterpret_cast<InternalFieldData*>(
36853689
c->GetAlignedPointerFromInternalField(1));
3686-
v8::Local<v8::Value> c2 = c->GetInternalField(2);
3690+
v8::Local<v8::Value> c2 = c->GetInternalField(2).As<v8::Value>();
36873691

36883692
CHECK(c0->IsUndefined());
36893693

‎test/unittests/objects/value-serializer-unittest.cc

+8-2
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,15 @@ class ValueSerializerTest : public TestWithIsolate {
6363
StringFromUtf8("value"),
6464
[](Local<String> property, const PropertyCallbackInfo<Value>& info) {
6565
CHECK(i::ValidateCallbackInfo(info));
66-
info.GetReturnValue().Set(info.Holder()->GetInternalField(0));
66+
info.GetReturnValue().Set(
67+
info.Holder()->GetInternalField(0).As<v8::Value>());
6768
});
6869
function_template->InstanceTemplate()->SetAccessor(
6970
StringFromUtf8("value2"),
7071
[](Local<String> property, const PropertyCallbackInfo<Value>& info) {
7172
CHECK(i::ValidateCallbackInfo(info));
72-
info.GetReturnValue().Set(info.Holder()->GetInternalField(1));
73+
info.GetReturnValue().Set(
74+
info.Holder()->GetInternalField(1).As<v8::Value>());
7375
});
7476
for (Local<Context> context :
7577
{serialization_context_, deserialization_context_}) {
@@ -2884,6 +2886,7 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripUint32) {
28842886
.WillRepeatedly(Invoke([this](Isolate*, Local<Object> object) {
28852887
uint32_t value = 0;
28862888
EXPECT_TRUE(object->GetInternalField(0)
2889+
.As<v8::Value>()
28872890
->Uint32Value(serialization_context())
28882891
.To(&value));
28892892
WriteExampleHostObjectTag();
@@ -2915,9 +2918,11 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripUint64) {
29152918
.WillRepeatedly(Invoke([this](Isolate*, Local<Object> object) {
29162919
uint32_t value = 0, value2 = 0;
29172920
EXPECT_TRUE(object->GetInternalField(0)
2921+
.As<v8::Value>()
29182922
->Uint32Value(serialization_context())
29192923
.To(&value));
29202924
EXPECT_TRUE(object->GetInternalField(1)
2925+
.As<v8::Value>()
29212926
->Uint32Value(serialization_context())
29222927
.To(&value2));
29232928
WriteExampleHostObjectTag();
@@ -2955,6 +2960,7 @@ TEST_F(ValueSerializerTestWithHostObject, RoundTripDouble) {
29552960
.WillRepeatedly(Invoke([this](Isolate*, Local<Object> object) {
29562961
double value = 0;
29572962
EXPECT_TRUE(object->GetInternalField(0)
2963+
.As<v8::Value>()
29582964
->NumberValue(serialization_context())
29592965
.To(&value));
29602966
WriteExampleHostObjectTag();

0 commit comments

Comments
 (0)