Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: implement query callbacks for vm #22390

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,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.16',
'v8_embedder_string': '-node.18',

# Enable disassembler for `--print-code` v8 options
'v8_enable_disassembler': 1,
Expand Down
39 changes: 39 additions & 0 deletions deps/v8/include/v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -5878,6 +5878,26 @@ enum class PropertyHandlerFlags {
};

Copy link
Member

@jdalton jdalton Aug 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu As someone not super familiar with the internals of this (me) can you explain a bit about how this is fixing the linked to issues.

struct NamedPropertyHandlerConfiguration {
NamedPropertyHandlerConfiguration(
GenericNamedPropertyGetterCallback getter,
GenericNamedPropertySetterCallback setter,
GenericNamedPropertyQueryCallback query,
GenericNamedPropertyDeleterCallback deleter,
GenericNamedPropertyEnumeratorCallback enumerator,
GenericNamedPropertyDefinerCallback definer,
GenericNamedPropertyDescriptorCallback descriptor,
Local<Value> data = Local<Value>(),
PropertyHandlerFlags flags = PropertyHandlerFlags::kNone)
: getter(getter),
setter(setter),
query(query),
deleter(deleter),
enumerator(enumerator),
definer(definer),
descriptor(descriptor),
data(data),
flags(flags) {}

NamedPropertyHandlerConfiguration(
/** Note: getter is required */
GenericNamedPropertyGetterCallback getter = 0,
Expand Down Expand Up @@ -5929,6 +5949,25 @@ struct NamedPropertyHandlerConfiguration {


struct IndexedPropertyHandlerConfiguration {
IndexedPropertyHandlerConfiguration(
IndexedPropertyGetterCallback getter,
IndexedPropertySetterCallback setter, IndexedPropertyQueryCallback query,
IndexedPropertyDeleterCallback deleter,
IndexedPropertyEnumeratorCallback enumerator,
IndexedPropertyDefinerCallback definer,
IndexedPropertyDescriptorCallback descriptor,
Local<Value> data = Local<Value>(),
PropertyHandlerFlags flags = PropertyHandlerFlags::kNone)
: getter(getter),
setter(setter),
query(query),
deleter(deleter),
enumerator(enumerator),
definer(definer),
descriptor(descriptor),
data(data),
flags(flags) {}

IndexedPropertyHandlerConfiguration(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu I noticed in the linked to issues in the opening summary that V8 work was needed. A patch for V8 was merged but then reverted for perf reasons. Was the perf issue eventually resolved and the appropriate V8 changed landed or is this PR working around V8?

/** Note: getter is required */
IndexedPropertyGetterCallback getter = 0,
Expand Down
4 changes: 0 additions & 4 deletions deps/v8/src/api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1811,10 +1811,6 @@ static i::Handle<i::InterceptorInfo> CreateInterceptorInfo(
i::Isolate* isolate, Getter getter, Setter setter, Query query,
Descriptor descriptor, Deleter remover, Enumerator enumerator,
Definer definer, Local<Value> data, PropertyHandlerFlags flags) {
// Either intercept attributes or descriptor.
DCHECK(query == nullptr || descriptor == nullptr);
// Only use descriptor callback with definer callback.
DCHECK(query == nullptr || definer == nullptr);
auto obj = i::Handle<i::InterceptorInfo>::cast(
isolate->factory()->NewStruct(i::INTERCEPTOR_INFO_TYPE, i::TENURED));
obj->set_flags(0);
Expand Down
73 changes: 39 additions & 34 deletions deps/v8/src/objects.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7662,47 +7662,52 @@ namespace {

Maybe<bool> GetPropertyDescriptorWithInterceptor(LookupIterator* it,
PropertyDescriptor* desc) {
bool has_access = true;
if (it->state() == LookupIterator::ACCESS_CHECK) {
has_access = it->HasAccess() || JSObject::AllCanRead(it);
it->Next();
if (it->HasAccess()) {
it->Next();
} else if (!JSObject::AllCanRead(it) ||
it->state() != LookupIterator::INTERCEPTOR) {
it->Restart();
return Just(false);
}
}

if (has_access && it->state() == LookupIterator::INTERCEPTOR) {
Isolate* isolate = it->isolate();
Handle<InterceptorInfo> interceptor = it->GetInterceptor();
if (!interceptor->descriptor()->IsUndefined(isolate)) {
Handle<Object> result;
Handle<JSObject> holder = it->GetHolder<JSObject>();
if (it->state() != LookupIterator::INTERCEPTOR) return Just(false);

Handle<Object> receiver = it->GetReceiver();
if (!receiver->IsJSReceiver()) {
ASSIGN_RETURN_ON_EXCEPTION_VALUE(
isolate, receiver, Object::ConvertReceiver(isolate, receiver),
Nothing<bool>());
}
Isolate* isolate = it->isolate();
Handle<InterceptorInfo> interceptor = it->GetInterceptor();
if (interceptor->descriptor()->IsUndefined(isolate)) return Just(false);

PropertyCallbackArguments args(isolate, interceptor->data(), *receiver,
*holder, kDontThrow);
if (it->IsElement()) {
result = args.CallIndexedDescriptor(interceptor, it->index());
} else {
result = args.CallNamedDescriptor(interceptor, it->name());
}
if (!result.is_null()) {
// Request successfully intercepted, try to set the property
// descriptor.
Utils::ApiCheck(
PropertyDescriptor::ToPropertyDescriptor(isolate, result, desc),
it->IsElement() ? "v8::IndexedPropertyDescriptorCallback"
: "v8::NamedPropertyDescriptorCallback",
"Invalid property descriptor.");
Handle<Object> result;
Handle<JSObject> holder = it->GetHolder<JSObject>();

return Just(true);
}
}
Handle<Object> receiver = it->GetReceiver();
if (!receiver->IsJSReceiver()) {
ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, receiver,
Object::ConvertReceiver(isolate, receiver),
Nothing<bool>());
}
it->Restart();

PropertyCallbackArguments args(isolate, interceptor->data(), *receiver,
*holder, kDontThrow);
if (it->IsElement()) {
result = args.CallIndexedDescriptor(interceptor, it->index());
} else {
result = args.CallNamedDescriptor(interceptor, it->name());
}
if (!result.is_null()) {
// Request successfully intercepted, try to set the property
// descriptor.
Utils::ApiCheck(
PropertyDescriptor::ToPropertyDescriptor(isolate, result, desc),
it->IsElement() ? "v8::IndexedPropertyDescriptorCallback"
: "v8::NamedPropertyDescriptorCallback",
"Invalid property descriptor.");

return Just(true);
}

it->Next();
return Just(false);
}
} // namespace
Expand Down
12 changes: 6 additions & 6 deletions deps/v8/test/cctest/test-api-interceptors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4768,7 +4768,7 @@ TEST(NamedAllCanReadInterceptor) {
ExpectInt32("checked.whatever", 17);
CHECK(!CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')")
->IsUndefined());
CHECK_EQ(6, access_check_data.count);
CHECK_EQ(5, access_check_data.count);

access_check_data.result = false;
ExpectInt32("checked.whatever", intercept_data_0.value);
Expand All @@ -4777,7 +4777,7 @@ TEST(NamedAllCanReadInterceptor) {
CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')");
CHECK(try_catch.HasCaught());
}
CHECK_EQ(9, access_check_data.count);
CHECK_EQ(8, access_check_data.count);

intercept_data_1.should_intercept = true;
ExpectInt32("checked.whatever", intercept_data_1.value);
Expand All @@ -4786,7 +4786,7 @@ TEST(NamedAllCanReadInterceptor) {
CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')");
CHECK(try_catch.HasCaught());
}
CHECK_EQ(12, access_check_data.count);
CHECK_EQ(11, access_check_data.count);
g_access_check_data = nullptr;
}

Expand Down Expand Up @@ -4855,7 +4855,7 @@ TEST(IndexedAllCanReadInterceptor) {
ExpectInt32("checked[15]", 17);
CHECK(!CompileRun("Object.getOwnPropertyDescriptor(checked, '15')")
->IsUndefined());
CHECK_EQ(6, access_check_data.count);
CHECK_EQ(5, access_check_data.count);

access_check_data.result = false;
ExpectInt32("checked[15]", intercept_data_0.value);
Expand All @@ -4864,7 +4864,7 @@ TEST(IndexedAllCanReadInterceptor) {
CompileRun("Object.getOwnPropertyDescriptor(checked, '15')");
CHECK(try_catch.HasCaught());
}
CHECK_EQ(9, access_check_data.count);
CHECK_EQ(8, access_check_data.count);

intercept_data_1.should_intercept = true;
ExpectInt32("checked[15]", intercept_data_1.value);
Expand All @@ -4873,7 +4873,7 @@ TEST(IndexedAllCanReadInterceptor) {
CompileRun("Object.getOwnPropertyDescriptor(checked, '15')");
CHECK(try_catch.HasCaught());
}
CHECK_EQ(12, access_check_data.count);
CHECK_EQ(11, access_check_data.count);

g_access_check_data = nullptr;
}
Expand Down
176 changes: 176 additions & 0 deletions deps/v8/test/unittests/api/interceptor-unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,180 @@ TEST_F(InterceptorTest, FreezeApiObjectWithInterceptor) {
}

} // namespace

namespace internal {
namespace {

class InterceptorLoggingTest : public TestWithNativeContext {
public:
InterceptorLoggingTest() {}

static const int kTestIndex = 0;

static void NamedPropertyGetter(Local<v8::Name> name,
const v8::PropertyCallbackInfo<Value>& info) {
LogCallback(info, "named getter");
}

static void NamedPropertySetter(Local<v8::Name> name, Local<v8::Value> value,
const v8::PropertyCallbackInfo<Value>& info) {
LogCallback(info, "named setter");
}

static void NamedPropertyQuery(
Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Integer>& info) {
LogCallback(info, "named query");
}

static void NamedPropertyDeleter(
Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Boolean>& info) {
LogCallback(info, "named deleter");
}

static void NamedPropertyEnumerator(
const v8::PropertyCallbackInfo<Array>& info) {
LogCallback(info, "named enumerator");
}

static void NamedPropertyDefiner(
Local<v8::Name> name, const v8::PropertyDescriptor& desc,
const v8::PropertyCallbackInfo<Value>& info) {
LogCallback(info, "named definer");
}

static void NamedPropertyDescriptor(
Local<v8::Name> name, const v8::PropertyCallbackInfo<Value>& info) {
LogCallback(info, "named descriptor");
}

static void IndexedPropertyGetter(
uint32_t index, const v8::PropertyCallbackInfo<Value>& info) {
LogCallback(info, "indexed getter");
}

static void IndexedPropertySetter(
uint32_t index, Local<v8::Value> value,
const v8::PropertyCallbackInfo<Value>& info) {
LogCallback(info, "indexed setter");
}

static void IndexedPropertyQuery(
uint32_t index, const v8::PropertyCallbackInfo<v8::Integer>& info) {
LogCallback(info, "indexed query");
}

static void IndexedPropertyDeleter(
uint32_t index, const v8::PropertyCallbackInfo<v8::Boolean>& info) {
LogCallback(info, "indexed deleter");
}

static void IndexedPropertyEnumerator(
const v8::PropertyCallbackInfo<Array>& info) {
LogCallback(info, "indexed enumerator");
}

static void IndexedPropertyDefiner(
uint32_t index, const v8::PropertyDescriptor& desc,
const v8::PropertyCallbackInfo<Value>& info) {
LogCallback(info, "indexed definer");
}

static void IndexedPropertyDescriptor(
uint32_t index, const v8::PropertyCallbackInfo<Value>& info) {
LogCallback(info, "indexed descriptor");
}

template <class T>
static void LogCallback(const v8::PropertyCallbackInfo<T>& info,
const char* callback_name) {
InterceptorLoggingTest* test = reinterpret_cast<InterceptorLoggingTest*>(
info.This()->GetAlignedPointerFromInternalField(kTestIndex));
test->Log(callback_name);
}

void Log(const char* callback_name) {
if (log_is_empty_) {
log_is_empty_ = false;
} else {
log_ << ", ";
}
log_ << callback_name;
}

protected:
void SetUp() override {
// Set up the object that supports full interceptors.
v8::Local<v8::ObjectTemplate> templ = v8::ObjectTemplate::New(v8_isolate());
templ->SetInternalFieldCount(1);
templ->SetHandler(v8::NamedPropertyHandlerConfiguration(
NamedPropertyGetter, NamedPropertySetter, NamedPropertyQuery,
NamedPropertyDeleter, NamedPropertyEnumerator, NamedPropertyDefiner,
NamedPropertyDescriptor));
templ->SetHandler(v8::IndexedPropertyHandlerConfiguration(
IndexedPropertyGetter, IndexedPropertySetter, IndexedPropertyQuery,
IndexedPropertyDeleter, IndexedPropertyEnumerator,
IndexedPropertyDefiner, IndexedPropertyDescriptor));
v8::Local<v8::Object> instance =
templ->NewInstance(context()).ToLocalChecked();
instance->SetAlignedPointerInInternalField(kTestIndex, this);
SetGlobalProperty("obj", instance);
}

std::string Run(const char* script) {
log_is_empty_ = true;
log_.str(std::string());
log_.clear();

RunJS(script);
return log_.str();
}

private:
bool log_is_empty_ = false;
std::stringstream log_;
};

TEST_F(InterceptorLoggingTest, DispatchTest) {
EXPECT_EQ(Run("for (var p in obj) {}"),
"indexed enumerator, named enumerator");
EXPECT_EQ(Run("Object.keys(obj)"), "indexed enumerator, named enumerator");

EXPECT_EQ(Run("obj.foo"), "named getter");
EXPECT_EQ(Run("obj[42]"), "indexed getter");

EXPECT_EQ(Run("obj.foo = null"), "named setter");
EXPECT_EQ(Run("obj[42] = null"), "indexed setter");

EXPECT_EQ(Run("Object.getOwnPropertyDescriptor(obj, 'foo')"),
"named descriptor");

EXPECT_EQ(Run("Object.getOwnPropertyDescriptor(obj, 42)"),
"indexed descriptor");

EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {value: 42})"),
"named descriptor, named definer, named setter");
EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {get(){} })"),
"named descriptor, named definer");
EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {set(value){}})"),
"named descriptor, named definer");
EXPECT_EQ(Run("Object.defineProperty(obj, 'foo', {get(){}, set(value){}})"),
"named descriptor, named definer");

EXPECT_EQ(Run("Object.defineProperty(obj, 42, {value: 'foo'})"),
"indexed descriptor, "
// then attempt definer first and fallback to setter.
"indexed definer, indexed setter");

EXPECT_EQ(Run("Object.prototype.propertyIsEnumerable.call(obj, 'a')"),
"named query");
EXPECT_EQ(Run("Object.prototype.propertyIsEnumerable.call(obj, 42)"),
"indexed query");

EXPECT_EQ(Run("Object.prototype.hasOwnProperty.call(obj, 'a')"),
"named query");
// TODO(cbruni): Fix once hasOnwProperty is fixed (https://crbug.com/872628)
EXPECT_EQ(Run("Object.prototype.hasOwnProperty.call(obj, '42')"), "");
}
} // namespace
} // namespace internal
} // namespace v8
Loading