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

@@ -379,8 +379,7 @@ static Handle<JSFunction> InstallFunction(Handle<JSObject> target,
   } else {
     attributes = DONT_ENUM;
   }
-  JSObject::SetOwnPropertyIgnoreAttributes(
+  JSObject::AddProperty(target, internalized_name, function, attributes);
-      target, internalized_name, function, attributes).Check();
   if (target->IsJSGlobalObject()) {
     function->shared()->set_instance_class_name(*internalized_name);
   }
@@ -889,9 +888,8 @@ void Genesis::InitializeGlobal(Handle<GlobalObject> inner_global,
   Heap* heap = isolate->heap();
 
   Handle<String> object_name = factory->Object_string();
-  JSObject::SetOwnPropertyIgnoreAttributes(
+  JSObject::AddProperty(
-      inner_global, object_name,
+      inner_global, object_name, isolate->object_function(), DONT_ENUM);
-      isolate->object_function(), DONT_ENUM).Check();
 
   Handle<JSObject> global(native_context()->global_object());
 
@@ -1090,8 +1088,7 @@ void Genesis::InitializeGlobal(Handle<GlobalObject> inner_global,
     cons->SetInstanceClassName(*name);
     Handle<JSObject> json_object = factory->NewJSObject(cons, TENURED);
     ASSERT(json_object->IsJSObject());
-    JSObject::SetOwnPropertyIgnoreAttributes(
+    JSObject::AddProperty(global, name, json_object, DONT_ENUM);
-        global, name, json_object, DONT_ENUM).Check();
     native_context()->set_json_object(*json_object);
   }
 
@@ -1156,14 +1153,14 @@ void Genesis::InitializeGlobal(Handle<GlobalObject> 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();
+        Object::FORCE_TAGGED, FORCE_FIELD);
-    JSObject::SetOwnPropertyIgnoreAttributes(
+    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<GlobalObject> inner_global,
     Handle<JSObject> 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<GlobalObject> inner_global,
     ASSERT(lookup.GetFieldIndex().property_index() ==
            Heap::kArgumentsLengthIndex);
 
+    Handle<Object> 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<String> global_string =
       factory()->InternalizeOneByteString(STATIC_ASCII_VECTOR("global"));
   Handle<Object> global_obj(native_context()->global_object(), isolate());
-  JSObject::SetOwnPropertyIgnoreAttributes(
+  JSObject::AddProperty(builtins, global_string, global_obj, attributes);
-      builtins, global_string, global_obj, attributes).Check();
   Handle<String> builtins_string =
       factory()->InternalizeOneByteString(STATIC_ASCII_VECTOR("builtins"));
-  JSObject::SetOwnPropertyIgnoreAttributes(
+  JSObject::AddProperty(builtins, builtins_string, builtins, attributes);
-      builtins, builtins_string, builtins, attributes).Check();
 
   // 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<JSObject> from,
           ASSERT(!descs->GetDetails(i).representation().IsDouble());
           Handle<Object> value = Handle<Object>(from->RawFastPropertyAt(index),
                                                 isolate());
-          JSObject::SetOwnPropertyIgnoreAttributes(
+          JSObject::AddProperty(to, key, value, details.attributes());
-              to, key, value, details.attributes()).Check();
           break;
         }
         case CONSTANT: {
           HandleScope inner(isolate());
           Handle<Name> key = Handle<Name>(descs->GetKey(i));
           Handle<Object> constant(descs->GetConstant(i), isolate());
-          JSObject::SetOwnPropertyIgnoreAttributes(
+          JSObject::AddProperty(to, key, constant, details.attributes());
-              to, key, constant, details.attributes()).Check();
           break;
         }
         case CALLBACKS: {
@@ -2513,8 +2505,7 @@ void Genesis::TransferNamedProperties(Handle<JSObject> from,
                                  isolate());
         }
         PropertyDetails details = properties->DetailsAt(i);
-        JSObject::SetOwnPropertyIgnoreAttributes(
+        JSObject::AddProperty(to, key, value, details.attributes());
-            to, key, value, details.attributes()).Check();
       }
     }
   }

src/factory.cc

