Introduce DefineOwnPropertyIgnoreAttributes and make it call SetPropertyWithInterceptor.

verwaest · Commit bot · commit fa3e6c0850be · 2015-06-12T14:14:12.000Z
Otherwise using Object.defineProperty with window.localStorage will not actually store the value into the database but on the object itself. BUG=v8:4137 LOG=n Review URL: https://codereview.chromium.org/1180073002 Cr-Commit-Position: refs/heads/master@{#29002}
diff --git a/src/api.cc b/src/api.cc
@@ -3638,7 +3638,8 @@ static i::MaybeHandle<i::Object> DefineObjectProperty(
     name = i::Handle<i::String>::cast(converted);
   }
 
-  return i::JSObject::DefinePropertyOrElement(js_object, name, value, attrs);
+  return i::JSObject::DefinePropertyOrElementIgnoreAttributes(js_object, name,
+                                                              value, attrs);
 }
 
 
diff --git a/src/json-parser.h b/src/json-parser.h
@@ -434,7 +434,8 @@ Handle<Object> JsonParser<seq_one_byte>::ParseJsonObject() {
       // Commit the intermediate state to the object and stop transitioning.
       CommitStateToJsonObject(json_object, map, &properties);
 
-      JSObject::DefinePropertyOrElement(json_object, key, value).Check();
+      JSObject::DefinePropertyOrElementIgnoreAttributes(json_object, key, value)
+          .Check();
     } while (transitioning && MatchSkipWhiteSpace(','));
 
     // If we transitioned until the very end, transition the map now.
@@ -470,7 +471,8 @@ Handle<Object> JsonParser<seq_one_byte>::ParseJsonObject() {
         value = ParseJsonValue();
         if (value.is_null()) return ReportUnexpectedCharacter();
 
-        JSObject::DefinePropertyOrElement(json_object, key, value).Check();
+        JSObject::DefinePropertyOrElementIgnoreAttributes(json_object, key,
+                                                          value).Check();
       }
     }
 
diff --git a/src/objects-inl.h b/src/objects-inl.h
@@ -1197,22 +1197,6 @@ MaybeHandle<Object> Object::GetProperty(Isolate* isolate,
 }
 
 
-MaybeHandle<Object> JSObject::DefinePropertyOrElement(
-    Handle<JSObject> object, Handle<Name> name, Handle<Object> value,
-    PropertyAttributes attributes, ExecutableAccessorInfoHandling handling) {
-  uint32_t index;
-  if (name->AsArrayIndex(&index)) {
-    return SetOwnElementIgnoreAttributes(object, index, value, attributes,
-                                         handling);
-  }
-
-  // TODO(verwaest): Is this necessary?
-  if (name->IsString()) name = String::Flatten(Handle<String>::cast(name));
-  return SetOwnPropertyIgnoreAttributes(object, name, value, attributes,
-                                        handling);
-}
-
-
 #define FIELD_ADDR(p, offset) \
   (reinterpret_cast<byte*>(p) + offset - kHeapObjectTag)
 
