Revert "[api] Prefer Descriptor interceptor over Getter in GetPropertyAttributes"

This reverts commit d5fbf7c5.

Reason for revert: Performance regression, see https://bugs.chromium.org/p/chromium/issues/detail?id=798279

Original change's description:
> [api] Prefer Descriptor interceptor over Getter in GetPropertyAttributes
> 
> Also fix GetPropertyDescriptorWithInterceptor so that it only calls the
> interceptor once.
> 
> R=​ahaas@chromium.org, franzih@chromium.org
> 
> Bug: node:17480, node:17481
> Change-Id: I2c3813f80df2962ec909bae7267884ce0b8ccbef
> Reviewed-on: https://chromium-review.googlesource.com/816515
> Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
> Reviewed-by: Franziska Hinkelmann <franzih@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#50260}

TBR=timothygu99@gmail.com,ahaas@chromium.org,franzih@chromium.org,sergiyb@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: node:17480, node:17481
Change-Id: I4997e0f3a330d719026e56dd83c1bb999b986bcf
Reviewed-on: https://chromium-review.googlesource.com/850355Reviewed-by: Franziska Hinkelmann <franzih@chromium.org>
Commit-Queue: Franziska Hinkelmann <franzih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50362}

AUTHORS

@@ -135,7 +135,6 @@ Sanjoy Das <sanjoy@playingwithpointers.com>
 Seo Sanghyeon <sanxiyn@gmail.com>
 Stefan Penner <stefan.penner@gmail.com>
 Sylvestre Ledru <sledru@mozilla.com>
-Timothy Gu <timothygu99@gmail.com>
 Tobias Burnus <burnus@net-b.de>
 Victor Costan <costan@gmail.com>
 Vlad Burlik <vladbph@gmail.com>

src/objects.cc

@@ -1895,22 +1895,6 @@ Maybe<PropertyAttributes> GetPropertyAttributesWithInterceptorInternal(
       CHECK(result->ToInt32(&value));
       return Just(static_cast<PropertyAttributes>(value));
     }
