Optionally invoke an interceptor on failed access checks

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index a5da88a0b3b2e0b5e44b17e0918d17655ea38e09..91180e238c46a4c74b0d21c10f4df6dbcc387f0e 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -1357,20 +1357,49 @@ bool JSObject::AllCanRead(LookupIterator* it) {
   return false;
 }
 
+Handle<InterceptorInfo> JSObject::GetInterceptorForFailedAccessCheck(

[Toon Verwaest, 2016/06/22 11:30:16]

Seems like this belongs on the LookupIterator, but I don't feel strongly about
it.

+    LookupIterator* it) {
+  Isolate* isolate = it->isolate();
+  Handle<JSObject> checked = it->GetHolder<JSObject>();
+  DisallowHeapAllocation no_gc;
+  AccessCheckInfo* access_check_info = AccessCheckInfo::Get(isolate, checked);
+  if (access_check_info) {
+    if (it->IsElement()) {
+      return handle(
+          InterceptorInfo::cast(access_check_info->indexed_interceptor()),
+          isolate);
+    } else {
+      return handle(
+          InterceptorInfo::cast(access_check_info->named_interceptor()),
+          isolate);
+    }
+  }
+  return Handle<InterceptorInfo>();
+}
 
 MaybeHandle<Object> JSObject::GetPropertyWithFailedAccessCheck(
     LookupIterator* it) {
+  Isolate* isolate = it->isolate();
   Handle<JSObject> checked = it->GetHolder<JSObject>();
-  while (AllCanRead(it)) {
-    if (it->state() == LookupIterator::ACCESSOR) {
-      return GetPropertyWithAccessor(it);
-    }
-    DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
+  Handle<InterceptorInfo> interceptor = GetInterceptorForFailedAccessCheck(it);
+  if (!interceptor.is_null()) {

[Toon Verwaest, 2016/06/22 11:30:16]

if (interceptor.is_null())? Negation just confuses.

+    MaybeHandle<Object> result;
     bool done;
-    Handle<Object> result;
-    ASSIGN_RETURN_ON_EXCEPTION(it->isolate(), result,
-                               GetPropertyWithInterceptor(it, &done), Object);
+    result = GetPropertyWithInterceptorInternal(it, interceptor, &done);
+    RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object);
     if (done) return result;
+  } else {
+    while (AllCanRead(it)) {
+      if (it->state() == LookupIterator::ACCESSOR) {
+        return GetPropertyWithAccessor(it);
+      }
+      DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
+      bool done;
+      Handle<Object> result;
+      ASSIGN_RETURN_ON_EXCEPTION(isolate, result,
+                                 GetPropertyWithInterceptor(it, &done), Object);
+      if (done) return result;
+    }
   }
 
   // Cross-Origin [[Get]] of Well-Known Symbols does not throw, and returns
@@ -1380,27 +1409,35 @@ MaybeHandle<Object> JSObject::GetPropertyWithFailedAccessCheck(
     return it->factory()->undefined_value();
   }
 
-  it->isolate()->ReportFailedAccessCheck(checked);
-  RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(it->isolate(), Object);
+  isolate->ReportFailedAccessCheck(checked);
+  RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object);
   return it->factory()->undefined_value();
 }
 
 
 Maybe<PropertyAttributes> JSObject::GetPropertyAttributesWithFailedAccessCheck(
     LookupIterator* it) {
+  Isolate* isolate = it->isolate();
   Handle<JSObject> checked = it->GetHolder<JSObject>();
-  while (AllCanRead(it)) {
-    if (it->state() == LookupIterator::ACCESSOR) {
-      return Just(it->property_attributes());
+  Handle<InterceptorInfo> interceptor = GetInterceptorForFailedAccessCheck(it);
+  if (!interceptor.is_null()) {
+    Maybe<PropertyAttributes> result =
+        GetPropertyAttributesWithInterceptorInternal(it, interceptor);
+    RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<PropertyAttributes>());
+    if (result.FromMaybe(ABSENT) != ABSENT) return result;
+  } else {
+    while (AllCanRead(it)) {
+      if (it->state() == LookupIterator::ACCESSOR) {
+        return Just(it->property_attributes());
+      }
+      DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
+      auto result = GetPropertyAttributesWithInterceptor(it);
+      if (isolate->has_scheduled_exception()) break;
+      if (result.IsJust() && result.FromJust() != ABSENT) return result;
     }
-    DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
-    auto result = GetPropertyAttributesWithInterceptor(it);
-    if (it->isolate()->has_scheduled_exception()) break;
-    if (result.IsJust() && result.FromJust() != ABSENT) return result;
   }
-  it->isolate()->ReportFailedAccessCheck(checked);
-  RETURN_VALUE_IF_SCHEDULED_EXCEPTION(it->isolate(),
-                                      Nothing<PropertyAttributes>());
+  isolate->ReportFailedAccessCheck(checked);
+  RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<PropertyAttributes>());
   return Just(ABSENT);
 }
 
