From 848b620bebd327c6895d0f5af51c8c44308fd337 Mon Sep 17 00:00:00 2001 From: verwaest Date: Thu, 11 Jun 2015 13:20:46 -0700 Subject: [PATCH] Reland of Use the LookupIterator in SetAccessor (patchset #1 id:1 of https://codereview.chromium.org/1175323004/) Reason: Didn't break anything Original issue's description: > 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 > > Committed: https://crrev.com/11dbd29de57b290ee8dac2a782a53f879beb416f > Cr-Commit-Position: refs/heads/master@{#28956} TBR=jkummerow@chromium.org,ishell@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=v8:4137 Review URL: https://codereview.chromium.org/1181813002 Cr-Commit-Position: refs/heads/master@{#28967} --- src/objects.cc | 83 ++++++++++++++++++------------------------------- test/cctest/test-api.cc | 54 +++++++++++++++++++++++--------- 2 files changed, 70 insertions(+), 67 deletions(-) diff --git a/src/objects.cc b/src/objects.cc index a497f55..edd7583 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -6465,75 +6465,52 @@ MaybeHandle JSObject::DefineAccessor(Handle object, MaybeHandle JSObject::SetAccessor(Handle object, Handle info) { Isolate* isolate = object->GetIsolate(); - Factory* factory = isolate->factory(); - Handle 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::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::cast(info->name())); if (name->IsString()) name = String::Flatten(Handle::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()); } diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 4f6de59..732e704 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -16783,6 +16783,8 @@ void FailedAccessCheckCallbackGC(Local target, v8::AccessType type, Local data) { CcTest::heap()->CollectAllGarbage(); + CcTest::isolate()->ThrowException( + v8::Exception::Error(v8_str("cross context"))); } @@ -16813,28 +16815,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 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)); @@ -16844,15 +16860,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"))); -- 2.7.4