FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: V8: backport 088b7112e7ab · nodejs/node@784431d · GitHub

/ node Public

Commit 784431d

Browse files
authored and
RafaelGSS
committed
deps: V8: backport 088b7112e7ab
Original commit message: [runtime] Fix contextual stores to global with interceptor According to the spec, contextual store in strict mode must first check whether property exists and if not, the ReferenceError should be thrown instead of calling the interceptor setter. See https://tc39.es/ecma262/#sec-object-environment-records-setmutablebinding-n-v-s Drive-by: - introduce new Api v8::Object::GetPropertyAttributes(..) which is able to return "property does not exist" result, which wasn't possible with the existing GetPropertyAttributes(..) Api, - update GenericInterceptor* callbacks in test-api-interceptors.cc to better suite for implementing a proxy-like interceptor. Bug: 455600234 Change-Id: I0986c18c406844f58c453e7aa7513c52a9097e04 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7718821 Reviewed-by: Leszek Swirski <leszeks@chromium.org> Commit-Queue: Igor Sheludko <ishell@chromium.org> Cr-Commit-Position: refs/heads/main@{#106322} Refs: v8/v8@088b711 Co-authored-by: Michaël Zasso <targos@protonmail.com> PR-URL: #61898 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> (cherry picked from commit 4dafa68)
1 parent 3839c4a commit 784431d

6 files changed

Lines changed: 237 additions & 10 deletions

File tree

‎common.gypi‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939
# Reset this number to 0 on major V8 upgrades.
4040
# Increment by one for each non-official patch applied to deps/v8.
41-
'v8_embedder_string': '-node.14',
41+
'v8_embedder_string': '-node.15',
4242

4343
##### V8 defaults for Node.js #####
4444

