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

deps: V8: cherry-pick 80bbbb143c24 · nodejs/node@e23e345 · GitHub

/ node Public

Commit e23e345

Browse files
targos
committed
deps: V8: cherry-pick 80bbbb143c24
Original commit message: [class] handle existing readonly properties in StoreOwnIC Previously, StoreOwnIC incorrectly reuses the [[Set]] semantics when initializing public literal class fields and object literals in certain cases (e.g. when there's no feedback). This was less of an issue for object literals, but with public class fields it's possible to define property attributes while the instance is still being initialized, or to encounter existing static "name" or "length" properties that should be readonly. This patch fixes it by 1) Emitting code that calls into the slow stub when handling StoreOwnIC with existing read-only properties. 2) Adding extra steps in StoreIC::Store to handle such stores properly with [[DefineOwnProperty]] semantics. Bug: v8:12421, v8:9888 Change-Id: I6547320a1caba58c66ee1043cd3183a2de7cefef Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3300092 Reviewed-by: Shu-yu Guo <syg@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/main@{#78659} Refs: v8/v8@80bbbb1 PR-URL: #40907 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 696ce7d commit e23e345

12 files changed

Lines changed: 727 additions & 57 deletions

‎common.gypi‎

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

3737
# Reset this number to 0 on major V8 upgrades.
3838
# Increment by one for each non-official patch applied to deps/v8.
39-
'v8_embedder_string': '-node.11',
39+
'v8_embedder_string': '-node.12',
4040

4141
##### V8 defaults for Node.js #####
4242

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

Lines changed: 123 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "src/objects/js-array-inl.h"
3636
#include "src/objects/megadom-handler.h"
3737
#include "src/objects/module-inl.h"
38+
#include "src/objects/property-descriptor.h"
3839
#include "src/objects/prototype.h"
3940
#include "src/objects/struct-inl.h"
4041
#include "src/runtime/runtime-utils.h"
@@ -1726,6 +1727,69 @@ MaybeHandle<Object> StoreGlobalIC::Store(Handle<Name> name,
17261727
return StoreIC::Store(global, name, value);
17271728
}
17281729

1730+
namespace {
1731+
Maybe<bool> DefineOwnDataProperty(LookupIterator* it,
1732+
LookupIterator::State original_state,
1733+
Handle<Object> value,
1734+
Maybe<ShouldThrow> should_throw,
1735+
StoreOrigin store_origin) {
1736+
// It should not be possible to call DefineOwnDataProperty in a
1737+
// contextual store (indicated by IsJSGlobalObject()).
1738+
DCHECK(!it->GetReceiver()->IsJSGlobalObject(it->isolate()));
1739+
1740+
// Handle special cases that can't be handled by
1741+
// DefineOwnPropertyIgnoreAttributes first.
1742+
switch (it->state()) {
1743+
case LookupIterator::JSPROXY: {
1744+
PropertyDescriptor new_desc;
1745+
new_desc.set_value(value);
1746+
new_desc.set_writable(true);
1747+
new_desc.set_enumerable(true);
1748+
new_desc.set_configurable(true);
1749+
DCHECK_EQ(original_state, LookupIterator::JSPROXY);
1750+
// TODO(joyee): this will start the lookup again. Ideally we should
1751+
// implement something that reuses the existing LookupIterator.
1752+
return JSProxy::DefineOwnProperty(it->isolate(), it->GetHolder<JSProxy>(),
1753+
it->GetName(), &new_desc, should_throw);
1754+
}
1755+
// When lazy feedback is disabled, the original state could be different
1756+
// while the object is already prepared for TRANSITION.
1757+
case LookupIterator::TRANSITION: {
1758+
switch (original_state) {
1759+
case LookupIterator::JSPROXY:
1760+
case LookupIterator::TRANSITION:
1761+
case LookupIterator::DATA:
1762+
case LookupIterator::INTERCEPTOR:
1763+
case LookupIterator::ACCESSOR:
1764+
case LookupIterator::INTEGER_INDEXED_EXOTIC:
1765+
UNREACHABLE();
1766+
case LookupIterator::ACCESS_CHECK: {
1767+
DCHECK(!it->GetHolder<JSObject>()->IsAccessCheckNeeded());
1768+
V8_FALLTHROUGH;
1769+
}
1770+
case LookupIterator::NOT_FOUND:
1771+
return Object::AddDataProperty(it, value, NONE,
1772+
Nothing<ShouldThrow>(), store_origin);
1773+
}
1774+
}
1775+
case LookupIterator::ACCESS_CHECK:
1776+
case LookupIterator::NOT_FOUND:
1777+
case LookupIterator::DATA:
1778+
case LookupIterator::ACCESSOR:
1779+
case LookupIterator::INTERCEPTOR:
1780+
case LookupIterator::INTEGER_INDEXED_EXOTIC:
1781+
break;
1782+
}
1783+
1784+
// We need to restart to handle interceptors properly.
1785+
it->Restart();
1786+
1787+
return JSObject::DefineOwnPropertyIgnoreAttributes(
1788+
it, value, NONE, should_throw, JSObject::DONT_FORCE_FIELD,
1789+
JSObject::EnforceDefineSemantics::kDefine);
1790+
}
1791+
} // namespace
1792+
17291793
MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
17301794
Handle<Object> value,
17311795
StoreOrigin store_origin) {
@@ -1737,7 +1801,15 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
17371801
isolate(), object, key,
17381802
IsAnyStoreOwn() ? LookupIterator::OWN : LookupIterator::DEFAULT);
17391803
DCHECK_IMPLIES(IsStoreOwnIC(), it.IsFound() && it.HolderIsReceiver());
1740-
MAYBE_RETURN_NULL(Object::SetProperty(&it, value, StoreOrigin::kNamed));
1804+
// TODO(joyee): IsStoreOwnIC() is used in [[DefineOwnProperty]]
1805+
// operations during initialization of object literals and class
1806+
// fields. Rename them or separate them out.
1807+
if (IsStoreOwnIC()) {
1808+
MAYBE_RETURN_NULL(
1809+
JSReceiver::CreateDataProperty(&it, value, Nothing<ShouldThrow>()));
1810+
} else {
1811+
MAYBE_RETURN_NULL(Object::SetProperty(&it, value, StoreOrigin::kNamed));
1812+
}
17411813
return value;
17421814
}
17431815

