Revert of Use the LookupIterator in SetAccessor (patchset #2 id:20001 of...

Revert of Use the LookupIterator in SetAccessor (patchset #2 id:20001 of https://codereview.chromium.org/1178673003/)

Reason for revert:
Blocks reverting of https://codereview.chromium.org/1175973002

Original issue's description:
> Use the LookupIterator in SetAccessor
>
> BUG=v8:4137
> LOG=n
>
> Committed: https://crrev.com/f93276bfe093f576595c5dcac69cf8f9163915d9
> Cr-Commit-Position: refs/heads/master@{#28955}

TBR=jkummerow@chromium.org,verwaest@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4137

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

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

src/objects.cc

@@ -6474,52 +6474,75 @@ 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;
-  LookupIterator::Configuration c = LookupIterator::HIDDEN_SKIP_INTERCEPTOR;
-  LookupIterator it = name->AsArrayIndex(&index)
-                          ? LookupIterator(isolate, object, index, c)
-                          : LookupIterator(object, name, c);
+  bool is_element = name->AsArrayIndex(&index);
 
-  // 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();
-  }
+  if (is_element) {
+    if (object->IsJSArray()) return factory->undefined_value();
 
-  CHECK(GetPropertyAttributes(&it).IsJust());
+    // 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;
 
-  // 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();
-  }
+#define TYPED_ARRAY_CASE(Type, type, TYPE, ctype, size)                        \
+      case EXTERNAL_##TYPE##_ELEMENTS:                                         \
+      case TYPE##_ELEMENTS:                                                    \
 
-  // Ignore accessors on typed arrays.
-  if (it.IsElement() && (object->HasFixedTypedArrayElements() ||
-                         object->HasExternalArrayElements())) {
-    return it.factory()->undefined_value();
-  }
+      TYPED_ARRAYS(TYPED_ARRAY_CASE)
+#undef TYPED_ARRAY_CASE
+        // Ignore getters and setters on pixel and external array
+        // elements.
+        return factory->undefined_value();
+
+      case DICTIONARY_ELEMENTS:
+        break;
+      case SLOPPY_ARGUMENTS_ELEMENTS:
+        UNIMPLEMENTED();
+        break;
+    }
 
-  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,8 +16781,6 @@ 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")));
 }
 
 
@@ -16813,42 +16811,28 @@ 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.
-  CHECK(CompileRun("other.x").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  ExpectUndefined("other.x");
 
   // Get element with failed access check.
-  CHECK(CompileRun("other[0]").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  ExpectUndefined("other[0]");
 
   // Set property with failed access check.
-  CHECK(CompileRun("other.x = new Object()").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  v8::Handle<v8::Value> result = CompileRun("other.x = new Object()");
+  CHECK(result->IsObject());
 
   // Set element with failed access check.
-  CHECK(CompileRun("other[0] = new Object()").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  result = CompileRun("other[0] = new Object()");
+  CHECK(result->IsObject());
 
   // Get property attribute with failed access check.
-  CHECK(CompileRun("\'x\' in other").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  ExpectFalse("\'x\' in other");
 
   // Get property attribute for element with failed access check.
-  CHECK(CompileRun("0 in other").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  ExpectFalse("0 in other");
 
   // Delete property.
-  CHECK(CompileRun("delete other.x").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  ExpectFalse("delete other.x");
 
   // Delete element.
   CHECK_EQ(false, global0->Delete(0));
@@ -16858,25 +16842,15 @@ TEST(GCInFailedAccessCheckCallback) {
            global0->SetAccessor(v8_str("x"), GetXValue, NULL, v8_str("x")));
 
   // Define JavaScript accessor.
-  CHECK(CompileRun(
-            "Object.prototype.__defineGetter__.call("
-            "    other, \'x\', function() { return 42; })").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  ExpectUndefined("Object.prototype.__defineGetter__.call("
+                  "    other, \'x\', function() { return 42; })");
 
   // LookupAccessor.
-  CHECK(CompileRun(
-            "Object.prototype.__lookupGetter__.call("
-            "    other, \'x\')").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  ExpectUndefined("Object.prototype.__lookupGetter__.call("
+                  "    other, \'x\')");
 
   // HasOwnElement.
-  CHECK(CompileRun(
-            "Object.prototype.hasOwnProperty.call("
-            "other, \'0\')").IsEmpty());
-  CHECK(try_catch.HasCaught());
-  try_catch.Reset();
+  ExpectFalse("Object.prototype.hasOwnProperty.call(other, \'0\')");
 
   CHECK_EQ(false, global0->HasRealIndexedProperty(0));
   CHECK_EQ(false, global0->HasRealNamedProperty(v8_str("x")));