Revision: 21558
Author: [email protected]
Date: Wed May 28 09:58:27 2014 UTC
Log: 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.
[email protected]
Review URL: https://codereview.chromium.org/262053011
http://code.google.com/p/v8/source/detail?r=21558
Added:
/branches/bleeding_edge/test/mjsunit/regress/regress-3334.js
Modified:
/branches/bleeding_edge/src/accessors.cc
/branches/bleeding_edge/src/accessors.h
/branches/bleeding_edge/src/objects-inl.h
/branches/bleeding_edge/src/objects.cc
/branches/bleeding_edge/src/objects.h
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/test/cctest/test-api.cc
=======================================
--- /dev/null
+++ /branches/bleeding_edge/test/mjsunit/regress/regress-3334.js Wed May 28
09:58:27 2014 UTC
@@ -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);
=======================================
--- /branches/bleeding_edge/src/accessors.cc Thu May 22 15:27:57 2014 UTC
+++ /branches/bleeding_edge/src/accessors.cc Wed May 28 09:58:27 2014 UTC
@@ -49,6 +49,21 @@
info->set_setter(*set);
return info;
}
+
+
+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>
=======================================
--- /branches/bleeding_edge/src/accessors.h Mon May 26 11:28:08 2014 UTC
+++ /branches/bleeding_edge/src/accessors.h Wed May 28 09:58:27 2014 UTC
@@ -86,6 +86,11 @@
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);
=======================================
--- /branches/bleeding_edge/src/objects-inl.h Tue May 27 13:43:29 2014 UTC
+++ /branches/bleeding_edge/src/objects-inl.h Wed May 28 09:58:27 2014 UTC
@@ -6418,6 +6418,11 @@
if (!function_template->IsFunctionTemplateInfo()) return true;
return
FunctionTemplateInfo::cast(function_template)->IsTemplateFor(receiver);
}
+
+
+void ExecutableAccessorInfo::clear_setter() {
+ set_setter(GetIsolate()->heap()->undefined_value(), SKIP_WRITE_BARRIER);
+}
void AccessorPair::set_access_flags(v8::AccessControl access_control) {
=======================================
--- /branches/bleeding_edge/src/objects.cc Wed May 28 09:29:27 2014 UTC
+++ /branches/bleeding_edge/src/objects.cc Wed May 28 09:58:27 2014 UTC
@@ -4335,9 +4335,6 @@
// 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 @@
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
@@ -4400,6 +4398,8 @@
}
old_attributes = lookup.GetAttributes();
}
+
+ bool executed_set_prototype = false;
// Check of IsReadOnly removed from here in clone.
if (lookup.IsTransition()) {
@@ -4425,8 +4425,48 @@
}
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 @@
}
}
- if (is_observed) {
+ if (is_observed && !executed_set_prototype) {
if (lookup.IsTransition()) {
EnqueueChangeRecord(object, "add", name, old_value);
} else if (old_value->IsTheHole()) {
=======================================
--- /branches/bleeding_edge/src/objects.h Wed May 28 09:29:27 2014 UTC
+++ /branches/bleeding_edge/src/objects.h Wed May 28 09:58:27 2014 UTC
@@ -2154,6 +2154,13 @@
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 @@
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 @@
static const int kDataOffset = kSetterOffset + kPointerSize;
static const int kSize = kDataOffset + kPointerSize;
+ inline void clear_setter();
+
private:
DISALLOW_IMPLICIT_CONSTRUCTORS(ExecutableAccessorInfo);
};
=======================================
--- /branches/bleeding_edge/src/runtime.cc Wed May 28 09:29:27 2014 UTC
+++ /branches/bleeding_edge/src/runtime.cc Wed May 28 09:58:27 2014 UTC
@@ -5174,26 +5174,6 @@
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
@@ -5209,14 +5189,25 @@
// 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;
}
=======================================
--- /branches/bleeding_edge/test/cctest/test-api.cc Wed May 28 08:07:18
2014 UTC
+++ /branches/bleeding_edge/test/cctest/test-api.cc Wed May 28 09:58:27
2014 UTC
@@ -2002,6 +2002,27 @@
ExpectInt32("child.age", 10);
ExpectInt32("child.accessor_age", 10);
}
+
+
+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-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