@@ -1785,6 +1857,23 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
17851857
use_ic = false;
17861858
}
17871859
}
1860+
1861+
// For IsStoreOwnIC(), we can't simply do CreateDataProperty below
1862+
// because we need to check the attributes before UpdateCaches updates
1863+
// the state of the LookupIterator.
1864+
LookupIterator::State original_state = it.state();
1865+
// We'll defer the check for JSProxy and objects with named interceptors,
1866+
// because the defineProperty traps need to be called first if they are
1867+
// present.
1868+
if (IsStoreOwnIC() && !object->IsJSProxy() &&
1869+
!Handle<JSObject>::cast(object)->HasNamedInterceptor()) {
1870+
Maybe<bool> can_define = JSReceiver::CheckIfCanDefine(
1871+
isolate(), &it, value, Nothing<ShouldThrow>());
1872+
if (can_define.IsNothing() || !can_define.FromJust()) {
1873+
return MaybeHandle<Object>();
1874+
}
1875+
}
1876+
17881877
if (use_ic) {
17891878
UpdateCaches(&it, value, store_origin);
17901879
} else if (state() == NO_FEEDBACK) {
@@ -1793,7 +1882,21 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
17931882
: TraceIC("StoreIC", name);
17941883
}
17951884

1796-
MAYBE_RETURN_NULL(Object::SetProperty(&it, value, store_origin));
1885+
// TODO(joyee): IsStoreOwnIC() is true in [[DefineOwnProperty]]
1886+
// operations during initialization of object literals and class
1887+
// fields. In both paths, Rename the operations properly to avoid
1888+
// confusion.
1889+
// ES #sec-definefield
1890+
// ES #sec-runtime-semantics-propertydefinitionevaluation
1891+
if (IsStoreOwnIC()) {
1892+
// Private property should be defined via DefineOwnIC (as private names) or
1893+
// stored via other store ICs through private symbols.
1894+
DCHECK(!name->IsPrivate());
1895+
MAYBE_RETURN_NULL(DefineOwnDataProperty(
1896+
&it, original_state, value, Nothing<ShouldThrow>(), store_origin));
1897+
} else {
1898+
MAYBE_RETURN_NULL(Object::SetProperty(&it, value, store_origin));
1899+
}
17971900
return value;
17981901
}
17991902

