[api] Avoid needlessly calling descriptor interceptors

TimothyGu · Commit Bot · commit 9eb96bb431a5 · 2018-07-17T14:12:07.000Z
Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515. Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82 Reviewed-on: https://chromium-review.googlesource.com/1138808 Commit-Queue: Timothy Gu <timothygu@chromium.org> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org> Cr-Commit-Position: refs/heads/master@{#54492}
diff --git a/src/objects.cc b/src/objects.cc
@@ -7807,13 +7807,17 @@ 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) {
+  if (it->state() == LookupIterator::INTERCEPTOR) {
     Isolate* isolate = it->isolate();
     Handle<InterceptorInfo> interceptor = it->GetInterceptor();
     if (!interceptor->descriptor()->IsUndefined(isolate)) {
@@ -7845,9 +7849,9 @@ Maybe<bool> GetPropertyDescriptorWithInterceptor(LookupIterator* it,
 
         return Just(true);
       }
+      it->Next();
     }
   }
-  it->Restart();
   return Just(false);
 }
 }  // namespace
diff --git a/test/cctest/test-api-interceptors.cc b/test/cctest/test-api-interceptors.cc
@@ -4771,7 +4771,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);
@@ -4780,7 +4780,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);
@@ -4789,7 +4789,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;
 }
 
@@ -4858,7 +4858,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);
@@ -4867,7 +4867,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);
@@ -4876,7 +4876,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;
 }

Original file line number	Diff line number	Diff line change
`@@ -7807,13 +7807,17 @@ namespace {`
`7807`	`7807`
`7808`	`7808`	`Maybe<bool> GetPropertyDescriptorWithInterceptor(LookupIterator* it,`
`7809`	`7809`	`PropertyDescriptor* desc) {`
`7810`		`- bool has_access = true;`
`7811`	`7810`	`if (it->state() == LookupIterator::ACCESS_CHECK) {`
`7812`		`- has_access = it->HasAccess() \|\| JSObject::AllCanRead(it);`
`7813`		`- it->Next();`
	`7811`	`+ if (it->HasAccess()) {`
	`7812`	`+ it->Next();`
	`7813`	`+ } else if (!JSObject::AllCanRead(it) \|\|`
	`7814`	`+ it->state() != LookupIterator::INTERCEPTOR) {`
	`7815`	`+ it->Restart();`
	`7816`	`+ return Just(false);`
	`7817`	`+ }`
`7814`	`7818`	`}`
`7815`	`7819`
`7816`		`- if (has_access && it->state() == LookupIterator::INTERCEPTOR) {`
	`7820`	`+ if (it->state() == LookupIterator::INTERCEPTOR) {`
`7817`	`7821`	`Isolate* isolate = it->isolate();`
`7818`	`7822`	`Handle<InterceptorInfo> interceptor = it->GetInterceptor();`
`7819`	`7823`	`if (!interceptor->descriptor()->IsUndefined(isolate)) {`
`@@ -7845,9 +7849,9 @@ Maybe<bool> GetPropertyDescriptorWithInterceptor(LookupIterator* it,`
`7845`	`7849`
`7846`	`7850`	`return Just(true);`
`7847`	`7851`	`}`
	`7852`	`+ it->Next();`
`7848`	`7853`	`}`
`7849`	`7854`	`}`
`7850`		`- it->Restart();`
`7851`	`7855`	`return Just(false);`
`7852`	`7856`	`}`
`7853`	`7857`	`} // namespace`