‎deps/v8/include/v8-object.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,9 +336,24 @@ class V8_EXPORT Object : public Value {
336336
* Gets the property attributes of a property which can be None or
337337
* any combination of ReadOnly, DontEnum and DontDelete. Returns
338338
* None when the property doesn't exist.
339+
*
340+
* This method will be deprecated soon, since it doesn't provide a way
341+
* to return "property does not exist" result. Use GetPropertyAttributes with
342+
* PropertyAttribute* instead.
339343
*/
340344
V8_WARN_UNUSED_RESULT Maybe<PropertyAttribute> GetPropertyAttributes(
341345
Local<Context> context, Local<Value> key);
346+
/**
347+
* Gets the property attributes of a property which can be None or
348+
* any combination of ReadOnly, DontEnum and DontDelete.
349+
*
350+
* Returns true and sets *out_attributes if the property exists, false if
351+
* not or empty Maybe if an exception is thrown. In the latter two cases,
352+
* the value of *out_attributes is not modified.
353+
*/
354+
V8_WARN_UNUSED_RESULT Maybe<bool> GetPropertyAttributes(
355+
Local<Context> context, Local<Value> key,
356+
PropertyAttribute* out_attributes);
342357

343358
/**
344359
* Implements Object.getOwnPropertyDescriptor(O, P), see

‎deps/v8/src/api/api.cc‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4752,6 +4752,16 @@ MaybeLocal<Value> v8::Object::GetPrivate(Local<Context> context,
47524752

47534753
Maybe<PropertyAttribute> v8::Object::GetPropertyAttributes(
47544754
Local<Context> context, Local<Value> key) {
4755+
PropertyAttribute attributes = PropertyAttribute::None;
4756+
auto result = GetPropertyAttributes(context, key, &attributes);
4757+
if (result.IsNothing()) return {};
4758+
// This will confusingly return None when the property doesn't exist.
4759+
return Just(attributes);
4760+
}
4761+
4762+
Maybe<bool> v8::Object::GetPropertyAttributes(
4763+
Local<Context> context, Local<Value> key,
4764+
PropertyAttribute* out_attributes) {
47554765
auto i_isolate = i::Isolate::Current();
47564766
EnterV8Scope<> api_scope{i_isolate, context,
47574767
RCCId::kAPI_Object_GetPropertyAttributes};
@@ -4764,9 +4774,10 @@ Maybe<PropertyAttribute> v8::Object::GetPropertyAttributes(
47644774
auto result = i::JSReceiver::GetPropertyAttributes(i_isolate, self, key_name);
47654775
if (result.IsNothing()) return {};
47664776
if (result.FromJust() == i::ABSENT) {
4767-
return Just(static_cast<PropertyAttribute>(i::NONE));
4777+
return Just(false);
47684778
}
4769-
return Just(static_cast<PropertyAttribute>(result.FromJust()));
4779+
*out_attributes = static_cast<PropertyAttribute>(result.FromJust());
4780+
return Just(true);
47704781
}
47714782

47724783
MaybeLocal<Value> v8::Object::GetOwnPropertyDescriptor(Local<Context> context,

‎deps/v8/src/ic/ic.cc‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1813,6 +1813,8 @@ bool StoreIC::LookupForWrite(LookupIterator* it, DirectHandle<Object> value,
18131813
case LookupIterator::NOT_FOUND:
18141814
// If we are in StoreGlobal then check if we should throw on
18151815
// non-existent properties.
1816+
// TODO(ishell): make store global ic kind reliable, currently it is
1817+
// not if the feedback vector is not available.
18161818
if (IsStoreGlobalIC() &&
18171819
(GetShouldThrow(it->isolate(), Nothing<ShouldThrow>()) ==
18181820
ShouldThrow::kThrowOnError)) {
@@ -2084,6 +2086,8 @@ MaybeDirectHandle<Object> StoreIC::Store(Handle<JSAny> object,
20842086
Nothing<ShouldThrow>(), store_origin));
20852087
}
20862088
} else {
2089+
// TODO(ishell): deduce should throw from IC kind once it's reliable,
2090+
// currently it is not if the feedback vector is not available.
20872091
MAYBE_RETURN_NULL(Object::SetProperty(&it, value, store_origin));
20882092
}
20892093
return value;
@@ -2166,6 +2170,20 @@ MaybeObjectHandle StoreIC::ComputeHandler(LookupIterator* lookup) {
21662170
isolate());
21672171

21682172
if (lookup->HolderIsReceiverOrHiddenPrototype()) {
2173+
// TODO(ishell): make store global ic kind reliable, currently it is
2174+
// not if the feedback vector is not available.
2175+
if (IsStoreGlobalIC() &&
2176+
(GetShouldThrow(isolate(), Nothing<ShouldThrow>()) ==
2177+
ShouldThrow::kThrowOnError)) {
2178+
DCHECK(IsJSGlobalObject(*lookup->GetHolder<Object>()));
2179+
// This is a contextual store to an interceptor object. Use slow
2180+
// handler because before calling the setter callback we need to
2181+
// check whether the property exists. See
2182+
// https://tc39.es/ecma262/#sec-object-environment-records-setmutablebinding-n-v-s
2183+
Handle<Smi> smi_handler = StoreHandler::StoreSlow(isolate());
2184+
return MaybeObjectHandle(smi_handler);
2185+
}
2186+
21692187
if (info->has_setter() && !IsAnyDefineOwn()) {
21702188
TRACE_HANDLER_STATS(isolate(), StoreIC_StoreInterceptorDH);
21712189
if (info->non_masking()) {

‎deps/v8/src/objects/objects.cc‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2382,6 +2382,28 @@ Maybe<bool> Object::SetPropertyInternal(LookupIterator* it,
23822382

23832383
case LookupIterator::INTERCEPTOR: {
23842384
if (it->HolderIsReceiverOrHiddenPrototype()) {
2385+
// In case we are executing contextual store to a global object with
2386+
// an interceptor in strict mode, we need to check that the property
2387+
// actually exists before calling the setter. See
2388+
// https://tc39.es/ecma262/#sec-object-environment-records-setmutablebinding-n-v-s
2389+
if (IsJSGlobalObject(*it->GetReceiver())) {
2390+
Isolate* isolate = it->isolate();
2391+
auto should_throw_value = GetShouldThrow(isolate, should_throw);
2392+
should_throw = Just(should_throw_value);
2393+
if (should_throw_value == kThrowOnError) {
2394+
Maybe<PropertyAttributes> maybe_attributes =
2395+
JSObject::GetPropertyAttributesWithInterceptor(it);
2396+
if (maybe_attributes.IsNothing()) return Nothing<bool>();
2397+
if ((maybe_attributes.FromJust() & READ_ONLY) != 0) {
2398+
return WriteToReadOnlyProperty(it, value, should_throw);
2399+
}
2400+
if (maybe_attributes.FromJust() == ABSENT) {
2401+
// Interceptor doesn't have the property, continue lookup.
2402+
continue;
2403+
}
2404+
}
2405+
}
2406+
23852407
InterceptorResult result;
23862408
if (!JSObject::SetPropertyWithInterceptor(it, should_throw, value)
23872409
.To(&result)) {

‎deps/v8/test/cctest/test-api-interceptors.cc‎

Lines changed: 168 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ v8::Intercepted GenericInterceptorGetter(
199199
Local<String> name = generic_name.As<String>();
200200
String::Utf8Value utf8(isolate, name);
201201
char* name_str = *utf8;
202-
if (*name_str == '_') return v8::Intercepted::kNo;
202+
if (*name_str != '$') return v8::Intercepted::kNo;
203203
str = String::Concat(isolate, v8_str("_str_"), name);
204204
}
205205

@@ -209,6 +209,35 @@ v8::Intercepted GenericInterceptorGetter(
209209
return v8::Intercepted::kYes;
210210
}
211211

212+
v8::Intercepted GenericInterceptorQuery(
213+
Local<Name> generic_name,
214+
const v8::PropertyCallbackInfo<v8::Integer>& info) {
215+
v8::Isolate* isolate = info.GetIsolate();
216+
Local<String> str;
217+
if (generic_name->IsSymbol()) {
218+
Local<Value> name = generic_name.As<Symbol>()->Description(isolate);
219+
if (name->IsUndefined()) return v8::Intercepted::kNo;
220+
str = String::Concat(isolate, v8_str("_sym_"), name.As<String>());
221+
} else {
222+
Local<String> name = generic_name.As<String>();
223+
String::Utf8Value utf8(isolate, name);
224+
char* name_str = *utf8;
225+
if (*name_str != '$') return v8::Intercepted::kNo;
226+
str = String::Concat(isolate, v8_str("_str_"), name);
227+
}
228+
229+
Local<Object> self = info.HolderV2();
230+
v8::PropertyAttribute attributes;
231+
bool exists;
232+
if (self->GetPropertyAttributes(isolate->GetCurrentContext(), str,
233+
&attributes)
234+
.To(&exists)) {
235+
if (!exists) return v8::Intercepted::kNo;
236+
info.GetReturnValue().Set(attributes);
237+
}
238+
return v8::Intercepted::kYes;
239+
}
240+
212241
v8::Intercepted GenericInterceptorSetter(
213242
Local<Name> generic_name, Local<Value> value,
214243
const v8::PropertyCallbackInfo<void>& info) {
@@ -222,7 +251,7 @@ v8::Intercepted GenericInterceptorSetter(
222251
Local<String> name = generic_name.As<String>();
223252
String::Utf8Value utf8(info.GetIsolate(), name);
224253
char* name_str = *utf8;
225-
if (*name_str == '_') return v8::Intercepted::kNo;
254+
if (*name_str != '$') return v8::Intercepted::kNo;
226255
str = String::Concat(info.GetIsolate(), v8_str("_str_"), name);
227256
}
228257

@@ -1632,9 +1661,9 @@ void InterceptorLoadICGlobalWithInterceptor(bool masking) {
16321661
ExpectInt32(
16331662
"(function() {"
16341663
" var f = function(obj) { "
1635-
" return obj.foo;"
1664+
" return obj.$foo;"
16361665
" };"
1637-
" this._str_foo = 42;"
1666+
" this._str_$foo = 42;"
16381667
" var obj = { __proto__: this };"
16391668
" for (var i = 0; i < 1500; i++) obj['p' + i] = 0;"
16401669
" /* Ensure that |obj| is in dictionary mode. */"
@@ -2061,6 +2090,138 @@ THREADED_TEST(EmptyInterceptorVsStoreGlobalICs) {
20612090
120);
20622091
}
20632092

