Changing the attributes of a data property implemented with

ExecutableAccessorInfo turns the property into a field. Better
to keep it as a callback, and correctly deal with the changed
property attributes.

R=ulan@chromium.org

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

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21558 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/accessors.cc b/src/accessors.cc
index 6245613..cd55091 100644 (file)
--- a/src/accessors.cc
+++ b/src/accessors.cc
@@ -51,6 +51,21 @@ Handle<AccessorInfo> Accessors::MakeAccessor(
 }
 
 
+Handle<ExecutableAccessorInfo> Accessors::CloneAccessor(
+    Isolate* isolate,
+    Handle<ExecutableAccessorInfo> accessor) {
+  Factory* factory = isolate->factory();
+  Handle<ExecutableAccessorInfo> info = factory->NewExecutableAccessorInfo();
+  info->set_name(accessor->name());
+  info->set_flag(accessor->flag());
+  info->set_expected_receiver_type(accessor->expected_receiver_type());
+  info->set_getter(accessor->getter());
+  info->set_setter(accessor->setter());
+  info->set_data(accessor->data());
+  return info;
+}
+
+
 template <class C>
 static C* FindInstanceOf(Isolate* isolate, Object* obj) {
   for (Object* cur = obj; !cur->IsNull(); cur = cur->GetPrototype(isolate)) {

diff --git a/src/accessors.h b/src/accessors.h
index d7296f2..ae748c5 100644 (file)
--- a/src/accessors.h
+++ b/src/accessors.h
@@ -86,6 +86,11 @@ class Accessors : public AllStatic {
       AccessorSetterCallback setter,
       PropertyAttributes attributes);
 
+  static Handle<ExecutableAccessorInfo> CloneAccessor(
+      Isolate* isolate,
+      Handle<ExecutableAccessorInfo> accessor);
+
+
  private:
   // Helper functions.
   static Handle<Object> FlattenNumber(Isolate* isolate, Handle<Object> value);

diff --git a/src/objects-inl.h b/src/objects-inl.h
index 9c76fae..29fcec6 100644 (file)
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -6420,6 +6420,11 @@ bool AccessorInfo::IsCompatibleReceiver(Object* receiver) {
 }
 
 
+void ExecutableAccessorInfo::clear_setter() {
+  set_setter(GetIsolate()->heap()->undefined_value(), SKIP_WRITE_BARRIER);
+}
+
+
 void AccessorPair::set_access_flags(v8::AccessControl access_control) {
   int current = access_flags()->value();
   current = BooleanBit::set(current,

diff --git a/src/objects.cc b/src/objects.cc
index 3d53f95..19dcf1b 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -4335,9 +4335,6 @@ MaybeHandle<Object> JSObject::SetPropertyForResult(
 // callback setter removed.  The two lines looking up the LookupResult
 // result are also added.  If one of the functions is changed, the other
 // should be.
-// Note that this method cannot be used to set the prototype of a function
-// because ConvertDescriptorToField() which is called in "case CALLBACKS:"
-// doesn't handle function prototypes correctly.
 MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
     Handle<JSObject> object,
     Handle<Name> name,
@@ -4346,7 +4343,8 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
     ValueType value_type,
     StoreMode mode,
     ExtensibilityCheck extensibility_check,
-    StoreFromKeyed store_from_keyed) {
+    StoreFromKeyed store_from_keyed,
+    ExecutableAccessorInfoHandling handling) {
   Isolate* isolate = object->GetIsolate();
 
   // Make sure that the top context does not change when doing callbacks or
@@ -4401,6 +4399,8 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
     old_attributes = lookup.GetAttributes();
   }
 
+  bool executed_set_prototype = false;
+
   // Check of IsReadOnly removed from here in clone.
   if (lookup.IsTransition()) {
     Handle<Object> result;
@@ -4425,8 +4425,48 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
         }
         break;
       case CALLBACKS:
-        ConvertAndSetOwnProperty(&lookup, name, value, attributes);
+      {
+        Handle<Object> callback(lookup.GetCallbackObject(), isolate);
+        if (callback->IsExecutableAccessorInfo() &&
+            handling == DONT_FORCE_FIELD) {
+          Handle<Object> result;
+          ASSIGN_RETURN_ON_EXCEPTION(
+              isolate, result,
+              JSObject::SetPropertyWithCallback(object,
+                                                name,
+                                                value,
+                                                handle(lookup.holder()),
+                                                callback,
+                                                STRICT),
+              Object);
+
+          if (attributes != lookup.GetAttributes()) {
+            Handle<ExecutableAccessorInfo> new_data =
+                Accessors::CloneAccessor(
+                    isolate, Handle<ExecutableAccessorInfo>::cast(callback));
+            new_data->set_property_attributes(attributes);
+            if (attributes & READ_ONLY) {
+              // This way we don't have to introduce a lookup to the setter,
+              // simply make it unavailable to reflect the attributes.
+              new_data->clear_setter();
+            }
+
+            SetPropertyCallback(object, name, new_data, attributes);
+          }
+          if (is_observed) {
+            // If we are setting the prototype of a function and are observed,
+            // don't send change records because the prototype handles that
+            // itself.
+            executed_set_prototype = object->IsJSFunction() &&
+                String::Equals(isolate->factory()->prototype_string(),
+                               Handle<String>::cast(name)) &&
+                Handle<JSFunction>::cast(object)->should_have_prototype();
+          }
+        } else {
+          ConvertAndSetOwnProperty(&lookup, name, value, attributes);
+        }
         break;
+      }
       case NONEXISTENT:
       case HANDLER:
       case INTERCEPTOR:
@@ -4434,7 +4474,7 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
     }
   }
 
-  if (is_observed) {
+  if (is_observed && !executed_set_prototype) {
     if (lookup.IsTransition()) {
       EnqueueChangeRecord(object, "add", name, old_value);
     } else if (old_value->IsTheHole()) {

diff --git a/src/objects.h b/src/objects.h
index cde3bf7..64664a5 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -2154,6 +2154,13 @@ class JSObject: public JSReceiver {
       StrictMode strict_mode,
       StoreFromKeyed store_mode = MAY_BE_STORE_FROM_KEYED);
 
+  // SetLocalPropertyIgnoreAttributes converts callbacks to fields. We need to
+  // grant an exemption to ExecutableAccessor callbacks in some cases.
+  enum ExecutableAccessorInfoHandling {
+    DEFAULT_HANDLING,
+    DONT_FORCE_FIELD
+  };
+
   MUST_USE_RESULT static MaybeHandle<Object> SetOwnPropertyIgnoreAttributes(
       Handle<JSObject> object,
       Handle<Name> key,
@@ -2162,7 +2169,8 @@ class JSObject: public JSReceiver {
       ValueType value_type = OPTIMAL_REPRESENTATION,
       StoreMode mode = ALLOW_AS_CONSTANT,
       ExtensibilityCheck extensibility_check = PERFORM_EXTENSIBILITY_CHECK,
-      StoreFromKeyed store_mode = MAY_BE_STORE_FROM_KEYED);
+      StoreFromKeyed store_mode = MAY_BE_STORE_FROM_KEYED,
+      ExecutableAccessorInfoHandling handling = DEFAULT_HANDLING);
 
   static inline Handle<String> ExpectedTransitionKey(Handle<Map> map);
   static inline Handle<Map> ExpectedTransitionTarget(Handle<Map> map);
@@ -10536,6 +10544,8 @@ class ExecutableAccessorInfo: public AccessorInfo {
   static const int kDataOffset = kSetterOffset + kPointerSize;
   static const int kSize = kDataOffset + kPointerSize;
 
+  inline void clear_setter();
+
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(ExecutableAccessorInfo);
 };

diff --git a/src/runtime.cc b/src/runtime.cc
index 68a730b..c5b60e9 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -5175,26 +5175,6 @@ RUNTIME_FUNCTION(Runtime_DefineOrRedefineDataProperty) {
   LookupResult lookup(isolate);
   js_object->LookupOwnRealNamedProperty(name, &lookup);
 
-  // Special case for callback properties.
-  if (lookup.IsPropertyCallbacks()) {
-    Handle<Object> callback(lookup.GetCallbackObject(), isolate);
-    // Avoid redefining callback as data property, just use the stored
-    // setter to update the value instead.
-    // TODO(mstarzinger): So far this only works if property attributes don't
-    // change, this should be fixed once we cleanup the underlying code.
-    ASSERT(!callback->IsForeign());
-    if (callback->IsAccessorInfo() &&
-        lookup.GetAttributes() == attr) {
-      Handle<Object> result_object;
-      ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-          isolate, result_object,
-          JSObject::SetPropertyWithCallback(js_object, name, obj_value,
-                                            handle(lookup.holder()),
-                                            callback, STRICT));
-      return *result_object;
-    }
-  }
-
   // Take special care when attributes are different and there is already
   // a property. For simplicity we normalize the property which enables us
   // to not worry about changing the instance_descriptor and creating a new
@@ -5209,14 +5189,25 @@ RUNTIME_FUNCTION(Runtime_DefineOrRedefineDataProperty) {
       // we don't have to check for null.
       js_object = Handle<JSObject>(JSObject::cast(js_object->GetPrototype()));
     }
-    JSObject::NormalizeProperties(js_object, CLEAR_INOBJECT_PROPERTIES, 0);
+
+    if (attr != lookup.GetAttributes() ||
+        (lookup.IsPropertyCallbacks() &&
+         !lookup.GetCallbackObject()->IsAccessorInfo())) {
+      JSObject::NormalizeProperties(js_object, CLEAR_INOBJECT_PROPERTIES, 0);
+    }
+
     // Use IgnoreAttributes version since a readonly property may be
     // overridden and SetProperty does not allow this.
     Handle<Object> result;
     ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
         isolate, result,
         JSObject::SetOwnPropertyIgnoreAttributes(
-            js_object, name, obj_value, attr));
+            js_object, name, obj_value, attr,
+            Object::OPTIMAL_REPRESENTATION,
+            ALLOW_AS_CONSTANT,
+            JSReceiver::PERFORM_EXTENSIBILITY_CHECK,
+            JSReceiver::MAY_BE_STORE_FROM_KEYED,
+            JSObject::DONT_FORCE_FIELD));
     return *result;
   }
 

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 6b55d7a..df5938d 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -2004,6 +2004,27 @@ THREADED_TEST(EmptyInterceptorDoesNotShadowAccessors) {
 }
 
 
+THREADED_TEST(ExecutableAccessorIsPreservedOnAttributeChange) {
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+  LocalContext env;
+  v8::Local<v8::Value> res = CompileRun("var a = []; a;");
+  i::Handle<i::JSObject> a(v8::Utils::OpenHandle(v8::Object::Cast(*res)));
+  CHECK(a->map()->instance_descriptors()->IsFixedArray());
+  CHECK_GT(i::FixedArray::cast(a->map()->instance_descriptors())->length(), 0);
+  CompileRun("Object.defineProperty(a, 'length', { writable: false });");
+  CHECK_EQ(i::FixedArray::cast(a->map()->instance_descriptors())->length(), 0);
+  // But we should still have an ExecutableAccessorInfo.
+  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  i::LookupResult lookup(i_isolate);
+  i::Handle<i::String> name(v8::Utils::OpenHandle(*v8_str("length")));
+  a->LookupOwnRealNamedProperty(name, &lookup);
+  CHECK(lookup.IsPropertyCallbacks());
+  i::Handle<i::Object> callback(lookup.GetCallbackObject(), i_isolate);
+  CHECK(callback->IsExecutableAccessorInfo());
+}
+
+
 THREADED_TEST(EmptyInterceptorBreakTransitions) {
   v8::HandleScope scope(CcTest::isolate());
   Handle<FunctionTemplate> templ = FunctionTemplate::New(CcTest::isolate());

diff --git a/test/mjsunit/regress/regress-3334.js b/test/mjsunit/regress/regress-3334.js
new file mode 100644 (file)
index 0000000..301155d
--- /dev/null
+++ b/test/mjsunit/regress/regress-3334.js
@@ -0,0 +1,13 @@
+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+function foo(){}
+Object.defineProperty(foo, "prototype", { value: 2 });
+assertEquals(2, foo.prototype);
+
+function bar(){}
+Object.defineProperty(bar, "prototype", { value: 2, writable: false });
+assertEquals(2, bar.prototype);
+assertThrows(function() { "use strict"; bar.prototype = 10; }, TypeError);
+assertEquals(false, Object.getOwnPropertyDescriptor(bar,"prototype").writable);