From 6ff2a7736450c8b22c89892d83edd1fc41d0d60a Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Mon, 30 Jun 2014 13:48:57 +0000 Subject: [PATCH] Wrap InitializeProperty around SetOwnPropertyIgnoreAttributes and switch over uses This is a step in the direction of disentangling all uses of SetOwnPropertyIgnoreAttributes so we can provide a more specific implementation for those usecases, and reduce the capabilities of those clients, avoiding subtle bugs. InitializeProperty only supports adding properties to extensible objects that do not contain the property yet. JSGlobalProxies cannot have properties themselves, so are not supported either. BUG= R=rossberg@chromium.org Review URL: https://codereview.chromium.org/352813002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@22095 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/bootstrapper.cc | 43 ++++++++++++++------------------- src/factory.cc | 23 +++++++++++------- src/isolate.cc | 32 +++++++++++-------------- src/objects.cc | 45 ++++++++++++++++++++++++----------- src/objects.h | 9 ++++++- src/runtime.cc | 68 +++++++++++++++++++---------------------------------- 6 files changed, 108 insertions(+), 112 deletions(-) diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index c240869..a2d1388 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -379,8 +379,7 @@ static Handle InstallFunction(Handle target, } else { attributes = DONT_ENUM; } - JSObject::SetOwnPropertyIgnoreAttributes( - target, internalized_name, function, attributes).Check(); + JSObject::AddProperty(target, internalized_name, function, attributes); if (target->IsJSGlobalObject()) { function->shared()->set_instance_class_name(*internalized_name); } @@ -889,9 +888,8 @@ void Genesis::InitializeGlobal(Handle inner_global, Heap* heap = isolate->heap(); Handle object_name = factory->Object_string(); - JSObject::SetOwnPropertyIgnoreAttributes( - inner_global, object_name, - isolate->object_function(), DONT_ENUM).Check(); + JSObject::AddProperty( + inner_global, object_name, isolate->object_function(), DONT_ENUM); Handle global(native_context()->global_object()); @@ -1090,8 +1088,7 @@ void Genesis::InitializeGlobal(Handle inner_global, cons->SetInstanceClassName(*name); Handle json_object = factory->NewJSObject(cons, TENURED); ASSERT(json_object->IsJSObject()); - JSObject::SetOwnPropertyIgnoreAttributes( - global, name, json_object, DONT_ENUM).Check(); + JSObject::AddProperty(global, name, json_object, DONT_ENUM); native_context()->set_json_object(*json_object); } @@ -1156,14 +1153,14 @@ void Genesis::InitializeGlobal(Handle inner_global, native_context()->set_sloppy_arguments_boilerplate(*result); // Note: length must be added as the first property and // callee must be added as the second property. - JSObject::SetOwnPropertyIgnoreAttributes( + JSObject::AddProperty( result, factory->length_string(), factory->undefined_value(), DONT_ENUM, - Object::FORCE_TAGGED, FORCE_FIELD).Check(); - JSObject::SetOwnPropertyIgnoreAttributes( + Object::FORCE_TAGGED, FORCE_FIELD); + JSObject::AddProperty( result, factory->callee_string(), factory->undefined_value(), DONT_ENUM, - Object::FORCE_TAGGED, FORCE_FIELD).Check(); + Object::FORCE_TAGGED, FORCE_FIELD); #ifdef DEBUG LookupResult lookup(isolate); @@ -1262,11 +1259,6 @@ void Genesis::InitializeGlobal(Handle inner_global, Handle result = factory->NewJSObjectFromMap(map); native_context()->set_strict_arguments_boilerplate(*result); - // Add length property only for strict mode boilerplate. - JSObject::SetOwnPropertyIgnoreAttributes( - result, factory->length_string(), - factory->undefined_value(), DONT_ENUM).Check(); - #ifdef DEBUG LookupResult lookup(isolate); result->LookupOwn(factory->length_string(), &lookup); @@ -1274,6 +1266,10 @@ void Genesis::InitializeGlobal(Handle inner_global, ASSERT(lookup.GetFieldIndex().property_index() == Heap::kArgumentsLengthIndex); + Handle length_value = Object::GetProperty( + result, factory->length_string()).ToHandleChecked(); + ASSERT_EQ(heap->undefined_value(), *length_value); + ASSERT(result->map()->inobject_properties() > Heap::kArgumentsLengthIndex); // Check the state of the object. @@ -1736,12 +1732,10 @@ bool Genesis::InstallNatives() { Handle global_string = factory()->InternalizeOneByteString(STATIC_ASCII_VECTOR("global")); Handle global_obj(native_context()->global_object(), isolate()); - JSObject::SetOwnPropertyIgnoreAttributes( - builtins, global_string, global_obj, attributes).Check(); + JSObject::AddProperty(builtins, global_string, global_obj, attributes); Handle builtins_string = factory()->InternalizeOneByteString(STATIC_ASCII_VECTOR("builtins")); - JSObject::SetOwnPropertyIgnoreAttributes( - builtins, builtins_string, builtins, attributes).Check(); + JSObject::AddProperty(builtins, builtins_string, builtins, attributes); // Set up the reference from the global object to the builtins object. JSGlobalObject::cast(native_context()->global_object())-> @@ -2454,16 +2448,14 @@ void Genesis::TransferNamedProperties(Handle from, ASSERT(!descs->GetDetails(i).representation().IsDouble()); Handle value = Handle(from->RawFastPropertyAt(index), isolate()); - JSObject::SetOwnPropertyIgnoreAttributes( - to, key, value, details.attributes()).Check(); + JSObject::AddProperty(to, key, value, details.attributes()); break; } case CONSTANT: { HandleScope inner(isolate()); Handle key = Handle(descs->GetKey(i)); Handle constant(descs->GetConstant(i), isolate()); - JSObject::SetOwnPropertyIgnoreAttributes( - to, key, constant, details.attributes()).Check(); + JSObject::AddProperty(to, key, constant, details.attributes()); break; } case CALLBACKS: { @@ -2513,8 +2505,7 @@ void Genesis::TransferNamedProperties(Handle from, isolate()); } PropertyDetails details = properties->DetailsAt(i); - JSObject::SetOwnPropertyIgnoreAttributes( - to, key, value, details.attributes()).Check(); + JSObject::AddProperty(to, key, value, details.attributes()); } } } diff --git a/src/factory.cc b/src/factory.cc index 29e396b..0f56fca 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -1333,10 +1333,7 @@ Handle Factory::NewFunctionPrototype(Handle function) { Handle prototype = NewJSObjectFromMap(new_map); if (!function->shared()->is_generator()) { - JSObject::SetOwnPropertyIgnoreAttributes(prototype, - constructor_string(), - function, - DONT_ENUM).Assert(); + JSObject::AddProperty(prototype, constructor_string(), function, DONT_ENUM); } return prototype; @@ -2159,11 +2156,19 @@ Handle Factory::CreateApiFunction( return result; } - JSObject::SetOwnPropertyIgnoreAttributes( - handle(JSObject::cast(result->prototype())), - constructor_string(), - result, - DONT_ENUM).Assert(); + if (prototype->IsTheHole()) { +#ifdef DEBUG + LookupIterator it(handle(JSObject::cast(result->prototype())), + constructor_string(), + LookupIterator::CHECK_OWN_REAL); + MaybeHandle maybe_prop = Object::GetProperty(&it); + ASSERT(it.IsFound()); + ASSERT(maybe_prop.ToHandleChecked().is_identical_to(result)); +#endif + } else { + JSObject::AddProperty(handle(JSObject::cast(result->prototype())), + constructor_string(), result, DONT_ENUM); + } // Down from here is only valid for API functions that can be used as a // constructor (don't set the "remove prototype" flag). diff --git a/src/isolate.cc b/src/isolate.cc index 10b1db1..05f8792 100644 --- a/src/isolate.cc +++ b/src/isolate.cc @@ -494,31 +494,29 @@ Handle Isolate::CaptureCurrentStackTrace( // tag. column_offset += script->column_offset()->value(); } - JSObject::SetOwnPropertyIgnoreAttributes( + JSObject::AddProperty( stack_frame, column_key, - Handle(Smi::FromInt(column_offset + 1), this), NONE).Check(); + handle(Smi::FromInt(column_offset + 1), this), NONE); } - JSObject::SetOwnPropertyIgnoreAttributes( + JSObject::AddProperty( stack_frame, line_key, - Handle(Smi::FromInt(line_number + 1), this), NONE).Check(); + handle(Smi::FromInt(line_number + 1), this), NONE); } if (options & StackTrace::kScriptId) { - Handle script_id(script->id(), this); - JSObject::SetOwnPropertyIgnoreAttributes( - stack_frame, script_id_key, script_id, NONE).Check(); + JSObject::AddProperty( + stack_frame, script_id_key, handle(script->id(), this), NONE); } if (options & StackTrace::kScriptName) { - Handle script_name(script->name(), this); - JSObject::SetOwnPropertyIgnoreAttributes( - stack_frame, script_name_key, script_name, NONE).Check(); + JSObject::AddProperty( + stack_frame, script_name_key, handle(script->name(), this), NONE); } if (options & StackTrace::kScriptNameOrSourceURL) { Handle result = Script::GetNameOrSourceURL(script); - JSObject::SetOwnPropertyIgnoreAttributes( - stack_frame, script_name_or_source_url_key, result, NONE).Check(); + JSObject::AddProperty( + stack_frame, script_name_or_source_url_key, result, NONE); } if (options & StackTrace::kFunctionName) { @@ -526,23 +524,21 @@ Handle Isolate::CaptureCurrentStackTrace( if (!fun_name->BooleanValue()) { fun_name = Handle(fun->shared()->inferred_name(), this); } - JSObject::SetOwnPropertyIgnoreAttributes( - stack_frame, function_key, fun_name, NONE).Check(); + JSObject::AddProperty(stack_frame, function_key, fun_name, NONE); } if (options & StackTrace::kIsEval) { Handle is_eval = script->compilation_type() == Script::COMPILATION_TYPE_EVAL ? factory()->true_value() : factory()->false_value(); - JSObject::SetOwnPropertyIgnoreAttributes( - stack_frame, eval_key, is_eval, NONE).Check(); + JSObject::AddProperty(stack_frame, eval_key, is_eval, NONE); } if (options & StackTrace::kIsConstructor) { Handle is_constructor = (frames[i].is_constructor()) ? factory()->true_value() : factory()->false_value(); - JSObject::SetOwnPropertyIgnoreAttributes( - stack_frame, constructor_key, is_constructor, NONE).Check(); + JSObject::AddProperty( + stack_frame, constructor_key, is_constructor, NONE); } FixedArray::cast(stack_trace->elements())->set(frames_seen, *stack_frame); diff --git a/src/objects.cc b/src/objects.cc index c0dde8d..176828e 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -1873,7 +1873,7 @@ void JSObject::AddSlowProperty(Handle object, } -MaybeHandle JSObject::AddProperty( +MaybeHandle JSObject::AddPropertyInternal( Handle object, Handle name, Handle value, @@ -3926,10 +3926,10 @@ MaybeHandle JSObject::SetPropertyUsingTransition( PropertyDetails details = descriptors->GetDetails(descriptor); if (details.type() == CALLBACKS || attributes != details.attributes()) { - // AddProperty will either normalize the object, or create a new fast copy - // of the map. If we get a fast copy of the map, all field representations - // will be tagged since the transition is omitted. - return JSObject::AddProperty( + // AddPropertyInternal will either normalize the object, or create a new + // fast copy of the map. If we get a fast copy of the map, all field + // representations will be tagged since the transition is omitted. + return JSObject::AddPropertyInternal( object, name, value, attributes, SLOPPY, JSReceiver::CERTAINLY_NOT_STORE_FROM_KEYED, JSReceiver::OMIT_EXTENSIBILITY_CHECK, @@ -4093,7 +4093,7 @@ MaybeHandle JSObject::SetPropertyForResult( if (!lookup->IsFound()) { // Neither properties nor transitions found. - return AddProperty( + return AddPropertyInternal( object, name, value, attributes, strict_mode, store_mode); } @@ -4173,6 +4173,28 @@ MaybeHandle JSObject::SetPropertyForResult( } +void JSObject::AddProperty( + Handle object, + Handle name, + Handle value, + PropertyAttributes attributes, + ValueType value_type, + StoreMode store_mode) { +#ifdef DEBUG + uint32_t index; + ASSERT(!object->IsJSProxy()); + ASSERT(!name->AsArrayIndex(&index)); + LookupIterator it(object, name, LookupIterator::CHECK_OWN_REAL); + GetPropertyAttributes(&it); + ASSERT(!it.IsFound()); + ASSERT(object->map()->is_extensible()); +#endif + SetOwnPropertyIgnoreAttributes( + object, name, value, attributes, value_type, store_mode, + OMIT_EXTENSIBILITY_CHECK).Check(); +} + + // Set a real own property, even if it is READ_ONLY. If the property is not // present, add it with attributes NONE. This code is an exact clone of // SetProperty, with the check for IsReadOnly and the check for a @@ -4228,7 +4250,7 @@ MaybeHandle JSObject::SetOwnPropertyIgnoreAttributes( TransitionFlag flag = lookup.IsFound() ? OMIT_TRANSITION : INSERT_TRANSITION; // Neither properties nor transitions found. - return AddProperty(object, name, value, attributes, SLOPPY, + return AddPropertyInternal(object, name, value, attributes, SLOPPY, store_from_keyed, extensibility_check, value_type, mode, flag); } @@ -5154,13 +5176,8 @@ Handle JSObject::GetOrCreateHiddenPropertiesHashtable( } JSObject::SetOwnPropertyIgnoreAttributes( - object, - isolate->factory()->hidden_string(), - hashtable, - DONT_ENUM, - OPTIMAL_REPRESENTATION, - ALLOW_AS_CONSTANT, - OMIT_EXTENSIBILITY_CHECK).Assert(); + object, isolate->factory()->hidden_string(), + hashtable, DONT_ENUM).Assert(); return hashtable; } diff --git a/src/objects.h b/src/objects.h index ef3cd34..764cdd9 100644 --- a/src/objects.h +++ b/src/objects.h @@ -2153,6 +2153,13 @@ class JSObject: public JSReceiver { StoreFromKeyed store_mode = MAY_BE_STORE_FROM_KEYED, ExecutableAccessorInfoHandling handling = DEFAULT_HANDLING); + static void AddProperty(Handle object, + Handle key, + Handle value, + PropertyAttributes attributes, + ValueType value_type = OPTIMAL_REPRESENTATION, + StoreMode mode = ALLOW_AS_CONSTANT); + // Extend the receiver with a single fast property appeared first in the // passed map. This also extends the property backing store if necessary. static void AllocateStorageForMap(Handle object, Handle map); @@ -2750,7 +2757,7 @@ class JSObject: public JSReceiver { StrictMode strict_mode); // Add a property to an object. - MUST_USE_RESULT static MaybeHandle AddProperty( + MUST_USE_RESULT static MaybeHandle AddPropertyInternal( Handle object, Handle name, Handle value, diff --git a/src/runtime.cc b/src/runtime.cc index 5efaaa1..888f665 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -303,8 +303,7 @@ MUST_USE_RESULT static MaybeHandle CreateObjectLiteralBoilerplate( const char* str = DoubleToCString(num, buffer); Handle name = isolate->factory()->NewStringFromAsciiChecked(str); maybe_result = JSObject::SetOwnPropertyIgnoreAttributes( - boilerplate, name, value, NONE, - value_type, mode); + boilerplate, name, value, NONE, value_type, mode); } // If setting the property on the boilerplate throws an // exception, the exception is converted to an empty handle in @@ -2396,10 +2395,7 @@ RUNTIME_FUNCTION(Runtime_InitializeConstGlobal) { if (!lookup.IsFound()) { HandleScope handle_scope(isolate); Handle global(isolate->context()->global_object()); - RETURN_FAILURE_ON_EXCEPTION( - isolate, - JSObject::SetOwnPropertyIgnoreAttributes(global, name, value, - attributes)); + JSObject::AddProperty(global, name, value, attributes); return *value; } @@ -8133,8 +8129,8 @@ RUNTIME_FUNCTION(Runtime_FunctionBindArguments) { static_cast(DONT_DELETE | DONT_ENUM | READ_ONLY); RETURN_FAILURE_ON_EXCEPTION( isolate, - JSObject::SetOwnPropertyIgnoreAttributes(bound_function, length_string, - new_length, attr)); + JSObject::SetOwnPropertyIgnoreAttributes( + bound_function, length_string, new_length, attr)); return *bound_function; } @@ -13809,18 +13805,10 @@ RUNTIME_FUNCTION(Runtime_GetLanguageTagVariants) { } Handle result = factory->NewJSObject(isolate->object_function()); - RETURN_FAILURE_ON_EXCEPTION(isolate, - JSObject::SetOwnPropertyIgnoreAttributes( - result, - maximized, - factory->NewStringFromAsciiChecked(base_max_locale), - NONE)); - RETURN_FAILURE_ON_EXCEPTION(isolate, - JSObject::SetOwnPropertyIgnoreAttributes( - result, - base, - factory->NewStringFromAsciiChecked(base_locale), - NONE)); + Handle value = factory->NewStringFromAsciiChecked(base_max_locale); + JSObject::AddProperty(result, maximized, value, NONE); + value = factory->NewStringFromAsciiChecked(base_locale); + JSObject::AddProperty(result, base, value, NONE); output->set(i, *result); } @@ -13937,12 +13925,10 @@ RUNTIME_FUNCTION(Runtime_CreateDateTimeFormat) { local_object->SetInternalField(0, reinterpret_cast(date_format)); - RETURN_FAILURE_ON_EXCEPTION(isolate, - JSObject::SetOwnPropertyIgnoreAttributes( - local_object, - isolate->factory()->NewStringFromStaticAscii("dateFormat"), - isolate->factory()->NewStringFromStaticAscii("valid"), - NONE)); + Factory* factory = isolate->factory(); + Handle key = factory->NewStringFromStaticAscii("dateFormat"); + Handle value = factory->NewStringFromStaticAscii("valid"); + JSObject::AddProperty(local_object, key, value, NONE); // Make object handle weak so we can delete the data format once GC kicks in. Handle wrapper = isolate->global_handles()->Create(*local_object); @@ -14036,12 +14022,10 @@ RUNTIME_FUNCTION(Runtime_CreateNumberFormat) { local_object->SetInternalField(0, reinterpret_cast(number_format)); - RETURN_FAILURE_ON_EXCEPTION(isolate, - JSObject::SetOwnPropertyIgnoreAttributes( - local_object, - isolate->factory()->NewStringFromStaticAscii("numberFormat"), - isolate->factory()->NewStringFromStaticAscii("valid"), - NONE)); + Factory* factory = isolate->factory(); + Handle key = factory->NewStringFromStaticAscii("numberFormat"); + Handle value = factory->NewStringFromStaticAscii("valid"); + JSObject::AddProperty(local_object, key, value, NONE); Handle wrapper = isolate->global_handles()->Create(*local_object); GlobalHandles::MakeWeak(wrapper.location(), @@ -14144,12 +14128,10 @@ RUNTIME_FUNCTION(Runtime_CreateCollator) { local_object->SetInternalField(0, reinterpret_cast(collator)); - RETURN_FAILURE_ON_EXCEPTION(isolate, - JSObject::SetOwnPropertyIgnoreAttributes( - local_object, - isolate->factory()->NewStringFromStaticAscii("collator"), - isolate->factory()->NewStringFromStaticAscii("valid"), - NONE)); + Factory* factory = isolate->factory(); + Handle key = factory->NewStringFromStaticAscii("collator"); + Handle value = factory->NewStringFromStaticAscii("valid"); + JSObject::AddProperty(local_object, key, value, NONE); Handle wrapper = isolate->global_handles()->Create(*local_object); GlobalHandles::MakeWeak(wrapper.location(), @@ -14250,12 +14232,10 @@ RUNTIME_FUNCTION(Runtime_CreateBreakIterator) { // Make sure that the pointer to adopted text is NULL. local_object->SetInternalField(1, reinterpret_cast(NULL)); - RETURN_FAILURE_ON_EXCEPTION(isolate, - JSObject::SetOwnPropertyIgnoreAttributes( - local_object, - isolate->factory()->NewStringFromStaticAscii("breakIterator"), - isolate->factory()->NewStringFromStaticAscii("valid"), - NONE)); + Factory* factory = isolate->factory(); + Handle key = factory->NewStringFromStaticAscii("breakIterator"); + Handle value = factory->NewStringFromStaticAscii("valid"); + JSObject::AddProperty(local_object, key, value, NONE); // Make object handle weak so we can delete the break iterator once GC kicks // in. -- 2.7.4