@@ -1333,10 +1333,7 @@ Handle<JSObject> Factory::NewFunctionPrototype(Handle<JSFunction> function) {
   Handle<JSObject> prototype = NewJSObjectFromMap(new_map);
 
   if (!function->shared()->is_generator()) {
-    JSObject::SetOwnPropertyIgnoreAttributes(prototype,
+    JSObject::AddProperty(prototype, constructor_string(), function, DONT_ENUM);
-                                             constructor_string(),
-                                             function,
-                                             DONT_ENUM).Assert();
   }
 
   return prototype;
@@ -2159,11 +2156,19 @@ Handle<JSFunction> Factory::CreateApiFunction(
     return result;
   }
 
-  JSObject::SetOwnPropertyIgnoreAttributes(
+  if (prototype->IsTheHole()) {
-      handle(JSObject::cast(result->prototype())),
+#ifdef DEBUG
-      constructor_string(),
+    LookupIterator it(handle(JSObject::cast(result->prototype())),
-      result,
+                      constructor_string(),
-      DONT_ENUM).Assert();
+                      LookupIterator::CHECK_OWN_REAL);
+    MaybeHandle<Object> 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).

src/isolate.cc

@@ -494,31 +494,29 @@ Handle<JSArray> Isolate::CaptureCurrentStackTrace(
             // tag.
             column_offset += script->column_offset()->value();
           }
-          JSObject::SetOwnPropertyIgnoreAttributes(
+          JSObject::AddProperty(
               stack_frame, column_key,
-              Handle<Smi>(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>(Smi::FromInt(line_number + 1), this), NONE).Check();
+            handle(Smi::FromInt(line_number + 1), this), NONE);
       }
 
       if (options & StackTrace::kScriptId) {
-        Handle<Smi> script_id(script->id(), this);
+        JSObject::AddProperty(
-        JSObject::SetOwnPropertyIgnoreAttributes(
+            stack_frame, script_id_key, handle(script->id(), this), NONE);
-            stack_frame, script_id_key, script_id, NONE).Check();
       }
 
       if (options & StackTrace::kScriptName) {
-        Handle<Object> script_name(script->name(), this);
+        JSObject::AddProperty(
-        JSObject::SetOwnPropertyIgnoreAttributes(
+            stack_frame, script_name_key, handle(script->name(), this), NONE);
-            stack_frame, script_name_key, script_name, NONE).Check();
       }
 
       if (options & StackTrace::kScriptNameOrSourceURL) {
         Handle<Object> result = Script::GetNameOrSourceURL(script);
-        JSObject::SetOwnPropertyIgnoreAttributes(
+        JSObject::AddProperty(
-            stack_frame, script_name_or_source_url_key, result, NONE).Check();
+            stack_frame, script_name_or_source_url_key, result, NONE);
       }
 
       if (options & StackTrace::kFunctionName) {
@@ -526,23 +524,21 @@ Handle<JSArray> Isolate::CaptureCurrentStackTrace(
         if (!fun_name->BooleanValue()) {
           fun_name = Handle<Object>(fun->shared()->inferred_name(), this);
         }
-        JSObject::SetOwnPropertyIgnoreAttributes(
+        JSObject::AddProperty(stack_frame, function_key, fun_name, NONE);
-            stack_frame, function_key, fun_name, NONE).Check();
       }
 
       if (options & StackTrace::kIsEval) {
         Handle<Object> is_eval =
             script->compilation_type() == Script::COMPILATION_TYPE_EVAL ?
                 factory()->true_value() : factory()->false_value();
-        JSObject::SetOwnPropertyIgnoreAttributes(
+        JSObject::AddProperty(stack_frame, eval_key, is_eval, NONE);
-            stack_frame, eval_key, is_eval, NONE).Check();
       }
 
       if (options & StackTrace::kIsConstructor) {
         Handle<Object> is_constructor = (frames[i].is_constructor()) ?
             factory()->true_value() : factory()->false_value();
-        JSObject::SetOwnPropertyIgnoreAttributes(
+        JSObject::AddProperty(
-            stack_frame, constructor_key, is_constructor, NONE).Check();
+            stack_frame, constructor_key, is_constructor, NONE);
       }
 
       FixedArray::cast(stack_trace->elements())->set(frames_seen, *stack_frame);

src/objects.cc

@@ -1873,7 +1873,7 @@ void JSObject::AddSlowProperty(Handle<JSObject> object,
 }
 
 
-MaybeHandle<Object> JSObject::AddProperty(
+MaybeHandle<Object> JSObject::AddPropertyInternal(
     Handle<JSObject> object,
     Handle<Name> name,
     Handle<Object> value,
@@ -3926,10 +3926,10 @@ MaybeHandle<Object> 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
+    // AddPropertyInternal will either normalize the object, or create a new
-    // of the map. If we get a fast copy of the map, all field representations
+    // fast copy of the map. If we get a fast copy of the map, all field
-    // will be tagged since the transition is omitted.
+    // representations will be tagged since the transition is omitted.
-    return JSObject::AddProperty(
+    return JSObject::AddPropertyInternal(
         object, name, value, attributes, SLOPPY,
         JSReceiver::CERTAINLY_NOT_STORE_FROM_KEYED,
         JSReceiver::OMIT_EXTENSIBILITY_CHECK,
@@ -4093,7 +4093,7 @@ MaybeHandle<Object> 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<Object> JSObject::SetPropertyForResult(
 }
 
 
+void JSObject::AddProperty(
+    Handle<JSObject> object,
+    Handle<Name> name,
+    Handle<Object> 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<Object> 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<ObjectHashTable> JSObject::GetOrCreateHiddenPropertiesHashtable(
   }
 
   JSObject::SetOwnPropertyIgnoreAttributes(
-      object,
+      object, isolate->factory()->hidden_string(),
-      isolate->factory()->hidden_string(),
+      hashtable, DONT_ENUM).Assert();
-      hashtable,
-      DONT_ENUM,
-      OPTIMAL_REPRESENTATION,
-      ALLOW_AS_CONSTANT,
-      OMIT_EXTENSIBILITY_CHECK).Assert();
 
   return hashtable;
 }

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<JSObject> object,
+                          Handle<Name> key,
+                          Handle<Object> 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<JSObject> object, Handle<Map> map);
@@ -2750,7 +2757,7 @@ class JSObject: public JSReceiver {
       StrictMode strict_mode);
 
   // Add a property to an object.
-  MUST_USE_RESULT static MaybeHandle<Object> AddProperty(
+  MUST_USE_RESULT static MaybeHandle<Object> AddPropertyInternal(
       Handle<JSObject> object,
       Handle<Name> name,
       Handle<Object> value,

src/runtime.cc

@@ -303,8 +303,7 @@ MUST_USE_RESULT static MaybeHandle<Object> CreateObjectLiteralBoilerplate(
       const char* str = DoubleToCString(num, buffer);
       Handle<String> name = isolate->factory()->NewStringFromAsciiChecked(str);
       maybe_result = JSObject::SetOwnPropertyIgnoreAttributes(
-          boilerplate, name, value, NONE,
+          boilerplate, name, value, NONE, value_type, mode);
-          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<GlobalObject> global(isolate->context()->global_object());
-    RETURN_FAILURE_ON_EXCEPTION(
+    JSObject::AddProperty(global, name, value, attributes);
-        isolate,
-        JSObject::SetOwnPropertyIgnoreAttributes(global, name, value,
-                                                 attributes));
     return *value;
   }
 
@@ -8133,8 +8129,8 @@ RUNTIME_FUNCTION(Runtime_FunctionBindArguments) {
       static_cast<PropertyAttributes>(DONT_DELETE | DONT_ENUM | READ_ONLY);
   RETURN_FAILURE_ON_EXCEPTION(
       isolate,
-      JSObject::SetOwnPropertyIgnoreAttributes(bound_function, length_string,
+      JSObject::SetOwnPropertyIgnoreAttributes(
-                                               new_length, attr));
+          bound_function, length_string, new_length, attr));
   return *bound_function;
 }
 
@@ -13809,18 +13805,10 @@ RUNTIME_FUNCTION(Runtime_GetLanguageTagVariants) {
     }
 
     Handle<JSObject> result = factory->NewJSObject(isolate->object_function());
-    RETURN_FAILURE_ON_EXCEPTION(isolate,
+    Handle<String> value = factory->NewStringFromAsciiChecked(base_max_locale);
-        JSObject::SetOwnPropertyIgnoreAttributes(
+    JSObject::AddProperty(result, maximized, value, NONE);
-            result,
+    value = factory->NewStringFromAsciiChecked(base_locale);
-            maximized,
+    JSObject::AddProperty(result, base, value, NONE);
-            factory->NewStringFromAsciiChecked(base_max_locale),
-            NONE));
-    RETURN_FAILURE_ON_EXCEPTION(isolate,
-        JSObject::SetOwnPropertyIgnoreAttributes(
-            result,
-            base,
-            factory->NewStringFromAsciiChecked(base_locale),
-            NONE));
     output->set(i, *result);
   }
 
@@ -13937,12 +13925,10 @@ RUNTIME_FUNCTION(Runtime_CreateDateTimeFormat) {
 
   local_object->SetInternalField(0, reinterpret_cast<Smi*>(date_format));
 
-  RETURN_FAILURE_ON_EXCEPTION(isolate,
+  Factory* factory = isolate->factory();
-      JSObject::SetOwnPropertyIgnoreAttributes(
+  Handle<String> key = factory->NewStringFromStaticAscii("dateFormat");
-          local_object,
+  Handle<String> value = factory->NewStringFromStaticAscii("valid");
-          isolate->factory()->NewStringFromStaticAscii("dateFormat"),
+  JSObject::AddProperty(local_object, key, value, NONE);
-          isolate->factory()->NewStringFromStaticAscii("valid"),
-          NONE));
 
   // Make object handle weak so we can delete the data format once GC kicks in.
   Handle<Object> wrapper = isolate->global_handles()->Create(*local_object);
@@ -14036,12 +14022,10 @@ RUNTIME_FUNCTION(Runtime_CreateNumberFormat) {
 
   local_object->SetInternalField(0, reinterpret_cast<Smi*>(number_format));
 
-  RETURN_FAILURE_ON_EXCEPTION(isolate,
+  Factory* factory = isolate->factory();
-      JSObject::SetOwnPropertyIgnoreAttributes(
+  Handle<String> key = factory->NewStringFromStaticAscii("numberFormat");
-          local_object,
+  Handle<String> value = factory->NewStringFromStaticAscii("valid");
-          isolate->factory()->NewStringFromStaticAscii("numberFormat"),
+  JSObject::AddProperty(local_object, key, value, NONE);
-          isolate->factory()->NewStringFromStaticAscii("valid"),
-          NONE));
 
   Handle<Object> 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<Smi*>(collator));
 
-  RETURN_FAILURE_ON_EXCEPTION(isolate,
+  Factory* factory = isolate->factory();
-      JSObject::SetOwnPropertyIgnoreAttributes(
+  Handle<String> key = factory->NewStringFromStaticAscii("collator");
-          local_object,
+  Handle<String> value = factory->NewStringFromStaticAscii("valid");
-          isolate->factory()->NewStringFromStaticAscii("collator"),
+  JSObject::AddProperty(local_object, key, value, NONE);
-          isolate->factory()->NewStringFromStaticAscii("valid"),
-          NONE));
 
   Handle<Object> 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<Smi*>(NULL));
 
-  RETURN_FAILURE_ON_EXCEPTION(isolate,
+  Factory* factory = isolate->factory();
-      JSObject::SetOwnPropertyIgnoreAttributes(
+  Handle<String> key = factory->NewStringFromStaticAscii("breakIterator");
-          local_object,
+  Handle<String> value = factory->NewStringFromStaticAscii("valid");
-          isolate->factory()->NewStringFromStaticAscii("breakIterator"),
+  JSObject::AddProperty(local_object, key, value, NONE);
-          isolate->factory()->NewStringFromStaticAscii("valid"),
-          NONE));
 
   // Make object handle weak so we can delete the break iterator once GC kicks
   // in.