2093+
namespace {
2094+
2095+
void CheckGlobalInterceptorIC(v8::NamedPropertyGetterCallback getter,
2096+
v8::NamedPropertySetterCallback setter,
2097+
v8::NamedPropertyQueryCallback query,
2098+
v8::PropertyHandlerFlags flags,
2099+
const char* source, std::optional<int> expected) {
2100+
v8::Isolate* isolate = CcTest::isolate();
2101+
v8::HandleScope scope(isolate);
2102+
v8::Local<v8::ObjectTemplate> templ_global = v8::ObjectTemplate::New(isolate);
2103+
v8::NamedPropertyHandlerConfiguration config(
2104+
getter, setter, query, nullptr /* deleter */, nullptr /* enumerator */,
2105+
nullptr /* definer */, nullptr /* descriptor */, {}, flags);
2106+
templ_global->SetHandler(config);
2107+
2108+
LocalContext context(nullptr, templ_global);
2109+
i::DirectHandle<i::JSReceiver> global_proxy =
2110+
v8::Utils::OpenDirectHandle<Object, i::JSReceiver>(context->Global());
2111+
CHECK(IsJSGlobalProxy(*global_proxy));
2112+
i::DirectHandle<i::JSGlobalObject> global(
2113+
i::Cast<i::JSGlobalObject>(global_proxy->map()->prototype()),
2114+
CcTest::i_isolate());
2115+
CHECK(global->map()->has_named_interceptor());
2116+
2117+
v8::Local<Value> value = CompileRun(source);
2118+
if (expected) {
2119+
CHECK_EQ(*expected, value->Int32Value(context.local()).FromJust());
2120+
} else {
2121+
CHECK(value.IsEmpty());
2122+
}
2123+
}
2124+
2125+
void StoreGlobalICWithGlobalInterceptor(bool masking) {
2126+
v8::PropertyHandlerFlags flags = v8::PropertyHandlerFlags::kNone;
2127+
if (!masking) {
2128+
flags = v8::PropertyHandlerFlags::kNonMasking;
2129+
}
2130+
2131+
// In sloppy mode storing to global must succeed.
2132+
CheckGlobalInterceptorIC( //
2133+
GenericInterceptorGetter, //
2134+
GenericInterceptorSetter, //
2135+
GenericInterceptorQuery, //
2136+
flags, //
2137+
R"(
2138+
// Make interceptor behave like it has a read-only property "$y".
2139+
Object.defineProperty(globalThis, '_str_$y',
2140+
{value: 153, writable: false});
2141+
2142+
let result = 0;
2143+
2144+
result += (typeof($x) === 'undefined' ? 0 : 10000);
2145+
result += ($y === 153 ? 0 : 10000);
2146+
2147+
for (var i = 0; i < 20; i++) {
2148+
try {
2149+
$x = i; // not intercepted, absent variable
2150+
result++;
2151+
} catch (e) {
2152+
}
2153+
}
2154+
for (var i = 0; i < 20; i++) {
2155+
try {
2156+
$y = i; // intercepted, read only property
2157+
result++;
2158+
} catch (e) {
2159+
}
2160+
}
2161+
2162+
// Check contextual stores succeeded.
2163+
result += ($x === 19 ? 0 : 100);
2164+
result += ($y === 153 ? 0 : 1000);
2165+
2166+
result
2167+
)",
2168+
40);
2169+
2170+
// In strict mode storing to global must throw.
2171+
CheckGlobalInterceptorIC( //
2172+
GenericInterceptorGetter, //
2173+
GenericInterceptorSetter, //
2174+
GenericInterceptorQuery, //
2175+
flags,
2176+
R"(
2177+
'use strict';
2178+
2179+
// Make interceptor behave like it has a read-only property "$y".
2180+
Object.defineProperty(globalThis, '_str_$y',
2181+
{value: 153, writable: false});
2182+
2183+
let result = 0;
2184+
2185+
result += (typeof($x) === 'undefined' ? 0 : 10000);
2186+
result += ($y === 153 ? 0 : 10000);
2187+
2188+
for (var i = 0; i < 20; i++) {
2189+
try {
2190+
$x = i; // not intercepted, absent variable
2191+
} catch (e) {
2192+
result++;
2193+
}
2194+
}
2195+
for (var i = 0; i < 20; i++) {
2196+
try {
2197+
$y = i; // intercepted, read only property
2198+
} catch (e) {
2199+
result++;
2200+
}
2201+
}
2202+
2203+
// Check contextual stores did not happen.
2204+
result += (typeof($x) === 'undefined' ? 0 : 100);
2205+
result += ($y === 153 ? 0 : 1000);
2206+
2207+
result
2208+
)",
2209+
40);
2210+
}
2211+
2212+
} // namespace
2213+
2214+
// Checks correctness of global objects with interceptor vs contextual stores.
2215+
THREADED_TEST(StoreGlobalICWithGlobalNonMaskingInterceptor) {
2216+
const bool masking = false;
2217+
StoreGlobalICWithGlobalInterceptor(masking);
2218+
}
2219+
2220+
THREADED_TEST(StoreGlobalICWithGlobalMaskingInterceptor) {
2221+
const bool masking = true;
2222+
StoreGlobalICWithGlobalInterceptor(masking);
2223+
}
2224+
20642225
THREADED_TEST(LegacyInterceptorDoesNotSeeSymbols) {
20652226
LocalContext env;
20662227
v8::Isolate* isolate = CcTest::isolate();
@@ -2113,9 +2274,9 @@ THREADED_TEST(GenericInterceptorDoesSeeSymbols) {
21132274
ExpectInt32("child._sym_age", 10);
21142275

21152276
// Check that it also sees strings.
2116-
CompileRun("child.foo = 47");
2117-
ExpectInt32("child.foo", 47);
2118-
ExpectInt32("child._str_foo", 47);
2277+
CompileRun("child.$foo = 47");
2278+
ExpectInt32("child.$foo", 47);
2279+
ExpectInt32("child._str_$foo", 47);
21192280

21202281
// Check that the interceptor can punt (in this case, on anonymous symbols).
21212282
CompileRun("child[anon] = 31337");

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL