Skip to content

Commit

Permalink
deps: V8: cherry-pick 80bbbb143c24
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@targos
targos committed Jan 20, 2022
1 parent 696ce7d commit e23e345
Show file tree
Hide file tree
Showing 12 changed files with 727 additions and 57 deletions.
2 changes: 1 addition & 1 deletion common.gypi
View file
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.11',
'v8_embedder_string': '-node.12',

##### V8 defaults for Node.js #####

Expand Down
131 changes: 123 additions & 8 deletions deps/v8/src/ic/ic.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "src/objects/js-array-inl.h"
#include "src/objects/megadom-handler.h"
#include "src/objects/module-inl.h"
#include "src/objects/property-descriptor.h"
#include "src/objects/prototype.h"
#include "src/objects/struct-inl.h"
#include "src/runtime/runtime-utils.h"
Expand Down Expand Up @@ -1726,6 +1727,69 @@ MaybeHandle<Object> StoreGlobalIC::Store(Handle<Name> name,
return StoreIC::Store(global, name, value);
}

namespace {
Maybe<bool> DefineOwnDataProperty(LookupIterator* it,
LookupIterator::State original_state,
Handle<Object> value,
Maybe<ShouldThrow> should_throw,
StoreOrigin store_origin) {
// It should not be possible to call DefineOwnDataProperty in a
// contextual store (indicated by IsJSGlobalObject()).
DCHECK(!it->GetReceiver()->IsJSGlobalObject(it->isolate()));

// Handle special cases that can't be handled by
// DefineOwnPropertyIgnoreAttributes first.
switch (it->state()) {
case LookupIterator::JSPROXY: {
PropertyDescriptor new_desc;
new_desc.set_value(value);
new_desc.set_writable(true);
new_desc.set_enumerable(true);
new_desc.set_configurable(true);
DCHECK_EQ(original_state, LookupIterator::JSPROXY);
// TODO(joyee): this will start the lookup again. Ideally we should
// implement something that reuses the existing LookupIterator.
return JSProxy::DefineOwnProperty(it->isolate(), it->GetHolder<JSProxy>(),
it->GetName(), &new_desc, should_throw);
}
// When lazy feedback is disabled, the original state could be different
// while the object is already prepared for TRANSITION.
case LookupIterator::TRANSITION: {
switch (original_state) {
case LookupIterator::JSPROXY:
case LookupIterator::TRANSITION:
case LookupIterator::DATA:
case LookupIterator::INTERCEPTOR:
case LookupIterator::ACCESSOR:
case LookupIterator::INTEGER_INDEXED_EXOTIC:
UNREACHABLE();
case LookupIterator::ACCESS_CHECK: {
DCHECK(!it->GetHolder<JSObject>()->IsAccessCheckNeeded());
V8_FALLTHROUGH;
}
case LookupIterator::NOT_FOUND:
return Object::AddDataProperty(it, value, NONE,
Nothing<ShouldThrow>(), store_origin);
}
}
case LookupIterator::ACCESS_CHECK:
case LookupIterator::NOT_FOUND:
case LookupIterator::DATA:
case LookupIterator::ACCESSOR:
case LookupIterator::INTERCEPTOR:
case LookupIterator::INTEGER_INDEXED_EXOTIC:
break;
}

// We need to restart to handle interceptors properly.
it->Restart();

return JSObject::DefineOwnPropertyIgnoreAttributes(
it, value, NONE, should_throw, JSObject::DONT_FORCE_FIELD,
JSObject::EnforceDefineSemantics::kDefine);
}
} // namespace

MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
Handle<Object> value,
StoreOrigin store_origin) {
Expand All @@ -1737,7 +1801,15 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
isolate(), object, key,
IsAnyStoreOwn() ? LookupIterator::OWN : LookupIterator::DEFAULT);
DCHECK_IMPLIES(IsStoreOwnIC(), it.IsFound() && it.HolderIsReceiver());
MAYBE_RETURN_NULL(Object::SetProperty(&it, value, StoreOrigin::kNamed));
// TODO(joyee): IsStoreOwnIC() is used in [[DefineOwnProperty]]
// operations during initialization of object literals and class
// fields. Rename them or separate them out.
if (IsStoreOwnIC()) {
MAYBE_RETURN_NULL(
JSReceiver::CreateDataProperty(&it, value, Nothing<ShouldThrow>()));
} else {
MAYBE_RETURN_NULL(Object::SetProperty(&it, value, StoreOrigin::kNamed));
}
return value;
}

Expand Down Expand Up @@ -1785,6 +1857,23 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
use_ic = false;
}
}

// For IsStoreOwnIC(), we can't simply do CreateDataProperty below
// because we need to check the attributes before UpdateCaches updates
// the state of the LookupIterator.
LookupIterator::State original_state = it.state();
// We'll defer the check for JSProxy and objects with named interceptors,
// because the defineProperty traps need to be called first if they are
// present.
if (IsStoreOwnIC() && !object->IsJSProxy() &&
!Handle<JSObject>::cast(object)->HasNamedInterceptor()) {
Maybe<bool> can_define = JSReceiver::CheckIfCanDefine(
isolate(), &it, value, Nothing<ShouldThrow>());
if (can_define.IsNothing() || !can_define.FromJust()) {
return MaybeHandle<Object>();
}
}