@@ -1871,11 +1974,14 @@ MaybeObjectHandle StoreIC::ComputeHandler(LookupIterator* lookup) {
18711974

18721975
// If the interceptor is on the receiver...
18731976
if (lookup->HolderIsReceiverOrHiddenPrototype() && !info.non_masking()) {
1874-
// ...return a store interceptor Smi handler if there is one...
1875-
if (!info.setter().IsUndefined(isolate())) {
1977+
// ...return a store interceptor Smi handler if there is a setter
1978+
// interceptor and it's not StoreOwnIC (which should call the
1979+
// definer)...
1980+
if (!info.setter().IsUndefined(isolate()) && !IsStoreOwnIC()) {
18761981
return MaybeObjectHandle(StoreHandler::StoreInterceptor(isolate()));
18771982
}
1878-
// ...otherwise return a slow-case Smi handler.
1983+
// ...otherwise return a slow-case Smi handler, which invokes the
1984+
// definer for StoreOwnIC.
18791985
return MaybeObjectHandle(StoreHandler::StoreSlow(isolate()));
18801986
}
18811987

@@ -2055,6 +2161,14 @@ MaybeObjectHandle StoreIC::ComputeHandler(LookupIterator* lookup) {
20552161
Handle<JSReceiver> receiver =
20562162
Handle<JSReceiver>::cast(lookup->GetReceiver());
20572163
Handle<JSProxy> holder = lookup->GetHolder<JSProxy>();
2164+
2165+
// IsStoreOwnIC() is true when we are defining public fields on a Proxy.
2166+
// In that case use the slow stub to invoke the define trap.
2167+
if (IsStoreOwnIC()) {
2168+
TRACE_HANDLER_STATS(isolate(), StoreIC_SlowStub);
2169+
return MaybeObjectHandle(StoreHandler::StoreSlow(isolate()));
2170+
}
2171+
20582172
return MaybeObjectHandle(StoreHandler::StoreProxy(
20592173
isolate(), lookup_start_object_map(), holder, receiver));
20602174
}
@@ -2766,9 +2880,10 @@ RUNTIME_FUNCTION(Runtime_StoreOwnIC_Slow) {
27662880

27672881
PropertyKey lookup_key(isolate, key);
27682882
LookupIterator it(isolate, object, lookup_key, LookupIterator::OWN);
2769-
MAYBE_RETURN(JSObject::DefineOwnPropertyIgnoreAttributes(
2770-
&it, value, NONE, Nothing<ShouldThrow>()),
2771-
ReadOnlyRoots(isolate).exception());
2883+
2884+
MAYBE_RETURN(
2885+
JSReceiver::CreateDataProperty(&it, value, Nothing<ShouldThrow>()),
2886+
ReadOnlyRoots(isolate).exception());
27722887
return *value;
27732888
}
27742889

‎deps/v8/src/ic/keyed-store-generic.cc‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ class KeyedStoreGenericAssembler : public AccessorAssembler {
151151

152152
bool ShouldCheckPrototype() const { return IsKeyedStore(); }
153153
bool ShouldReconfigureExisting() const { return IsStoreInLiteral(); }
154-
bool ShouldCallSetter() const { return IsKeyedStore() || IsKeyedStoreOwn(); }
154+
bool ShouldCallSetter() const { return IsKeyedStore(); }
155155
bool ShouldCheckPrototypeValidity() const {
156156
// We don't do this for "in-literal" stores, because it is impossible for
157157
// the target object to be a "prototype".
@@ -1008,20 +1008,28 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
10081008
if (!ShouldReconfigureExisting()) {
10091009
BIND(&readonly);
10101010
{
1011-
LanguageMode language_mode;
1012-
if (maybe_language_mode.To(&language_mode)) {
1013-
if (language_mode == LanguageMode::kStrict) {
1014-
TNode<String> type = Typeof(p->receiver());
1015-
ThrowTypeError(p->context(), MessageTemplate::kStrictReadOnlyProperty,
1016-
name, type, p->receiver());
1011+
// FIXME(joyee): IsKeyedStoreOwn is actually true from
1012+
// StaNamedOwnProperty, which implements [[DefineOwnProperty]]
1013+
// semantics. Rename them.
1014+
if (IsKeyedDefineOwn() || IsKeyedStoreOwn()) {
1015+
Goto(slow);
1016+
} else {
1017+
LanguageMode language_mode;
1018+
if (maybe_language_mode.To(&language_mode)) {
1019+
if (language_mode == LanguageMode::kStrict) {
1020+
TNode<String> type = Typeof(p->receiver());
1021+
ThrowTypeError(p->context(),
1022+
MessageTemplate::kStrictReadOnlyProperty, name, type,
1023+
p->receiver());
1024+
} else {
1025+
exit_point->Return(p->value());
1026+
}
10171027
} else {
1028+
CallRuntime(Runtime::kThrowTypeErrorIfStrict, p->context(),
1029+
SmiConstant(MessageTemplate::kStrictReadOnlyProperty),
1030+
name, Typeof(p->receiver()), p->receiver());
10181031
exit_point->Return(p->value());
10191032
}
1020-
} else {
1021-
CallRuntime(Runtime::kThrowTypeErrorIfStrict, p->context(),
1022-
SmiConstant(MessageTemplate::kStrictReadOnlyProperty), name,
1023-
Typeof(p->receiver()), p->receiver());
1024-
exit_point->Return(p->value());
10251033
}
10261034
}
10271035
}

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

Lines changed: 59 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,27 @@ Maybe<bool> JSReceiver::HasInPrototypeChain(Isolate* isolate,
184184
}
185185
}
186186

187+
// static
188+
Maybe<bool> JSReceiver::CheckIfCanDefine(Isolate* isolate, LookupIterator* it,
189+
Handle<Object> value,
190+
Maybe<ShouldThrow> should_throw) {
191+
if (it->IsFound()) {
192+
Maybe<PropertyAttributes> attributes = GetPropertyAttributes(it);
193+
MAYBE_RETURN(attributes, Nothing<bool>());
194+
if ((attributes.FromJust() & DONT_DELETE) != 0) {
195+
RETURN_FAILURE(
196+
isolate, GetShouldThrow(isolate, should_throw),
197+
NewTypeError(MessageTemplate::kRedefineDisallowed, it->GetName()));
198+
}
199+
} else if (!JSObject::IsExtensible(
200+
Handle<JSObject>::cast(it->GetReceiver()))) {
201+
RETURN_FAILURE(
202+
isolate, GetShouldThrow(isolate, should_throw),
203+
NewTypeError(MessageTemplate::kDefineDisallowed, it->GetName()));
204+
}
205+
return Just(true);
206+
}
207+
187208
namespace {
188209

189210
bool HasExcludedProperty(
@@ -3365,23 +3386,25 @@ void JSObject::AddProperty(Isolate* isolate, Handle<JSObject> object,
33653386
// hidden prototypes.
33663387
MaybeHandle<Object> JSObject::DefineOwnPropertyIgnoreAttributes(
33673388
LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
3368-
AccessorInfoHandling handling) {
3389+
AccessorInfoHandling handling, EnforceDefineSemantics semantics) {
33693390
MAYBE_RETURN_NULL(DefineOwnPropertyIgnoreAttributes(
3370-
it, value, attributes, Just(ShouldThrow::kThrowOnError), handling));
3391+
it, value, attributes, Just(ShouldThrow::kThrowOnError), handling,
3392+
semantics));
33713393
return value;
33723394
}
33733395

33743396
Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
33753397
LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
3376-
Maybe<ShouldThrow> should_throw, AccessorInfoHandling handling) {
3398+
Maybe<ShouldThrow> should_throw, AccessorInfoHandling handling,
3399+
EnforceDefineSemantics semantics) {
33773400
it->UpdateProtector();
33783401
Handle<JSObject> object = Handle<JSObject>::cast(it->GetReceiver());
33793402

33803403
for (; it->IsFound(); it->Next()) {
33813404
switch (it->state()) {
33823405
case LookupIterator::JSPROXY:
3383-
case LookupIterator::NOT_FOUND:
33843406
case LookupIterator::TRANSITION:
3407+
case LookupIterator::NOT_FOUND:
33853408
UNREACHABLE();
33863409

33873410
case LookupIterator::ACCESS_CHECK:
@@ -3400,13 +3423,36 @@ Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
34003423
// TODO(verwaest): JSProxy afterwards verify the attributes that the
34013424
// JSProxy claims it has, and verifies that they are compatible. If not,
34023425
// they throw. Here we should do the same.
3403-
case LookupIterator::INTERCEPTOR:
3404-
if (handling == DONT_FORCE_FIELD) {
3405-
Maybe<bool> result =
3406-
JSObject::SetPropertyWithInterceptor(it, should_throw, value);
3407-
if (result.IsNothing() || result.FromJust()) return result;
3426+
case LookupIterator::INTERCEPTOR: {
3427+
Maybe<bool> result = Just(false);
3428+
if (semantics == EnforceDefineSemantics::kDefine) {
3429+
PropertyDescriptor descriptor;
3430+
descriptor.set_configurable((attributes & DONT_DELETE) != 0);
3431+
descriptor.set_enumerable((attributes & DONT_ENUM) != 0);
3432+
descriptor.set_writable((attributes & READ_ONLY) != 0);
3433+
descriptor.set_value(value);
3434+
result = DefinePropertyWithInterceptorInternal(
3435+
it, it->GetInterceptor(), should_throw, &descriptor);
3436+
} else {
3437+
DCHECK_EQ(semantics, EnforceDefineSemantics::kSet);
3438+
if (handling == DONT_FORCE_FIELD) {
3439+
result =
3440+
JSObject::SetPropertyWithInterceptor(it, should_throw, value);
3441+
}
3442+
}
3443+
if (result.IsNothing() || result.FromJust()) return result;
3444+
3445+
if (semantics == EnforceDefineSemantics::kDefine) {
3446+
it->Restart();
3447+
Maybe<bool> can_define = JSReceiver::CheckIfCanDefine(
3448+
it->isolate(), it, value, should_throw);
3449+
if (can_define.IsNothing() || !can_define.FromJust()) {
3450+
return can_define;
3451+
}
3452+
it->Restart();
34083453
}
34093454
break;
3455+
}
34103456

34113457
case LookupIterator::ACCESSOR: {
34123458
Handle<Object> accessors = it->GetAccessors();
@@ -3824,20 +3870,10 @@ Maybe<bool> JSObject::CreateDataProperty(LookupIterator* it,
38243870
Handle<JSReceiver> receiver = Handle<JSReceiver>::cast(it->GetReceiver());
38253871
Isolate* isolate = receiver->GetIsolate();
38263872

3827-
if (it->IsFound()) {
3828-
Maybe<PropertyAttributes> attributes = GetPropertyAttributes(it);
3829-
MAYBE_RETURN(attributes, Nothing<bool>());
3830-
if ((attributes.FromJust() & DONT_DELETE) != 0) {
3831-
RETURN_FAILURE(
3832-
isolate, GetShouldThrow(isolate, should_throw),
3833-
NewTypeError(MessageTemplate::kRedefineDisallowed, it->GetName()));
3834-
}
3835-
} else {
3836-
if (!JSObject::IsExtensible(Handle<JSObject>::cast(it->GetReceiver()))) {
3837-
RETURN_FAILURE(
3838-
isolate, GetShouldThrow(isolate, should_throw),
3839-
NewTypeError(MessageTemplate::kDefineDisallowed, it->GetName()));
3840-
}
3873+
Maybe<bool> can_define =
3874+
JSReceiver::CheckIfCanDefine(isolate, it, value, should_throw);
3875+
if (can_define.IsNothing() || !can_define.FromJust()) {
3876+
return can_define;
38413877
}
38423878

38433879
RETURN_ON_EXCEPTION_VALUE(it->isolate(),

0 commit comments

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