Optionally invoke an interceptor on failed access checks

src/keys.cc

Index: src/keys.cc
diff --git a/src/keys.cc b/src/keys.cc
index eee015075a584871fcb0aba448d63aca94601740..61e0589fffff25259a11897e4d6711a7c46f1c85 100644
--- a/src/keys.cc
+++ b/src/keys.cc
@@ -434,18 +434,30 @@ Maybe<bool> CollectInterceptorKeys(Handle<JSReceiver> receiver,
                                    Handle<JSObject> object,
                                    KeyAccumulator* accumulator) {
   Isolate* isolate = accumulator->isolate();
-  if (type == kIndexed) {
-    if (!object->HasIndexedInterceptor()) return Just(true);
+  Handle<InterceptorInfo> interceptor;
+  if ((accumulator->filter() & USE_ACCESS_CHECK_INTERCEPTOR) &&
+      object->IsAccessCheckNeeded()) {
+    DisallowHeapAllocation no_gc;
+    AccessCheckInfo* access_check_info = AccessCheckInfo::Get(isolate, object);
+    if (!access_check_info) return Just(true);
+    Object* maybe_interceptor = type == kIndexed
+                                    ? access_check_info->indexed_interceptor()
+                                    : access_check_info->named_interceptor();
+    if (!maybe_interceptor) return Just(true);
+    interceptor = handle(InterceptorInfo::cast(maybe_interceptor), isolate);
   } else {
-    if (!object->HasNamedInterceptor()) return Just(true);
-  }
-  Handle<InterceptorInfo> interceptor(type == kIndexed
-                                          ? object->GetIndexedInterceptor()
+    if (type == kIndexed) {
+      if (!object->HasIndexedInterceptor()) return Just(true);
+    } else {
+      if (!object->HasNamedInterceptor()) return Just(true);
+    }
+    interceptor = handle(type == kIndexed ? object->GetIndexedInterceptor()
                                           : object->GetNamedInterceptor(),
-                                      isolate);
-  if ((accumulator->filter() & ONLY_ALL_CAN_READ) &&
-      !interceptor->all_can_read()) {
-    return Just(true);
+                         isolate);
+    if ((accumulator->filter() & ONLY_ALL_CAN_READ) &&
+        !interceptor->all_can_read()) {
+      return Just(true);
+    }
   }
   PropertyCallbackArguments args(isolate, interceptor->data(), *receiver,
                                  *object, Object::DONT_THROW);
@@ -484,6 +496,7 @@ int CollectOwnPropertyNamesInternal(Handle<JSObject> object,
                                     KeyAccumulator* keys,
                                     Handle<DescriptorArray> descs,
                                     int start_index, int limit) {
+  DCHECK(!(keys->filter() & USE_ACCESS_CHECK_INTERCEPTOR));
   int first_skipped = -1;
   for (int i = start_index; i < limit; i++) {
     PropertyDetails details = descs->GetDetails(i);
@@ -513,7 +526,7 @@ Maybe<bool> KeyAccumulator::CollectOwnPropertyNames(Handle<JSReceiver> receiver,
     Handle<FixedArray> enum_keys =
         KeyAccumulator::GetEnumPropertyKeys(isolate_, object);
     AddKeys(enum_keys, DO_NOT_CONVERT);
-  } else {
+  } else if (!(filter_ & USE_ACCESS_CHECK_INTERCEPTOR)) {
     if (object->HasFastProperties()) {
       int limit = object->map()->NumberOfOwnDescriptors();
       Handle<DescriptorArray> descs(object->map()->instance_descriptors(),
@@ -546,6 +559,8 @@ Maybe<bool> KeyAccumulator::CollectOwnKeys(Handle<JSReceiver> receiver,
   // Check access rights if required.
   if (object->IsAccessCheckNeeded() &&
       !isolate_->MayAccess(handle(isolate_->context()), object)) {
+    DisallowHeapAllocation no_gc;
+
     // The cross-origin spec says that [[Enumerate]] shall return an empty
     // iterator when it doesn't have access...
     if (mode_ == KeyCollectionMode::kIncludePrototypes) {
@@ -553,7 +568,14 @@ Maybe<bool> KeyAccumulator::CollectOwnKeys(Handle<JSReceiver> receiver,
     }
     // ...whereas [[OwnPropertyKeys]] shall return whitelisted properties.
     DCHECK(KeyCollectionMode::kOwnOnly == mode_);
-    filter_ = static_cast<PropertyFilter>(filter_ | ONLY_ALL_CAN_READ);
+    AccessCheckInfo* access_check_info = AccessCheckInfo::Get(isolate_, object);
+    // We always either have both a named and an indexed interceptor or none.
+    if (!access_check_info || !access_check_info->named_interceptor()) {
+      filter_ = static_cast<PropertyFilter>(filter_ | ONLY_ALL_CAN_READ);
+    } else {
+      filter_ =
+          static_cast<PropertyFilter>(filter_ | USE_ACCESS_CHECK_INTERCEPTOR);

[Toon Verwaest, 2016/06/27 09:34:58]

We should get rid of these filters. Just duplicate the collecting path; we're
not going to walk the prototype chain anyway.

So I'd suggest keeping the old ALL_CAN_READ filter for the old case, removing it
once we migrated everyone over. And calling into a new method that just returns
the values from the interceptor directly, rather than reusing the complex
machinery of CollectOwn*; which becomes even more complex with these flags.

Keeping things separate will make both paths faster and less convoluted.

[Toon Verwaest, 2016/06/27 09:37:59]

I think you basically want to reuse the lower half of CollectInterceptorKeys and
only that for the !MayAccess case.

+    }
   }
   MAYBE_RETURN(CollectOwnElementIndices(receiver, object), Nothing<bool>());
   MAYBE_RETURN(CollectOwnPropertyNames(receiver, object), Nothing<bool>());
@@ -563,6 +585,8 @@ Maybe<bool> KeyAccumulator::CollectOwnKeys(Handle<JSReceiver> receiver,
 // static
 Handle<FixedArray> KeyAccumulator::GetEnumPropertyKeys(
     Isolate* isolate, Handle<JSObject> object) {
+  DCHECK(!object->IsAccessCheckNeeded() ||
+         isolate->MayAccess(handle(isolate->context()), object));
   if (object->HasFastProperties()) {
     return GetFastEnumPropertyKeys(isolate, object);
   } else if (object->IsJSGlobalObject()) {