Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1235)

Unified Diff: src/runtime.cc

Issue 6534029: Change behavior of global declarations in the presence of setters. (Closed) Base URL: https://v8.googlecode.com/svn/branches/bleeding_edge
Patch Set: Created 9 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | test/cctest/test-decls.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index 48ff69f5d273071e0615743e2b2760f2abb71e70..2a503e3cebf18c7bc8752bceba57a3ed83961e53 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -1051,6 +1051,12 @@ static MaybeObject* Runtime_DeclareGlobals(Arguments args) {
// Fall-through and introduce the absent property by using
// SetProperty.
} else {
+ // For const properties, we treat a callback with this name
+ // even in the prototype as a conflicting declaration.
+ if (is_const_property && (lookup.type() == CALLBACKS)) {
+ return ThrowRedeclarationError("const", name);
+ }
+ // Otherwise, we check for locally conflicting declarations.
if (is_local && (is_read_only || is_const_property)) {
const char* type = (is_read_only) ? "const" : "var";
return ThrowRedeclarationError(type, name);
@@ -1076,29 +1082,34 @@ static MaybeObject* Runtime_DeclareGlobals(Arguments args) {
? static_cast<PropertyAttributes>(base | READ_ONLY)
: base;
- if (lookup.IsProperty()) {
- // There's a local property that we need to overwrite because
- // we're either declaring a function or there's an interceptor
- // that claims the property is absent.
-
- // Check for conflicting re-declarations. We cannot have
- // conflicting types in case of intercepted properties because
- // they are absent.
- if (lookup.type() != INTERCEPTOR &&
- (lookup.IsReadOnly() || is_const_property)) {
- const char* type = (lookup.IsReadOnly()) ? "const" : "var";
- return ThrowRedeclarationError(type, name);
- }
- RETURN_IF_EMPTY_HANDLE(SetProperty(global, name, value, attributes));
+ // There's a local property that we need to overwrite because
+ // we're either declaring a function or there's an interceptor
+ // that claims the property is absent.
+ //
+ // Check for conflicting re-declarations. We cannot have
+ // conflicting types in case of intercepted properties because
+ // they are absent.
+ if (lookup.IsProperty() &&
+ (lookup.type() != INTERCEPTOR) &&
+ (lookup.IsReadOnly() || is_const_property)) {
+ const char* type = (lookup.IsReadOnly()) ? "const" : "var";
+ return ThrowRedeclarationError(type, name);
+ }
+
+ // Safari does not allow the invocation of callback setters for
+ // function declarations. To mimic this behavior, we do not allow
+ // the invocation of setters for function values. This makes a
+ // difference for global functions with the same names as event
+ // handlers such as "function onload() {}". Firefox does call the
+ // onload setter in those case and Safari does not. We follow
+ // Safari for compatibility.
+ if (value->IsJSFunction()) {
+ RETURN_IF_EMPTY_HANDLE(SetLocalPropertyIgnoreAttributes(global,
+ name,
+ value,
+ attributes));
} else {
- // If a property with this name does not already exist on the
- // global object add the property locally. We take special
- // precautions to always add it as a local property even in case
- // of callbacks in the prototype chain (this rules out using
- // SetProperty). Also, we must use the handle-based version to
- // avoid GC issues.
- RETURN_IF_EMPTY_HANDLE(
- SetLocalPropertyIgnoreAttributes(global, name, value, attributes));
+ RETURN_IF_EMPTY_HANDLE(SetProperty(global, name, value, attributes));
}
}
@@ -1186,6 +1197,20 @@ static MaybeObject* Runtime_DeclareContextSlot(Arguments args) {
ASSERT(!context_ext->HasLocalProperty(*name));
Handle<Object> value(Heap::undefined_value());
if (*initial_value != NULL) value = initial_value;
+ // Declaring a const context slot is a conflicting declaration if
+ // there is a callback with that name in a prototype. It is
+ // allowed to introduce const variables in
+ // JSContextExtensionObjects. They are treated specially in
+ // SetProperty and no setters are invoked for those since they are
+ // not real JSObjects.
+ if (initial_value->IsTheHole() &&
+ !context_ext->IsJSContextExtensionObject()) {
+ LookupResult lookup;
+ context_ext->Lookup(*name, &lookup);
+ if (lookup.IsProperty() && (lookup.type() == CALLBACKS)) {
+ return ThrowRedeclarationError("const", name);
+ }
+ }
RETURN_IF_EMPTY_HANDLE(SetProperty(context_ext, name, value, mode));
}
@@ -1212,11 +1237,7 @@ static MaybeObject* Runtime_InitializeVarGlobal(Arguments args) {
// there, there is a property with this name in the prototype chain.
// We follow Safari and Firefox behavior and only set the property
// locally if there is an explicit initialization value that we have
- // to assign to the property. When adding the property we take
- // special precautions to always add it as a local property even in
- // case of callbacks in the prototype chain (this rules out using
- // SetProperty). We have SetLocalPropertyIgnoreAttributes for
- // this.
+ // to assign to the property.
// Note that objects can have hidden prototypes, so we need to traverse
// the whole chain of hidden prototypes to do a 'local' lookup.
JSObject* real_holder = global;
@@ -1277,11 +1298,7 @@ static MaybeObject* Runtime_InitializeVarGlobal(Arguments args) {
}
global = Top::context()->global();
- if (assign) {
- return global->SetLocalPropertyIgnoreAttributes(*name,
- args[1],
- attributes);
- }
+ if (assign) return global->SetProperty(*name, args[1], attributes);
return Heap::undefined_value();
}
« no previous file with comments | « no previous file | test/cctest/test-decls.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698