Use the LookupIterator in SetAccessor

BUG=v8:4137
LOG=n

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

Cr-Commit-Position: refs/heads/master@{#28955}

src/objects.cc

@@ -6474,75 +6474,52 @@ MaybeHandle<Object> JSObject::DefineAccessor(Handle<JSObject> object,
 MaybeHandle<Object> JSObject::SetAccessor(Handle<JSObject> object,
                                           Handle<AccessorInfo> info) {
   Isolate* isolate = object->GetIsolate();
-  Factory* factory = isolate->factory();
-  Handle<Name> name(Name::cast(info->name()));
-
-  // Check access rights if needed.
-  if (object->IsAccessCheckNeeded() && !isolate->MayAccess(object)) {
-    isolate->ReportFailedAccessCheck(object);
-    RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object);
-    return factory->undefined_value();
-  }
-
-  if (object->IsJSGlobalProxy()) {
-    PrototypeIterator iter(isolate, object);
-    if (iter.IsAtEnd()) return object;
-    DCHECK(PrototypeIterator::GetCurrent(iter)->IsJSGlobalObject());
-    return SetAccessor(
-        Handle<JSObject>::cast(PrototypeIterator::GetCurrent(iter)), info);
-  }
 
   // Make sure that the top context does not change when doing callbacks or
   // interceptor calls.
   AssertNoContextChange ncc(isolate);
 
   // Try to flatten before operating on the string.
+  Handle<Name> name(Name::cast(info->name()));
   if (name->IsString()) name = String::Flatten(Handle<String>::cast(name));
 
   uint32_t index = 0;
-  bool is_element = name->AsArrayIndex(&index);
-
-  if (is_element) {
-    if (object->IsJSArray()) return factory->undefined_value();
+  LookupIterator::Configuration c = LookupIterator::HIDDEN_SKIP_INTERCEPTOR;
+  LookupIterator it = name->AsArrayIndex(&index)
+                          ? LookupIterator(isolate, object, index, c)
+                          : LookupIterator(object, name, c);
 
-    // Accessors overwrite previous callbacks (cf. with getters/setters).
-    switch (object->GetElementsKind()) {
-      case FAST_SMI_ELEMENTS:
-      case FAST_ELEMENTS:
-      case FAST_DOUBLE_ELEMENTS:
-      case FAST_HOLEY_SMI_ELEMENTS:
-      case FAST_HOLEY_ELEMENTS:
-      case FAST_HOLEY_DOUBLE_ELEMENTS:
-        break;
+  // Duplicate ACCESS_CHECK outside of GetPropertyAttributes for the case that
+  // the FailedAccessCheckCallbackFunction doesn't throw an exception.
+  //
+  // TODO(verwaest): Force throw an exception if the callback doesn't, so we can
+  // remove reliance on default return values.
+  if (it.state() == LookupIterator::ACCESS_CHECK) {
+    if (!it.HasAccess()) {
+      isolate->ReportFailedAccessCheck(object);
+      RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, Object);
+      return it.factory()->undefined_value();
+    }
+    it.Next();
+  }
 
-#define TYPED_ARRAY_CASE(Type, type, TYPE, ctype, size)                        \
-      case EXTERNAL_##TYPE##_ELEMENTS:                                         \
-      case TYPE##_ELEMENTS:                                                    \
+  CHECK(GetPropertyAttributes(&it).IsJust());
 
-      TYPED_ARRAYS(TYPED_ARRAY_CASE)
-#undef TYPED_ARRAY_CASE
-        // Ignore getters and setters on pixel and external array
-        // elements.
-        return factory->undefined_value();
+  // ES5 forbids turning a property into an accessor if it's not
+  // configurable. See 8.6.1 (Table 5).
+  if (it.IsFound() && (it.IsReadOnly() || !it.IsConfigurable())) {
+    return it.factory()->undefined_value();
+  }
 
-      case DICTIONARY_ELEMENTS:
-        break;
-      case SLOPPY_ARGUMENTS_ELEMENTS:
-        UNIMPLEMENTED();
-        break;
-    }
+  // Ignore accessors on typed arrays.
+  if (it.IsElement() && (object->HasFixedTypedArrayElements() ||
+                         object->HasExternalArrayElements())) {
+    return it.factory()->undefined_value();
+  }
 
+  if (it.IsElement()) {
     SetElementCallback(object, index, info, info->property_attributes());
   } else {
-    // Lookup the name.
-    LookupIterator it(object, name, LookupIterator::HIDDEN_SKIP_INTERCEPTOR);
-    CHECK(GetPropertyAttributes(&it).IsJust());
-    // ES5 forbids turning a property into an accessor if it's not
-    // configurable. See 8.6.1 (Table 5).
-    if (it.IsFound() && (it.IsReadOnly() || !it.IsConfigurable())) {
-      return factory->undefined_value();
-    }
-
     SetPropertyCallback(object, name, info, info->property_attributes());
   }
 

test/cctest/test-api.cc

@@ -16781,6 +16781,8 @@ void FailedAccessCheckCallbackGC(Local<v8::Object> target,
                                  v8::AccessType type,
                                  Local<v8::Value> data) {
   CcTest::heap()->CollectAllGarbage();
+  CcTest::isolate()->ThrowException(
+      v8::Exception::Error(v8_str("cross context")));
 }
 
 
@@ -16811,28 +16813,42 @@ TEST(GCInFailedAccessCheckCallback) {
   LocalContext context1(NULL, global_template);
   context1->Global()->Set(v8_str("other"), global0);
 
+  v8::TryCatch try_catch(isolate);
+
   // Get property with failed access check.
-  ExpectUndefined("other.x");
+  CHECK(CompileRun("other.x").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // Get element with failed access check.
-  ExpectUndefined("other[0]");
+  CHECK(CompileRun("other[0]").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // Set property with failed access check.
-  v8::Handle<v8::Value> result = CompileRun("other.x = new Object()");
-  CHECK(result->IsObject());
+  CHECK(CompileRun("other.x = new Object()").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // Set element with failed access check.
-  result = CompileRun("other[0] = new Object()");
-  CHECK(result->IsObject());
+  CHECK(CompileRun("other[0] = new Object()").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // Get property attribute with failed access check.
-  ExpectFalse("\'x\' in other");
+  CHECK(CompileRun("\'x\' in other").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // Get property attribute for element with failed access check.
-  ExpectFalse("0 in other");
+  CHECK(CompileRun("0 in other").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // Delete property.
-  ExpectFalse("delete other.x");
+  CHECK(CompileRun("delete other.x").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // Delete element.
   CHECK_EQ(false, global0->Delete(0));
@@ -16842,15 +16858,25 @@ TEST(GCInFailedAccessCheckCallback) {
            global0->SetAccessor(v8_str("x"), GetXValue, NULL, v8_str("x")));
 
   // Define JavaScript accessor.
-  ExpectUndefined("Object.prototype.__defineGetter__.call("
-                  "    other, \'x\', function() { return 42; })");
+  CHECK(CompileRun(
+            "Object.prototype.__defineGetter__.call("
+            "    other, \'x\', function() { return 42; })").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // LookupAccessor.
-  ExpectUndefined("Object.prototype.__lookupGetter__.call("
-                  "    other, \'x\')");
+  CHECK(CompileRun(
+            "Object.prototype.__lookupGetter__.call("
+            "    other, \'x\')").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   // HasOwnElement.
-  ExpectFalse("Object.prototype.hasOwnProperty.call(other, \'0\')");
+  CHECK(CompileRun(
+            "Object.prototype.hasOwnProperty.call("
+            "other, \'0\')").IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
 
   CHECK_EQ(false, global0->HasRealIndexedProperty(0));
   CHECK_EQ(false, global0->HasRealNamedProperty(v8_str("x")));