Never skip access checks in the lookup iterator

BUG=
R=yangguo@chromium.org

Review URL: https://codereview.chromium.org/536943002

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@23661 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
index 59f9667..0790c9a 100644 (file)
--- a/src/bootstrapper.cc
+++ b/src/bootstrapper.cc
@@ -785,7 +785,7 @@ Handle<JSGlobalProxy> Genesis::CreateNewGlobals(
         name, code, prototype, JS_GLOBAL_OBJECT_TYPE, JSGlobalObject::kSize);
 #ifdef DEBUG
     LookupIterator it(prototype, factory()->constructor_string(),
-                      LookupIterator::OWN_PROPERTY);
+                      LookupIterator::OWN_SKIP_INTERCEPTOR);
     Handle<Object> value = JSReceiver::GetProperty(&it).ToHandleChecked();
     DCHECK(it.IsFound());
     DCHECK_EQ(*isolate()->object_function(), *value);
@@ -2485,7 +2485,8 @@ void Genesis::TransferNamedProperties(Handle<JSObject> from,
         }
         case CALLBACKS: {
           Handle<Name> key(descs->GetKey(i));
-          LookupIterator it(to, key, LookupIterator::OWN_PROPERTY);
+          LookupIterator it(to, key, LookupIterator::OWN_SKIP_INTERCEPTOR);
+          CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
           // If the property is already there we skip it
           if (it.IsFound() && it.HasProperty()) continue;
           HandleScope inner(isolate());
@@ -2513,7 +2514,8 @@ void Genesis::TransferNamedProperties(Handle<JSObject> from,
         DCHECK(raw_key->IsName());
         // If the property is already there we skip it.
         Handle<Name> key(Name::cast(raw_key));
-        LookupIterator it(to, key, LookupIterator::OWN_PROPERTY);
+        LookupIterator it(to, key, LookupIterator::OWN_SKIP_INTERCEPTOR);
+        CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
         if (it.IsFound() && it.HasProperty()) continue;
         // Set the property.
         Handle<Object> value = Handle<Object>(properties->ValueAt(i),

diff --git a/src/factory.cc b/src/factory.cc
index 85713ee..162cffc 100644 (file)
--- a/src/factory.cc
+++ b/src/factory.cc
@@ -2182,7 +2182,8 @@ Handle<JSFunction> Factory::CreateApiFunction(
   if (prototype->IsTheHole()) {
 #ifdef DEBUG
     LookupIterator it(handle(JSObject::cast(result->prototype())),
-                      constructor_string(), LookupIterator::OWN_PROPERTY);
+                      constructor_string(),
+                      LookupIterator::OWN_SKIP_INTERCEPTOR);
     MaybeHandle<Object> maybe_prop = Object::GetProperty(&it);
     DCHECK(it.IsFound());
     DCHECK(maybe_prop.ToHandleChecked().is_identical_to(result));

diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 21dbe2b..c923d2c 100644 (file)
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -5292,16 +5292,30 @@ void HOptimizedGraphBuilder::VisitConditional(Conditional* expr) {
 HOptimizedGraphBuilder::GlobalPropertyAccess
 HOptimizedGraphBuilder::LookupGlobalProperty(Variable* var, LookupIterator* it,
                                              PropertyAccessType access_type) {
-  DCHECK_EQ(*var->name(), *it->name());
   if (var->is_this() || !current_info()->has_global_object()) {
     return kUseGeneric;
   }
-  if (!it->HasProperty() || it->property_kind() != LookupIterator::DATA ||
-      (access_type == STORE && it->IsReadOnly())) {
-    return kUseGeneric;
-  }
 
-  return kUseCell;
+  switch (it->state()) {
+    case LookupIterator::ACCESS_CHECK:
+    case LookupIterator::INTERCEPTOR:
+    case LookupIterator::NOT_FOUND:
+      return kUseGeneric;
+    case LookupIterator::PROPERTY:
+      if (!it->HasProperty()) return kUseGeneric;
+      switch (it->property_kind()) {
+        case LookupIterator::DATA:
+          if (access_type == STORE && it->IsReadOnly()) return kUseGeneric;
+          return kUseCell;
+        case LookupIterator::ACCESSOR:
+          return kUseGeneric;
+      }
+    case LookupIterator::JSPROXY:
+    case LookupIterator::TRANSITION:
+      UNREACHABLE();
+  }
+  UNREACHABLE();
+  return kUseGeneric;
 }
 
 
@@ -5343,14 +5357,10 @@ void HOptimizedGraphBuilder::VisitVariableProxy(VariableProxy* expr) {
       }
 
       Handle<GlobalObject> global(current_info()->global_object());
-      LookupIterator it(global, variable->name(), LookupIterator::OWN_PROPERTY);
+      LookupIterator it(global, variable->name(),
+                        LookupIterator::OWN_SKIP_INTERCEPTOR);
       GlobalPropertyAccess type = LookupGlobalProperty(variable, &it, LOAD);
 
-      if (type == kUseCell &&
-          current_info()->global_object()->IsAccessCheckNeeded()) {
-        type = kUseGeneric;
-      }
-
       if (type == kUseCell) {
         Handle<PropertyCell> cell = it.GetPropertyCell();
         if (cell->type()->IsConstant()) {
@@ -5797,7 +5807,8 @@ HInstruction* HOptimizedGraphBuilder::BuildLoadNamedField(
         HConstant::cast(checked_object->ActualValue())->handle(isolate()));
 
     if (object->IsJSObject()) {
-      LookupIterator it(object, info->name(), LookupIterator::OWN_PROPERTY);
+      LookupIterator it(object, info->name(),
+                        LookupIterator::OWN_SKIP_INTERCEPTOR);
       Handle<Object> value = JSObject::GetDataProperty(&it);
       if (it.IsFound() && it.IsReadOnly() && !it.IsConfigurable()) {
         return New<HConstant>(value);
@@ -6461,7 +6472,7 @@ void HOptimizedGraphBuilder::HandleGlobalVariableAssignment(
     HValue* value,
     BailoutId ast_id) {
   Handle<GlobalObject> global(current_info()->global_object());
-  LookupIterator it(global, var->name(), LookupIterator::OWN_PROPERTY);
+  LookupIterator it(global, var->name(), LookupIterator::OWN_SKIP_INTERCEPTOR);
   GlobalPropertyAccess type = LookupGlobalProperty(var, &it, STORE);
   if (type == kUseCell) {
     Handle<PropertyCell> cell = it.GetPropertyCell();
@@ -9047,10 +9058,10 @@ void HOptimizedGraphBuilder::VisitCall(Call* expr) {
       // access check is not enabled we assume that the function will not change
       // and generate optimized code for calling the function.
       Handle<GlobalObject> global(current_info()->global_object());
-      LookupIterator it(global, var->name(), LookupIterator::OWN_PROPERTY);
+      LookupIterator it(global, var->name(),
+                        LookupIterator::OWN_SKIP_INTERCEPTOR);
       GlobalPropertyAccess type = LookupGlobalProperty(var, &it, LOAD);
-      if (type == kUseCell &&
-          !current_info()->global_object()->IsAccessCheckNeeded()) {
+      if (type == kUseCell) {
         Handle<GlobalObject> global(current_info()->global_object());
         known_global_function = expr->ComputeGlobalTarget(global, &it);
       }
@@ -10686,12 +10697,10 @@ void HOptimizedGraphBuilder::VisitCompareOperation(CompareOperation* expr) {
     Handle<JSFunction> target = Handle<JSFunction>::null();
     VariableProxy* proxy = expr->right()->AsVariableProxy();
     bool global_function = (proxy != NULL) && proxy->var()->IsUnallocated();
-    if (global_function &&
-        current_info()->has_global_object() &&
-        !current_info()->global_object()->IsAccessCheckNeeded()) {
+    if (global_function && current_info()->has_global_object()) {
       Handle<String> name = proxy->name();
       Handle<GlobalObject> global(current_info()->global_object());
-      LookupIterator it(global, name, LookupIterator::OWN_PROPERTY);
+      LookupIterator it(global, name, LookupIterator::OWN_SKIP_INTERCEPTOR);
       Handle<Object> value = JSObject::GetDataProperty(&it);
       if (it.IsFound() && value->IsJSFunction()) {
         Handle<JSFunction> candidate = Handle<JSFunction>::cast(value);

diff --git a/src/ic/ic.cc b/src/ic/ic.cc
index e23cad5..c59fa5d 100644 (file)
--- a/src/ic/ic.cc
+++ b/src/ic/ic.cc
@@ -283,7 +283,8 @@ bool IC::TryRemoveInvalidPrototypeDependentStub(Handle<Object> receiver,
 
   if (receiver->IsGlobalObject()) {
     Handle<GlobalObject> global = Handle<GlobalObject>::cast(receiver);
-    LookupIterator it(global, name, LookupIterator::OWN_PROPERTY);
+    LookupIterator it(global, name, LookupIterator::OWN_SKIP_INTERCEPTOR);
+    if (it.state() == LookupIterator::ACCESS_CHECK) return false;
     if (!it.IsFound() || !it.HasProperty()) return false;
     Handle<PropertyCell> cell = it.GetPropertyCell();
     return cell->type()->IsConstant();

diff --git a/src/isolate.cc b/src/isolate.cc
index 1dccc62..f4addcf 100644 (file)
--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -1055,7 +1055,7 @@ void Isolate::DoThrow(Object* exception, MessageLocation* location) {
           // probably not a valid Error object.  In that case, we fall through
           // and capture the stack trace at this throw site.
           LookupIterator lookup(exception_handle, key,
-                                LookupIterator::OWN_PROPERTY);
+                                LookupIterator::OWN_SKIP_INTERCEPTOR);
           Handle<Object> stack_trace_property;
           if (Object::GetProperty(&lookup).ToHandle(&stack_trace_property) &&
               stack_trace_property->IsJSArray()) {

diff --git a/src/lookup-inl.h b/src/lookup-inl.h
index d87221e..022bf7d 100644 (file)
--- a/src/lookup-inl.h
+++ b/src/lookup-inl.h
@@ -36,12 +36,8 @@ LookupIterator::State LookupIterator::LookupInHolder(Map* map) {
   DisallowHeapAllocation no_gc;
   switch (state_) {
     case NOT_FOUND:
-      if (map->IsJSProxyMap()) {
-        return JSPROXY;
-      }
-      if (check_access_check() && map->is_access_check_needed()) {
-        return ACCESS_CHECK;
-      }
+      if (map->IsJSProxyMap()) return JSPROXY;
+      if (map->is_access_check_needed()) return ACCESS_CHECK;
     // Fall through.
     case ACCESS_CHECK:
       if (check_interceptor() && map->has_named_interceptor()) {

diff --git a/src/lookup.h b/src/lookup.h
index 3d263e9..6427aa9 100644 (file)
--- a/src/lookup.h
+++ b/src/lookup.h
@@ -16,21 +16,17 @@ class LookupIterator FINAL BASE_EMBEDDED {
  public:
   enum Configuration {
     // Configuration bits.
-    kAccessCheck = 1 << 0,
-    kHidden = 1 << 1,
-    kInterceptor = 1 << 2,
-    kPrototypeChain = 1 << 3,
+    kHidden = 1 << 0,
+    kInterceptor = 1 << 1,
+    kPrototypeChain = 1 << 2,
 
     // Convience combinations of bits.
-    OWN_PROPERTY = 0,
-    OWN_SKIP_INTERCEPTOR = kAccessCheck,
-    OWN = kAccessCheck | kInterceptor,
-    HIDDEN_PROPERTY = kHidden,
-    HIDDEN_SKIP_INTERCEPTOR = kAccessCheck | kHidden,
-    HIDDEN = kAccessCheck | kHidden | kInterceptor,
-    PROTOTYPE_CHAIN_PROPERTY = kHidden | kPrototypeChain,
-    PROTOTYPE_CHAIN_SKIP_INTERCEPTOR = kAccessCheck | kHidden | kPrototypeChain,
-    PROTOTYPE_CHAIN = kAccessCheck | kHidden | kPrototypeChain | kInterceptor
+    OWN_SKIP_INTERCEPTOR = 0,
+    OWN = kInterceptor,
+    HIDDEN_SKIP_INTERCEPTOR = kHidden,
+    HIDDEN = kHidden | kInterceptor,
+    PROTOTYPE_CHAIN_SKIP_INTERCEPTOR = kHidden | kPrototypeChain,
+    PROTOTYPE_CHAIN = kHidden | kPrototypeChain | kInterceptor
   };
 
   enum State {
@@ -191,9 +187,6 @@ class LookupIterator FINAL BASE_EMBEDDED {
   bool is_guaranteed_to_have_holder() const {
     return !maybe_receiver_.is_null();
   }
-  bool check_access_check() const {
-    return (configuration_ & kAccessCheck) != 0;
-  }
   bool check_hidden() const { return (configuration_ & kHidden) != 0; }
   bool check_interceptor() const {
     return !IsBootstrapping() && (configuration_ & kInterceptor) != 0;

diff --git a/src/objects.cc b/src/objects.cc
index cb13526..0ccd927 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -144,7 +144,8 @@ MaybeHandle<Object> Object::GetProperty(LookupIterator* it) {
 
 Handle<Object> JSObject::GetDataProperty(Handle<JSObject> object,
                                          Handle<Name> key) {
-  LookupIterator it(object, key, LookupIterator::PROTOTYPE_CHAIN_PROPERTY);
+  LookupIterator it(object, key,
+                    LookupIterator::PROTOTYPE_CHAIN_SKIP_INTERCEPTOR);
   return GetDataProperty(&it);
 }
 
@@ -152,11 +153,13 @@ Handle<Object> JSObject::GetDataProperty(Handle<JSObject> object,
 Handle<Object> JSObject::GetDataProperty(LookupIterator* it) {
   for (; it->IsFound(); it->Next()) {
     switch (it->state()) {
-      case LookupIterator::ACCESS_CHECK:
       case LookupIterator::INTERCEPTOR:
       case LookupIterator::NOT_FOUND:
       case LookupIterator::TRANSITION:
         UNREACHABLE();
+      case LookupIterator::ACCESS_CHECK:
+        if (it->HasAccess(v8::ACCESS_GET)) continue;
+      // Fall through.
       case LookupIterator::JSPROXY:
         it->NotFound();
         return it->isolate()->factory()->undefined_value();
@@ -3787,7 +3790,8 @@ void JSObject::WriteToField(int descriptor, Object* value) {
 void JSObject::AddProperty(Handle<JSObject> object, Handle<Name> name,
                            Handle<Object> value,
                            PropertyAttributes attributes) {
-  LookupIterator it(object, name, LookupIterator::OWN_PROPERTY);
+  LookupIterator it(object, name, LookupIterator::OWN_SKIP_INTERCEPTOR);
+  CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
 #ifdef DEBUG
   uint32_t index;
   DCHECK(!object->IsJSProxy());
@@ -4687,11 +4691,9 @@ void JSObject::DeleteHiddenProperty(Handle<JSObject> object, Handle<Name> key) {
 
 bool JSObject::HasHiddenProperties(Handle<JSObject> object) {
   Handle<Name> hidden = object->GetIsolate()->factory()->hidden_string();
-  LookupIterator it(object, hidden, LookupIterator::OWN_PROPERTY);
-  Maybe<PropertyAttributes> maybe = GetPropertyAttributes(&it);
-  // Cannot get an exception since the hidden_string isn't accessible to JS.
-  DCHECK(maybe.has_value);
-  return maybe.value != ABSENT;
+  LookupIterator it(object, hidden, LookupIterator::OWN_SKIP_INTERCEPTOR);
+  CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
+  return it.IsFound() && it.HasProperty();
 }
 
 
@@ -4722,7 +4724,8 @@ Object* JSObject::GetHiddenPropertiesHashTable() {
   } else {
     Isolate* isolate = GetIsolate();
     LookupIterator it(handle(this), isolate->factory()->hidden_string(),
-                      LookupIterator::OWN_PROPERTY);
+                      LookupIterator::OWN_SKIP_INTERCEPTOR);
+    CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
     if (it.IsFound() && it.HasProperty()) {
       DCHECK_EQ(LookupIterator::DATA, it.property_kind());
       return *it.GetDataValue();
@@ -6163,7 +6166,8 @@ MaybeHandle<Object> JSObject::DefineAccessor(Handle<JSObject> object,
            setter->IsNull());
     // At least one of the accessors needs to be a new value.
     DCHECK(!getter->IsNull() || !setter->IsNull());
-    LookupIterator it(object, name, LookupIterator::OWN_PROPERTY);
+    LookupIterator it(object, name, LookupIterator::OWN_SKIP_INTERCEPTOR);
+    CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
     if (!getter->IsNull()) {
       it.TransitionToAccessorProperty(ACCESSOR_GETTER, getter, attributes);
     }
@@ -12843,7 +12847,8 @@ bool JSArray::WouldChangeReadOnlyLength(Handle<JSArray> array,
   CHECK(array->length()->ToArrayIndex(&length));
   if (length <= index) {
     LookupIterator it(array, array->GetIsolate()->factory()->length_string(),
-                      LookupIterator::OWN_PROPERTY);
+                      LookupIterator::OWN_SKIP_INTERCEPTOR);
+    CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
     CHECK(it.IsFound());
     CHECK(it.HasProperty());
     return it.IsReadOnly();

diff --git a/src/runtime.cc b/src/runtime.cc
index e627706..2dab6eb 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -2180,12 +2180,12 @@ static Object* DeclareGlobals(Isolate* isolate, Handle<GlobalObject> global,
                               PropertyAttributes attr, bool is_var,
                               bool is_const, bool is_function) {
   // Do the lookup own properties only, see ES5 erratum.
-  LookupIterator it(global, name, LookupIterator::HIDDEN_PROPERTY);
+  LookupIterator it(global, name, LookupIterator::HIDDEN_SKIP_INTERCEPTOR);
   Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
-  DCHECK(maybe.has_value);
-  PropertyAttributes old_attributes = maybe.value;
+  if (!maybe.has_value) return isolate->heap()->exception();
 
-  if (old_attributes != ABSENT) {
+  if (it.IsFound()) {
+    PropertyAttributes old_attributes = maybe.value;
     // The name was declared before; check for conflicting re-declarations.
     if (is_const) return ThrowRedeclarationError(isolate, name);
 
@@ -2310,7 +2310,7 @@ RUNTIME_FUNCTION(Runtime_InitializeConstGlobal) {
   Handle<GlobalObject> global = isolate->global_object();
 
   // Lookup the property as own on the global object.
-  LookupIterator it(global, name, LookupIterator::HIDDEN_PROPERTY);
+  LookupIterator it(global, name, LookupIterator::HIDDEN_SKIP_INTERCEPTOR);
   Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
   DCHECK(maybe.has_value);
   PropertyAttributes old_attributes = maybe.value;
@@ -2460,7 +2460,7 @@ RUNTIME_FUNCTION(Runtime_InitializeLegacyConstLookupSlot) {
     // code can run in between that modifies the declared property.
     DCHECK(holder->IsJSGlobalObject() || holder->IsJSContextExtensionObject());
 
-    LookupIterator it(holder, name, LookupIterator::HIDDEN_PROPERTY);
+    LookupIterator it(holder, name, LookupIterator::HIDDEN_SKIP_INTERCEPTOR);
     Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
     if (!maybe.has_value) return isolate->heap()->exception();
     PropertyAttributes old_attributes = maybe.value;
@@ -5040,14 +5040,14 @@ RUNTIME_FUNCTION(Runtime_DefineDataPropertyUnchecked) {
   RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0);
   PropertyAttributes attr = static_cast<PropertyAttributes>(unchecked);
 
-  // Check access rights if needed.
-  if (js_object->IsAccessCheckNeeded() &&
-      !isolate->MayNamedAccess(js_object, name, v8::ACCESS_SET)) {
-    return isolate->heap()->undefined_value();
+  LookupIterator it(js_object, name, LookupIterator::OWN_SKIP_INTERCEPTOR);
+  if (it.IsFound() && it.state() == LookupIterator::ACCESS_CHECK) {
+    if (!isolate->MayNamedAccess(js_object, name, v8::ACCESS_SET)) {
+      return isolate->heap()->undefined_value();
+    }
+    it.Next();
   }
 
-  LookupIterator it(js_object, name, LookupIterator::OWN_PROPERTY);
-
   // Take special care when attributes are different and there is already
   // a property.
   if (it.IsFound() && it.HasProperty() &&
@@ -5293,9 +5293,9 @@ RUNTIME_FUNCTION(Runtime_AddNamedProperty) {
 #ifdef DEBUG
   uint32_t index = 0;
   DCHECK(!key->ToArrayIndex(&index));
-  LookupIterator it(object, key, LookupIterator::OWN_PROPERTY);
+  LookupIterator it(object, key, LookupIterator::OWN_SKIP_INTERCEPTOR);
   Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
-  DCHECK(maybe.has_value);
+  if (!maybe.has_value) return isolate->heap()->exception();
   RUNTIME_ASSERT(!it.IsFound());
 #endif
 
@@ -5325,7 +5325,7 @@ RUNTIME_FUNCTION(Runtime_AddPropertyForTemplate) {
   bool duplicate;
   if (key->IsName()) {
     LookupIterator it(object, Handle<Name>::cast(key),
-                      LookupIterator::OWN_PROPERTY);
+                      LookupIterator::OWN_SKIP_INTERCEPTOR);
     Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
     DCHECK(maybe.has_value);
     duplicate = it.IsFound();

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index db0831c..a00eb0d 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -2044,7 +2044,8 @@ THREADED_TEST(ExecutableAccessorIsPreservedOnAttributeChange) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   i::LookupResult lookup(i_isolate);
   i::Handle<i::String> name(v8::Utils::OpenHandle(*v8_str("length")));
-  i::LookupIterator it(a, name, i::LookupIterator::OWN_PROPERTY);
+  i::LookupIterator it(a, name, i::LookupIterator::OWN_SKIP_INTERCEPTOR);
+  CHECK_NE(i::LookupIterator::ACCESS_CHECK, it.state());
   CHECK(it.HasProperty());
   CHECK(it.GetAccessors()->IsExecutableAccessorInfo());
 }