diff --git a/src/objects.cc b/src/objects.cc
@@ -536,10 +536,11 @@ static bool FindAllCanWriteHolder(LookupIterator* it) {
 
 
 MaybeHandle<Object> JSObject::SetPropertyWithFailedAccessCheck(
-    LookupIterator* it, Handle<Object> value, LanguageMode language_mode) {
+    LookupIterator* it, Handle<Object> value) {
   Handle<JSObject> checked = it->GetHolder<JSObject>();
   if (FindAllCanWriteHolder(it)) {
-    return SetPropertyWithAccessor(it, value, language_mode);
+    // The supplied language-mode is ignored by SetPropertyWithAccessor.
+    return SetPropertyWithAccessor(it, value, SLOPPY);
   }
 
   it->isolate()->ReportFailedAccessCheck(checked);
@@ -3044,8 +3045,7 @@ MaybeHandle<Object> Object::SetPropertyInternal(LookupIterator* it,
         if (it->HasAccess()) break;
         // Check whether it makes sense to reuse the lookup iterator. Here it
         // might still call into setters up the prototype chain.
-        return JSObject::SetPropertyWithFailedAccessCheck(it, value,
-                                                          language_mode);
+        return JSObject::SetPropertyWithFailedAccessCheck(it, value);
 
       case LookupIterator::JSPROXY:
         if (it->HolderIsReceiverOrHiddenPrototype()) {
@@ -3165,9 +3165,8 @@ MaybeHandle<Object> Object::SetSuperProperty(LookupIterator* it,
   for (; own_lookup.IsFound(); own_lookup.Next()) {
     switch (own_lookup.state()) {
       case LookupIterator::ACCESS_CHECK:
-        if (!it->isolate()->MayAccess(own_lookup.GetHolder<JSObject>())) {
-          return JSObject::SetPropertyWithFailedAccessCheck(&own_lookup, value,
-                                                            language_mode);
+        if (!own_lookup.HasAccess()) {
+          return JSObject::SetPropertyWithFailedAccessCheck(&own_lookup, value);
         }
         break;
 
@@ -3177,17 +3176,17 @@ MaybeHandle<Object> Object::SetSuperProperty(LookupIterator* it,
       case LookupIterator::DATA: {
         PropertyDetails details = own_lookup.property_details();
         if (details.IsConfigurable() || !details.IsReadOnly()) {
-          return JSObject::ReconfigureAsDataProperty(&own_lookup, value,
-                                                     details.attributes());
+          return JSObject::DefineOwnPropertyIgnoreAttributes(
+              &own_lookup, value, details.attributes());
         }
         return WriteToReadOnlyProperty(&own_lookup, value, language_mode);
       }
 
       case LookupIterator::ACCESSOR: {
         PropertyDetails details = own_lookup.property_details();
         if (details.IsConfigurable()) {
-          return JSObject::ReconfigureAsDataProperty(&own_lookup, value,
-                                                     details.attributes());
+          return JSObject::DefineOwnPropertyIgnoreAttributes(
+              &own_lookup, value, details.attributes());
         }
 
         return RedefineNonconfigurableProperty(it->isolate(), it->GetName(),
@@ -4131,166 +4130,172 @@ void ExecutableAccessorInfo::ClearSetter(Handle<ExecutableAccessorInfo> info) {
 }
 
 
-MaybeHandle<Object> JSObject::ReconfigureAsDataProperty(
+// Reconfigures a property to a data property with attributes, even if it is not
+// reconfigurable.
+// Requires a LookupIterator that does not look at the prototype chain beyond
+// hidden prototypes.
+MaybeHandle<Object> JSObject::DefineOwnPropertyIgnoreAttributes(
     LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
     ExecutableAccessorInfoHandling handling) {
   Handle<JSObject> object = Handle<JSObject>::cast(it->GetReceiver());
   bool is_observed = object->map()->is_observed() &&
                      (it->IsElement() ||
                       !it->isolate()->IsInternallyUsedPropertyName(it->name()));
 
-  switch (it->state()) {
-    case LookupIterator::INTERCEPTOR:
-    case LookupIterator::JSPROXY:
-    case LookupIterator::NOT_FOUND:
-    case LookupIterator::TRANSITION:
-    case LookupIterator::ACCESS_CHECK:
-      UNREACHABLE();
+  for (; it->IsFound(); it->Next()) {
+    switch (it->state()) {
+      case LookupIterator::JSPROXY:
+      case LookupIterator::NOT_FOUND:
+      case LookupIterator::TRANSITION:
+        UNREACHABLE();
 
-    case LookupIterator::INTEGER_INDEXED_EXOTIC:
-      return value;
+      case LookupIterator::ACCESS_CHECK:
+        if (!it->HasAccess()) {
+          return SetPropertyWithFailedAccessCheck(it, value);
+        }
+        break;
 
-    case LookupIterator::ACCESSOR: {
-      PropertyDetails details = it->property_details();
-      // Ensure the context isn't changed after calling into accessors.
-      AssertNoContextChange ncc(it->isolate());
+      // If there's an interceptor, try to store the property with the
+      // interceptor.
+      // In case of success, the attributes will have been reset to the default
+      // attributes of the interceptor, rather than the incoming attributes.
+      //
+      // TODO(verwaest): JSProxy afterwards verify the attributes that the
+      // JSProxy claims it has, and verifies that they are compatible. If not,
+      // they throw. Here we should do the same.
+      case LookupIterator::INTERCEPTOR:
+        if (handling == DONT_FORCE_FIELD) {
+          MaybeHandle<Object> maybe_result =
+              JSObject::SetPropertyWithInterceptor(it, value);
+          if (!maybe_result.is_null()) return maybe_result;
+          if (it->isolate()->has_pending_exception()) return maybe_result;
+        }
+        break;
 
-      Handle<Object> accessors = it->GetAccessors();
+      case LookupIterator::INTEGER_INDEXED_EXOTIC:
+        return value;
 
-      // Special handling for ExecutableAccessorInfo, which behaves like a
-      // data property.
-      if (accessors->IsExecutableAccessorInfo() &&
-          handling == DONT_FORCE_FIELD) {
-        Handle<Object> result;
-        ASSIGN_RETURN_ON_EXCEPTION(
-            it->isolate(), result,
-            JSObject::SetPropertyWithAccessor(it, value, STRICT), Object);
-        DCHECK(result->SameValue(*value));
-
-        if (details.attributes() == attributes) return value;
-
-        // Reconfigure the accessor if attributes mismatch.
-        Handle<ExecutableAccessorInfo> new_data = Accessors::CloneAccessor(
-            it->isolate(), Handle<ExecutableAccessorInfo>::cast(accessors));
-        new_data->set_property_attributes(attributes);
-        // By clearing the setter we don't have to introduce a lookup to
-        // the setter, simply make it unavailable to reflect the
-        // attributes.
-        if (attributes & READ_ONLY) {
-          ExecutableAccessorInfo::ClearSetter(new_data);
-        }
+      case LookupIterator::ACCESSOR: {
+        Handle<Object> accessors = it->GetAccessors();
 
-        if (it->IsElement()) {
-          SetElementCallback(it->GetHolder<JSObject>(), it->index(), new_data,
-                             attributes);
+        // Special handling for ExecutableAccessorInfo, which behaves like a
+        // data property.
+        if (accessors->IsExecutableAccessorInfo() &&
+            handling == DONT_FORCE_FIELD) {
+          PropertyDetails details = it->property_details();
+          // Ensure the context isn't changed after calling into accessors.
+          AssertNoContextChange ncc(it->isolate());
+
+          Handle<Object> result;
+          ASSIGN_RETURN_ON_EXCEPTION(
+              it->isolate(), result,
+              JSObject::SetPropertyWithAccessor(it, value, STRICT), Object);
+          DCHECK(result->SameValue(*value));
+
+          if (details.attributes() == attributes) return value;
+
+          // Reconfigure the accessor if attributes mismatch.
+          Handle<ExecutableAccessorInfo> new_data = Accessors::CloneAccessor(
+              it->isolate(), Handle<ExecutableAccessorInfo>::cast(accessors));
+          new_data->set_property_attributes(attributes);
+          // By clearing the setter we don't have to introduce a lookup to
+          // the setter, simply make it unavailable to reflect the
+          // attributes.
+          if (attributes & READ_ONLY) {
+            ExecutableAccessorInfo::ClearSetter(new_data);
+          }
+
+          if (it->IsElement()) {
+            SetElementCallback(it->GetHolder<JSObject>(), it->index(), new_data,
+                               attributes);
+          } else {
+            SetPropertyCallback(it->GetHolder<JSObject>(), it->name(), new_data,
+                                attributes);
+          }
         } else {
-          SetPropertyCallback(it->GetHolder<JSObject>(), it->name(), new_data,
-                              attributes);
+          it->ReconfigureDataProperty(value, attributes);
+          it->WriteDataValue(value);
         }
+
         if (is_observed) {
           RETURN_ON_EXCEPTION(
               it->isolate(),
               EnqueueChangeRecord(object, "reconfigure", it->GetName(),
                                   it->factory()->the_hole_value()),
               Object);
         }
-        return value;
-      }
-
-      it->ReconfigureDataProperty(value, attributes);
-      it->WriteDataValue(value);
-
-      if (is_observed) {
-        RETURN_ON_EXCEPTION(
-            it->isolate(),
-            EnqueueChangeRecord(object, "reconfigure", it->GetName(),
-                                it->factory()->the_hole_value()),
-            Object);
-      }
 
-      return value;
-    }
-
-    case LookupIterator::DATA: {
-      PropertyDetails details = it->property_details();
-      Handle<Object> old_value = it->factory()->the_hole_value();
-      // Regular property update if the attributes match.
-      if (details.attributes() == attributes) {
-        return SetDataProperty(it, value);
+        return value;
       }
+      case LookupIterator::DATA: {
+        PropertyDetails details = it->property_details();
+        Handle<Object> old_value = it->factory()->the_hole_value();
+        // Regular property update if the attributes match.
+        if (details.attributes() == attributes) {
+          return SetDataProperty(it, value);
+        }
 
-      // Special case: properties of typed arrays cannot be reconfigured to
-      // non-writable nor to non-enumerable.
-      if (it->IsElement() && (object->HasExternalArrayElements() ||
-                              object->HasFixedTypedArrayElements())) {
-        return RedefineNonconfigurableProperty(it->isolate(), it->GetName(),
-                                               value, STRICT);
-      }
+        // Special case: properties of typed arrays cannot be reconfigured to
+        // non-writable nor to non-enumerable.
+        if (it->IsElement() && (object->HasExternalArrayElements() ||
+                                object->HasFixedTypedArrayElements())) {
+          return RedefineNonconfigurableProperty(it->isolate(), it->GetName(),
+                                                 value, STRICT);
+        }
 
-      // Reconfigure the data property if the attributes mismatch.
-      if (is_observed) old_value = it->GetDataValue();
+        // Reconfigure the data property if the attributes mismatch.
+        if (is_observed) old_value = it->GetDataValue();
 
-      it->ReconfigureDataProperty(value, attributes);
-      it->WriteDataValue(value);
+        it->ReconfigureDataProperty(value, attributes);
+        it->WriteDataValue(value);
 
-      if (is_observed) {
-        if (old_value->SameValue(*value)) {
-          old_value = it->factory()->the_hole_value();
+        if (is_observed) {
+          if (old_value->SameValue(*value)) {
+            old_value = it->factory()->the_hole_value();
+          }
+          RETURN_ON_EXCEPTION(it->isolate(),
+                              EnqueueChangeRecord(object, "reconfigure",
+                                                  it->GetName(), old_value),
+                              Object);
         }
-        RETURN_ON_EXCEPTION(it->isolate(),
-                            EnqueueChangeRecord(object, "reconfigure",
-                                                it->GetName(), old_value),
-                            Object);
+        return value;
       }
     }
   }
 
-  return value;
+  return AddDataProperty(it, value, attributes, STRICT,
+                         CERTAINLY_NOT_STORE_FROM_KEYED);
 }
 
 
-// Reconfigures a property to a data property with attributes, even if it is not
-// reconfigurable.
 MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
     Handle<JSObject> object, Handle<Name> name, Handle<Object> value,
     PropertyAttributes attributes, ExecutableAccessorInfoHandling handling) {
   DCHECK(!value->IsTheHole());
-  LookupIterator it(object, name, LookupIterator::OWN_SKIP_INTERCEPTOR);
-  if (it.state() == LookupIterator::ACCESS_CHECK) {
-    if (!it.HasAccess()) {
-      return SetPropertyWithFailedAccessCheck(&it, value, SLOPPY);
-    }
-    it.Next();
-  }
-
-  if (it.IsFound()) {
-    return ReconfigureAsDataProperty(&it, value, attributes, handling);
-  }
-
-  return AddDataProperty(&it, value, attributes, STRICT,
-                         CERTAINLY_NOT_STORE_FROM_KEYED);
+  LookupIterator it(object, name, LookupIterator::OWN);
+  return DefineOwnPropertyIgnoreAttributes(&it, value, attributes, handling);
 }
 
 
 MaybeHandle<Object> JSObject::SetOwnElementIgnoreAttributes(
     Handle<JSObject> object, uint32_t index, Handle<Object> value,
     PropertyAttributes attributes, ExecutableAccessorInfoHandling handling) {
   Isolate* isolate = object->GetIsolate();
-  LookupIterator it(isolate, object, index,
-                    LookupIterator::OWN_SKIP_INTERCEPTOR);
-  if (it.state() == LookupIterator::ACCESS_CHECK) {
-    if (!it.HasAccess()) {
-      return SetPropertyWithFailedAccessCheck(&it, value, STRICT);
-    }
-    it.Next();
-  }
+  LookupIterator it(isolate, object, index, LookupIterator::OWN);
+  return DefineOwnPropertyIgnoreAttributes(&it, value, attributes, handling);
+}
 
-  if (it.IsFound()) {
-    return ReconfigureAsDataProperty(&it, value, attributes, handling);
-  }
 
-  return AddDataProperty(&it, value, attributes, STRICT,
-                         MAY_BE_STORE_FROM_KEYED);
+MaybeHandle<Object> JSObject::DefinePropertyOrElementIgnoreAttributes(
+    Handle<JSObject> object, Handle<Name> name, Handle<Object> value,
+    PropertyAttributes attributes, ExecutableAccessorInfoHandling handling) {
+  Isolate* isolate = object->GetIsolate();
+  uint32_t index;
+  LookupIterator it =
+      name->AsArrayIndex(&index)
+          ? LookupIterator(isolate, object, index, LookupIterator::OWN)
+          : LookupIterator(object, name, LookupIterator::OWN);
+  return DefineOwnPropertyIgnoreAttributes(&it, value, attributes, handling);
 }
 
 
diff --git a/src/objects.h b/src/objects.h
diff --git a/src/runtime/runtime-classes.cc b/src/runtime/runtime-classes.cc

Original file line number	Diff line number	Diff line change
`@@ -3638,7 +3638,8 @@ static i::MaybeHandle<i::Object> DefineObjectProperty(`
`3638`	`3638`	`name = i::Handle<i::String>::cast(converted);`
`3639`	`3639`	`}`
`3640`	`3640`
`3641`		`- return i::JSObject::DefinePropertyOrElement(js_object, name, value, attrs);`
	`3641`	`+ return i::JSObject::DefinePropertyOrElementIgnoreAttributes(js_object, name,`
	`3642`	`+ value, attrs);`
`3642`	`3643`	`}`
`3643`	`3644`
`3644`	`3645`