Revision: 6846
Author: [email protected]
Date: Thu Feb 17 13:04:53 2011
Log: Change behavior of global declarations in the presence of setters.
Call accessors in the global object prototype when initializing global
variables. Function declarations are special cased for compatibility
with Safari and setters are not called for them. If this special
casing was not done webkit layout tests would fail.
Make the declaration of global const variables in the presence of
callbacks a redeclaration error.
Handle const context slot declarations conflicting with a CALLBACK as
a redeclaration error. That is, unless it is on a context extension
object which is not a real object and therefore conceptually have no
accessors in prototype chains. Accessors in prototype chains of
context extension objects are explicitly ignored in SetProperty.
Review URL: http://codereview.chromium.org/6534029
http://code.google.com/p/v8/source/detail?r=6846
Added:
/branches/bleeding_edge/test/mjsunit/regress/regress-1170.js
Deleted:
/branches/bleeding_edge/test/mjsunit/regress/regress-1105.js
Modified:
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/test/cctest/test-decls.cc
=======================================
--- /dev/null
+++ /branches/bleeding_edge/test/mjsunit/regress/regress-1170.js Thu Feb 17
13:04:53 2011
@@ -0,0 +1,66 @@
+// Copyright 2011 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+// * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+// * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following
+// disclaimer in the documentation and/or other materials provided
+// with the distribution.
+// * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived
+// from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+var setter_value = 0;
+
+__proto__.__defineSetter__("a", function(v) { setter_value = v; });
+eval("var a = 1");
+assertEquals(1, setter_value);
+assertFalse(hasOwnProperty("a"));
+
+eval("with({}) { eval('var a = 2') }");
+assertEquals(2, setter_value);
+assertFalse(hasOwnProperty("a"));
+
+// Function declarations are treated specially to match Safari. We do
+// not call setters for them.
+eval("function a() {}");
+assertTrue(hasOwnProperty("a"));
+
+__proto__.__defineSetter__("b", function(v) { assertUnreachable(); });
+try {
+ eval("const b = 23");
+ assertUnreachable();
+} catch(e) {
+ assertTrue(/TypeError/.test(e));
+}
+try {
+ eval("with({}) { eval('const b = 23') }");
+ assertUnreachable();
+} catch(e) {
+ assertTrue(/TypeError/.test(e));
+}
+
+__proto__.__defineSetter__("c", function(v) { throw 42; });
+try {
+ eval("var c = 1");
+ assertUnreachable();
+} catch(e) {
+ assertEquals(42, e);
+ assertFalse(hasOwnProperty("c"));
+}
+
=======================================
--- /branches/bleeding_edge/test/mjsunit/regress/regress-1105.js Tue Feb 8
06:04:27 2011
+++ /dev/null
@@ -1,38 +0,0 @@
-// Copyright 2011 the V8 project authors. All rights reserved.
-// Redistribution and use in source and binary forms, with or without
-// modification, are permitted provided that the following conditions are
-// met:
-//
-// * Redistributions of source code must retain the above copyright
-// notice, this list of conditions and the following disclaimer.
-// * Redistributions in binary form must reproduce the above
-// copyright notice, this list of conditions and the following
-// disclaimer in the documentation and/or other materials provided
-// with the distribution.
-// * Neither the name of Google Inc. nor the names of its
-// contributors may be used to endorse or promote products derived
-// from this software without specific prior written permission.
-//
-// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
-// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
-// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
-// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-// This should properly catch the exception from the setter triggered
-// by the loaded file, and it should not fail an assertion in debug mode.
-
-__defineSetter__("x", function(){ throw 42; });
-
-try {
- this.eval('function x(){}');
- assertUnreachable();
-} catch (e) {
- assertEquals(42, e);
-}
=======================================
--- /branches/bleeding_edge/src/runtime.cc Thu Feb 17 08:54:49 2011
+++ /branches/bleeding_edge/src/runtime.cc Thu Feb 17 13:04:53 2011
@@ -1051,6 +1051,12 @@
// 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_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 @@
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 @@
// 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 @@
}
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();
}
=======================================
--- /branches/bleeding_edge/test/cctest/test-decls.cc Tue Dec 7 03:01:02
2010
+++ /branches/bleeding_edge/test/cctest/test-decls.cc Thu Feb 17 13:04:53
2011
@@ -223,7 +223,7 @@
{ DeclarationContext context;
context.Check("function x() { }; x",
1, // access
- 1, // declaration
+ 0,
0,
EXPECT_RESULT);
}
@@ -278,7 +278,7 @@
{ PresentPropertyContext context;
context.Check("function x() { }; x",
1, // access
- 1, // declaration
+ 0,
0,
EXPECT_RESULT);
}
@@ -332,7 +332,7 @@
{ AbsentPropertyContext context;
context.Check("function x() { }; x",
1, // access
- 1, // declaration
+ 0,
0,
EXPECT_RESULT);
}
@@ -422,7 +422,7 @@
{ AppearingPropertyContext context;
context.Check("function x() { }; x",
1, // access
- 1, // declaration
+ 0,
0,
EXPECT_RESULT);
}
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