@@ -1421,13 +1458,20 @@ bool JSObject::AllCanWrite(LookupIterator* it) {
 
 Maybe<bool> JSObject::SetPropertyWithFailedAccessCheck(
     LookupIterator* it, Handle<Object> value, ShouldThrow should_throw) {
+  Isolate* isolate = it->isolate();
   Handle<JSObject> checked = it->GetHolder<JSObject>();
-  if (AllCanWrite(it)) {
+  Handle<InterceptorInfo> interceptor = GetInterceptorForFailedAccessCheck(it);
+  if (!interceptor.is_null()) {
+    Maybe<bool> result = SetPropertyWithInterceptorInternal(
+        it, interceptor, should_throw, value);
+    RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
+    if (result.IsJust()) return result;
+  } else if (AllCanWrite(it)) {
     return SetPropertyWithAccessor(it, value, should_throw);
   }
 
-  it->isolate()->ReportFailedAccessCheck(checked);
-  RETURN_VALUE_IF_SCHEDULED_EXCEPTION(it->isolate(), Nothing<bool>());
+  isolate->ReportFailedAccessCheck(checked);
+  RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
   return Just(true);
 }
 
@@ -4217,17 +4261,14 @@ Handle<Map> Map::Update(Handle<Map> map) {
                              ALLOW_IN_DESCRIPTOR);
 }
 
-
-Maybe<bool> JSObject::SetPropertyWithInterceptor(LookupIterator* it,
-                                                 ShouldThrow should_throw,
-                                                 Handle<Object> value) {
+Maybe<bool> JSObject::SetPropertyWithInterceptorInternal(

[Toon Verwaest, 2016/06/22 11:30:16]

namespace {} helper method?

+    LookupIterator* it, Handle<InterceptorInfo> interceptor,
+    ShouldThrow should_throw, Handle<Object> value) {
   Isolate* isolate = it->isolate();
   // Make sure that the top context does not change when doing callbacks or
   // interceptor calls.
   AssertNoContextChange ncc(isolate);
 
-  DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
-  Handle<InterceptorInfo> interceptor(it->GetInterceptor());
   if (interceptor->setter()->IsUndefined(isolate)) return Just(false);
 
   Handle<JSObject> holder = it->GetHolder<JSObject>();
@@ -4266,6 +4307,13 @@ Maybe<bool> JSObject::SetPropertyWithInterceptor(LookupIterator* it,
   return Just(result);
 }
 
+Maybe<bool> JSObject::SetPropertyWithInterceptor(LookupIterator* it,
+                                                 ShouldThrow should_throw,
+                                                 Handle<Object> value) {
+  DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
+  return SetPropertyWithInterceptorInternal(it, it->GetInterceptor(),
+                                            should_throw, value);
+}
 
 MaybeHandle<Object> Object::SetProperty(Handle<Object> object,
                                         Handle<Name> name, Handle<Object> value,
@@ -5525,9 +5573,9 @@ MaybeHandle<Object> JSObject::DefinePropertyOrElementIgnoreAttributes(
   return DefineOwnPropertyIgnoreAttributes(&it, value, attributes);
 }
 
-
-Maybe<PropertyAttributes> JSObject::GetPropertyAttributesWithInterceptor(
-    LookupIterator* it) {
+Maybe<PropertyAttributes>
+JSObject::GetPropertyAttributesWithInterceptorInternal(

[Toon Verwaest, 2016/06/22 11:30:16]

Same here?

+    LookupIterator* it, Handle<InterceptorInfo> interceptor) {
   Isolate* isolate = it->isolate();
   // Make sure that the top context does not change when doing
   // callbacks or interceptor calls.
@@ -5535,7 +5583,6 @@ Maybe<PropertyAttributes> JSObject::GetPropertyAttributesWithInterceptor(
   HandleScope scope(isolate);
 
   Handle<JSObject> holder = it->GetHolder<JSObject>();
-  Handle<InterceptorInfo> interceptor(it->GetInterceptor());
   if (!it->IsElement() && it->name()->IsSymbol() &&
       !interceptor->can_intercept_symbols()) {
     return Just(ABSENT);
@@ -5591,6 +5638,10 @@ Maybe<PropertyAttributes> JSObject::GetPropertyAttributesWithInterceptor(
   return Just(ABSENT);
 }
 
+Maybe<PropertyAttributes> JSObject::GetPropertyAttributesWithInterceptor(
+    LookupIterator* it) {
+  return GetPropertyAttributesWithInterceptorInternal(it, it->GetInterceptor());
+}
 
 Maybe<PropertyAttributes> JSReceiver::GetPropertyAttributes(
     LookupIterator* it) {
@@ -15455,17 +15506,14 @@ void Dictionary<Derived, Shape, Key>::CopyValuesTo(FixedArray* elements) {
   DCHECK(pos == elements->length());
 }
 
-
-MaybeHandle<Object> JSObject::GetPropertyWithInterceptor(LookupIterator* it,
-                                                         bool* done) {
+MaybeHandle<Object> JSObject::GetPropertyWithInterceptorInternal(
+    LookupIterator* it, Handle<InterceptorInfo> interceptor, bool* done) {
   *done = false;
   Isolate* isolate = it->isolate();
   // Make sure that the top context does not change when doing callbacks or
   // interceptor calls.
   AssertNoContextChange ncc(isolate);
 
-  DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
-  Handle<InterceptorInfo> interceptor = it->GetInterceptor();
   if (interceptor->getter()->IsUndefined(isolate)) {
     return isolate->factory()->undefined_value();
   }
@@ -15506,6 +15554,11 @@ MaybeHandle<Object> JSObject::GetPropertyWithInterceptor(LookupIterator* it,
   return handle(*result, isolate);
 }
 
+MaybeHandle<Object> JSObject::GetPropertyWithInterceptor(LookupIterator* it,
+                                                         bool* done) {
+  DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
+  return GetPropertyWithInterceptorInternal(it, it->GetInterceptor(), done);
+}
 
 Maybe<bool> JSObject::HasRealNamedProperty(Handle<JSObject> object,
                                            Handle<Name> name) {
@@ -18863,5 +18916,20 @@ void PropertyCell::SetValueWithInvalidation(Handle<PropertyCell> cell,
   }
 }
 
+// static
+AccessCheckInfo* AccessCheckInfo::Get(Isolate* isolate,
+                                      Handle<JSObject> receiver) {
+  Object* maybe_constructor = receiver->map()->GetConstructor();
+  if (!maybe_constructor->IsJSFunction()) return NULL;

[Toon Verwaest, 2016/06/22 11:30:16]

Shouldn't this be a DCHECK; in addition to a
DCHECK(receiver->map()->is_access_check_needed())?

[jochen (gone - plz use gerrit), 2016/06/22 12:28:22]

it's null for detached global proxies

+  JSFunction* constructor = JSFunction::cast(maybe_constructor);
+  if (!constructor->shared()->IsApiFunction()) return NULL;

[Toon Verwaest, 2016/06/22 11:30:16]

In which case this has to be an API function, so another DCHECK.

[jochen (gone - plz use gerrit), 2016/06/22 12:28:22]

it's not for the debug context.

+
+  Object* data_obj =
+      constructor->shared()->get_api_func_data()->access_check_info();

[Toon Verwaest, 2016/06/22 11:30:16]

Unrelated to this particular CL, but we should probably make api-func-data
directly available on API functions rather than getting it from the SFI. We
could e.g., install it in the literals array field of the jsfunction upon
FunctionTemplate instantiation.

+  if (data_obj->IsUndefined(isolate)) return NULL;
+
+  return AccessCheckInfo::cast(data_obj);
+}
+
 }  // namespace internal
 }  // namespace v8