if (use_ic) {
UpdateCaches(&it, value, store_origin);
} else if (state() == NO_FEEDBACK) {
Expand All @@ -1793,7 +1882,21 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
: TraceIC("StoreIC", name);
}

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

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

// If the interceptor is on the receiver...
if (lookup->HolderIsReceiverOrHiddenPrototype() && !info.non_masking()) {
// ...return a store interceptor Smi handler if there is one...
if (!info.setter().IsUndefined(isolate())) {
// ...return a store interceptor Smi handler if there is a setter
// interceptor and it's not StoreOwnIC (which should call the
// definer)...
if (!info.setter().IsUndefined(isolate()) && !IsStoreOwnIC()) {
return MaybeObjectHandle(StoreHandler::StoreInterceptor(isolate()));
}
// ...otherwise return a slow-case Smi handler.
// ...otherwise return a slow-case Smi handler, which invokes the
// definer for StoreOwnIC.
return MaybeObjectHandle(StoreHandler::StoreSlow(isolate()));
}

Expand Down Expand Up @@ -2055,6 +2161,14 @@ MaybeObjectHandle StoreIC::ComputeHandler(LookupIterator* lookup) {
Handle<JSReceiver> receiver =
Handle<JSReceiver>::cast(lookup->GetReceiver());
Handle<JSProxy> holder = lookup->GetHolder<JSProxy>();

// IsStoreOwnIC() is true when we are defining public fields on a Proxy.
// In that case use the slow stub to invoke the define trap.
if (IsStoreOwnIC()) {
TRACE_HANDLER_STATS(isolate(), StoreIC_SlowStub);
return MaybeObjectHandle(StoreHandler::StoreSlow(isolate()));
}

return MaybeObjectHandle(StoreHandler::StoreProxy(
isolate(), lookup_start_object_map(), holder, receiver));
}
Expand Down Expand Up @@ -2766,9 +2880,10 @@ RUNTIME_FUNCTION(Runtime_StoreOwnIC_Slow) {

PropertyKey lookup_key(isolate, key);
LookupIterator it(isolate, object, lookup_key, LookupIterator::OWN);
MAYBE_RETURN(JSObject::DefineOwnPropertyIgnoreAttributes(
&it, value, NONE, Nothing<ShouldThrow>()),
ReadOnlyRoots(isolate).exception());

MAYBE_RETURN(
JSReceiver::CreateDataProperty(&it, value, Nothing<ShouldThrow>()),
ReadOnlyRoots(isolate).exception());
return *value;
}

Expand Down
32 changes: 20 additions & 12 deletions deps/v8/src/ic/keyed-store-generic.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ class KeyedStoreGenericAssembler : public AccessorAssembler {

bool ShouldCheckPrototype() const { return IsKeyedStore(); }
bool ShouldReconfigureExisting() const { return IsStoreInLiteral(); }
bool ShouldCallSetter() const { return IsKeyedStore() || IsKeyedStoreOwn(); }
bool ShouldCallSetter() const { return IsKeyedStore(); }
bool ShouldCheckPrototypeValidity() const {
// We don't do this for "in-literal" stores, because it is impossible for
// the target object to be a "prototype".
Expand Down Expand Up @@ -1008,20 +1008,28 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore(
if (!ShouldReconfigureExisting()) {
BIND(&readonly);
{
LanguageMode language_mode;
if (maybe_language_mode.To(&language_mode)) {
if (language_mode == LanguageMode::kStrict) {
TNode<String> type = Typeof(p->receiver());
ThrowTypeError(p->context(), MessageTemplate::kStrictReadOnlyProperty,
name, type, p->receiver());
// FIXME(joyee): IsKeyedStoreOwn is actually true from
// StaNamedOwnProperty, which implements [[DefineOwnProperty]]
// semantics. Rename them.
if (IsKeyedDefineOwn() || IsKeyedStoreOwn()) {
Goto(slow);
} else {
LanguageMode language_mode;
if (maybe_language_mode.To(&language_mode)) {
if (language_mode == LanguageMode::kStrict) {
TNode<String> type = Typeof(p->receiver());
ThrowTypeError(p->context(),
MessageTemplate::kStrictReadOnlyProperty, name, type,
p->receiver());
} else {
exit_point->Return(p->value());
}
} else {
CallRuntime(Runtime::kThrowTypeErrorIfStrict, p->context(),
SmiConstant(MessageTemplate::kStrictReadOnlyProperty),
name, Typeof(p->receiver()), p->receiver());
exit_point->Return(p->value());
}
} else {
CallRuntime(Runtime::kThrowTypeErrorIfStrict, p->context(),
SmiConstant(MessageTemplate::kStrictReadOnlyProperty), name,
Typeof(p->receiver()), p->receiver());
exit_point->Return(p->value());
}
}
}
Expand Down
82 changes: 59 additions & 23 deletions deps/v8/src/objects/js-objects.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,27 @@ Maybe<bool> JSReceiver::HasInPrototypeChain(Isolate* isolate,
}
}

// static
Maybe<bool> JSReceiver::CheckIfCanDefine(Isolate* isolate, LookupIterator* it,
Handle<Object> value,
Maybe<ShouldThrow> should_throw) {
if (it->IsFound()) {
Maybe<PropertyAttributes> attributes = GetPropertyAttributes(it);
MAYBE_RETURN(attributes, Nothing<bool>());
if ((attributes.FromJust() & DONT_DELETE) != 0) {
RETURN_FAILURE(
isolate, GetShouldThrow(isolate, should_throw),
NewTypeError(MessageTemplate::kRedefineDisallowed, it->GetName()));
}
} else if (!JSObject::IsExtensible(
Handle<JSObject>::cast(it->GetReceiver()))) {
RETURN_FAILURE(
isolate, GetShouldThrow(isolate, should_throw),
NewTypeError(MessageTemplate::kDefineDisallowed, it->GetName()));
}
return Just(true);
}

namespace {

bool HasExcludedProperty(
Expand Down Expand Up @@ -3365,23 +3386,25 @@ void JSObject::AddProperty(Isolate* isolate, Handle<JSObject> object,
// hidden prototypes.
MaybeHandle<Object> JSObject::DefineOwnPropertyIgnoreAttributes(
LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
AccessorInfoHandling handling) {
AccessorInfoHandling handling, EnforceDefineSemantics semantics) {
MAYBE_RETURN_NULL(DefineOwnPropertyIgnoreAttributes(
it, value, attributes, Just(ShouldThrow::kThrowOnError), handling));
it, value, attributes, Just(ShouldThrow::kThrowOnError), handling,
semantics));
return value;
}

Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
Maybe<ShouldThrow> should_throw, AccessorInfoHandling handling) {
Maybe<ShouldThrow> should_throw, AccessorInfoHandling handling,
EnforceDefineSemantics semantics) {
it->UpdateProtector();
Handle<JSObject> object = Handle<JSObject>::cast(it->GetReceiver());

for (; it->IsFound(); it->Next()) {
switch (it->state()) {
case LookupIterator::JSPROXY:
case LookupIterator::NOT_FOUND:
case LookupIterator::TRANSITION:
case LookupIterator::NOT_FOUND:
UNREACHABLE();

case LookupIterator::ACCESS_CHECK:
Expand All @@ -3400,13 +3423,36 @@ Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
// TODO(verwaest): JSProxy afterwards verify the attributes that the
// JSProxy claims it has, and verifies that they are compatible. If not,
// they throw. Here we should do the same.
case LookupIterator::INTERCEPTOR:
if (handling == DONT_FORCE_FIELD) {
Maybe<bool> result =
JSObject::SetPropertyWithInterceptor(it, should_throw, value);
if (result.IsNothing() || result.FromJust()) return result;
case LookupIterator::INTERCEPTOR: {
Maybe<bool> result = Just(false);
if (semantics == EnforceDefineSemantics::kDefine) {
PropertyDescriptor descriptor;
descriptor.set_configurable((attributes & DONT_DELETE) != 0);
descriptor.set_enumerable((attributes & DONT_ENUM) != 0);
descriptor.set_writable((attributes & READ_ONLY) != 0);
descriptor.set_value(value);
result = DefinePropertyWithInterceptorInternal(
it, it->GetInterceptor(), should_throw, &descriptor);
} else {
DCHECK_EQ(semantics, EnforceDefineSemantics::kSet);
if (handling == DONT_FORCE_FIELD) {
result =
JSObject::SetPropertyWithInterceptor(it, should_throw, value);
}
}
if (result.IsNothing() || result.FromJust()) return result;

if (semantics == EnforceDefineSemantics::kDefine) {
it->Restart();
Maybe<bool> can_define = JSReceiver::CheckIfCanDefine(
it->isolate(), it, value, should_throw);
if (can_define.IsNothing() || !can_define.FromJust()) {
return can_define;
}
it->Restart();
}
break;
}

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

if (it->IsFound()) {
Maybe<PropertyAttributes> attributes = GetPropertyAttributes(it);
MAYBE_RETURN(attributes, Nothing<bool>());
if ((attributes.FromJust() & DONT_DELETE) != 0) {
RETURN_FAILURE(
isolate, GetShouldThrow(isolate, should_throw),
NewTypeError(MessageTemplate::kRedefineDisallowed, it->GetName()));
}
} else {
if (!JSObject::IsExtensible(Handle<JSObject>::cast(it->GetReceiver()))) {
RETURN_FAILURE(
isolate, GetShouldThrow(isolate, should_throw),
NewTypeError(MessageTemplate::kDefineDisallowed, it->GetName()));
}
Maybe<bool> can_define =
JSReceiver::CheckIfCanDefine(isolate, it, value, should_throw);
if (can_define.IsNothing() || !can_define.FromJust()) {
return can_define;
}

RETURN_ON_EXCEPTION_VALUE(it->isolate(),
Expand Down
Loading

0 comments on commit e23e345

Please sign in to comment.