-  } else if (!interceptor->descriptor()->IsUndefined(isolate)) {
-    Handle<Object> result;
-    if (it->IsElement()) {
-      result = args.CallIndexedDescriptor(interceptor, it->index());
-    } else {
-      result = args.CallNamedDescriptor(interceptor, it->name());
-    }
-    if (!result.is_null()) {
-      PropertyDescriptor desc;
-      Utils::ApiCheck(
-          PropertyDescriptor::ToPropertyDescriptor(isolate, result, &desc),
-          it->IsElement() ? "v8::IndexedPropertyDescriptorCallback"
-                          : "v8::NamedPropertyDescriptorCallback",
-          "Invalid property descriptor.");
-      return Just(desc.ToAttributes());
-    }
   } else if (!interceptor->getter()->IsUndefined(isolate)) {
     // TODO(verwaest): Use GetPropertyWithInterceptor?
     Handle<Object> result;
@@ -7638,17 +7622,13 @@ namespace {
 
 Maybe<bool> GetPropertyDescriptorWithInterceptor(LookupIterator* it,
                                                  PropertyDescriptor* desc) {
+  bool has_access = true;
   if (it->state() == LookupIterator::ACCESS_CHECK) {
-    if (it->HasAccess()) {
-      it->Next();
-    } else if (!JSObject::AllCanRead(it) ||
-               it->state() != LookupIterator::INTERCEPTOR) {
-      it->Restart();
-      return Just(false);
-    }
+    has_access = it->HasAccess() || JSObject::AllCanRead(it);
+    it->Next();
   }
 
-  if (it->state() == LookupIterator::INTERCEPTOR) {
+  if (has_access && it->state() == LookupIterator::INTERCEPTOR) {
     Isolate* isolate = it->isolate();
     Handle<InterceptorInfo> interceptor = it->GetInterceptor();
     if (!interceptor->descriptor()->IsUndefined(isolate)) {
@@ -7680,9 +7660,9 @@ Maybe<bool> GetPropertyDescriptorWithInterceptor(LookupIterator* it,
 
         return Just(true);
       }
-      it->Next();
     }
   }
+  it->Restart();
   return Just(false);
 }
 }  // namespace

test/cctest/test-api-interceptors.cc

@@ -2029,18 +2029,14 @@ THREADED_TEST(PropertyDefinerCallbackWithSetter) {
 }
 
 namespace {
-int empty_descriptor_called;
 void EmptyPropertyDescriptorCallback(
     Local<Name> name, const v8::PropertyCallbackInfo<v8::Value>& info) {
   // Do not intercept by not calling info.GetReturnValue().Set().
-  empty_descriptor_called++;
 }
 
-int intercepting_descriptor_called;
 void InterceptingPropertyDescriptorCallback(
     Local<Name> name, const v8::PropertyCallbackInfo<v8::Value>& info) {
   // Intercept the callback by setting a different descriptor.
-  intercepting_descriptor_called++;
   const char* code =
       "var desc = {value: 42};"
       "desc;";
@@ -2049,12 +2045,6 @@ void InterceptingPropertyDescriptorCallback(
                                 .ToLocalChecked();
   info.GetReturnValue().Set(descriptor);
 }
-
-template <class S, class T>
-void CrashingPropertyCallback(Local<S> name,
-                              const v8::PropertyCallbackInfo<T>& info) {
-  UNREACHABLE();
-}
 }  // namespace
 
 THREADED_TEST(PropertyDescriptorCallback) {
@@ -2062,11 +2052,10 @@ THREADED_TEST(PropertyDescriptorCallback) {
   LocalContext env;
 
   {  // Normal behavior of getOwnPropertyDescriptor() with empty callback.
-    empty_descriptor_called = 0;
     v8::Local<v8::FunctionTemplate> templ =
         v8::FunctionTemplate::New(CcTest::isolate());
     templ->InstanceTemplate()->SetHandler(v8::NamedPropertyHandlerConfiguration(
-        CrashingPropertyCallback, 0, EmptyPropertyDescriptorCallback, 0, 0, 0));
+        0, 0, EmptyPropertyDescriptorCallback, 0, 0, 0));
     env->Global()
         ->Set(env.local(), v8_str("obj"), templ->GetFunction(env.local())
                                               .ToLocalChecked()
@@ -2082,21 +2071,9 @@ THREADED_TEST(PropertyDescriptorCallback) {
                      .ToLocalChecked()
                      ->Int32Value(env.local())
                      .FromJust());
-    CHECK_EQ(false, v8_compile("'empty' in obj;")
-                        ->Run(env.local())
-                        .ToLocalChecked()
-                        ->BooleanValue(env.local())
-                        .FromJust());
-    CHECK_EQ(true, v8_compile("'x' in obj;")
-                       ->Run(env.local())
-                       .ToLocalChecked()
-                       ->BooleanValue(env.local())
-                       .FromJust());
-    CHECK_EQ(3, empty_descriptor_called);
   }
 
   {  // Intercept getOwnPropertyDescriptor().
-    intercepting_descriptor_called = 0;
     v8::Local<v8::FunctionTemplate> templ =
         v8::FunctionTemplate::New(CcTest::isolate());
     templ->InstanceTemplate()->SetHandler(v8::NamedPropertyHandlerConfiguration(
@@ -2116,17 +2093,6 @@ THREADED_TEST(PropertyDescriptorCallback) {
                      .ToLocalChecked()
                      ->Int32Value(env.local())
                      .FromJust());
-    CHECK_EQ(true, v8_compile("'empty' in obj;")
-                       ->Run(env.local())
-                       .ToLocalChecked()
-                       ->BooleanValue(env.local())
-                       .FromJust());
-    CHECK_EQ(true, v8_compile("'x' in obj;")
-                       ->Run(env.local())
-                       .ToLocalChecked()
-                       ->BooleanValue(env.local())
-                       .FromJust());
-    CHECK_EQ(3, intercepting_descriptor_called);
   }
 }
 
@@ -4748,7 +4714,7 @@ TEST(NamedAllCanReadInterceptor) {
   ExpectInt32("checked.whatever", 17);
   CHECK(!CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')")
              ->IsUndefined());
-  CHECK_EQ(5, access_check_data.count);
+  CHECK_EQ(6, access_check_data.count);
 
   access_check_data.result = false;
   ExpectInt32("checked.whatever", intercept_data_0.value);
@@ -4757,7 +4723,7 @@ TEST(NamedAllCanReadInterceptor) {
     CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')");
     CHECK(try_catch.HasCaught());
   }
-  CHECK_EQ(8, access_check_data.count);
+  CHECK_EQ(9, access_check_data.count);
 
   intercept_data_1.should_intercept = true;
   ExpectInt32("checked.whatever", intercept_data_1.value);
@@ -4766,7 +4732,7 @@ TEST(NamedAllCanReadInterceptor) {
     CompileRun("Object.getOwnPropertyDescriptor(checked, 'whatever')");
     CHECK(try_catch.HasCaught());
   }
-  CHECK_EQ(11, access_check_data.count);
+  CHECK_EQ(12, access_check_data.count);
   g_access_check_data = nullptr;
 }
 
@@ -4835,7 +4801,7 @@ TEST(IndexedAllCanReadInterceptor) {
   ExpectInt32("checked[15]", 17);
   CHECK(!CompileRun("Object.getOwnPropertyDescriptor(checked, '15')")
              ->IsUndefined());
-  CHECK_EQ(5, access_check_data.count);
+  CHECK_EQ(6, access_check_data.count);
 
   access_check_data.result = false;
   ExpectInt32("checked[15]", intercept_data_0.value);
@@ -4844,7 +4810,7 @@ TEST(IndexedAllCanReadInterceptor) {
     CompileRun("Object.getOwnPropertyDescriptor(checked, '15')");
     CHECK(try_catch.HasCaught());
   }
-  CHECK_EQ(8, access_check_data.count);
+  CHECK_EQ(9, access_check_data.count);
 
   intercept_data_1.should_intercept = true;
   ExpectInt32("checked[15]", intercept_data_1.value);
@@ -4853,7 +4819,7 @@ TEST(IndexedAllCanReadInterceptor) {
     CompileRun("Object.getOwnPropertyDescriptor(checked, '15')");
     CHECK(try_catch.HasCaught());
   }
-  CHECK_EQ(11, access_check_data.count);
+  CHECK_EQ(12, access_check_data.count);
 
   g_access_check_data = nullptr;
 }