Call accessors in the global object prototype when initializing global

variables.

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/6519050

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

diff --git a/src/runtime.cc b/src/runtime.cc
index 48ff69f..a6670bf 100644 (file)
--- 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,30 +1082,20 @@ 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));
-    } 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));
+    // 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);
     }
+    RETURN_IF_EMPTY_HANDLE(SetProperty(global, name, value, attributes));
   }
 
   ASSERT(!Top::has_pending_exception());
@@ -1186,6 +1182,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 +1222,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 +1283,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();
 }
 

diff --git a/test/mjsunit/regress/regress-1170.js b/test/mjsunit/regress/regress-1170.js
new file mode 100644 (file)
index 0000000..090e22e
--- /dev/null
+++ b/test/mjsunit/regress/regress-1170.js
@@ -0,0 +1,52 @@
+// 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"));
+
+__proto__.__defineSetter__("b", function(v) { setter_value = v; });
+eval("with({}) { eval('var b = 2') }");
+assertEquals(2, setter_value);
+assertFalse(hasOwnProperty("b"));
+
+__proto__.__defineSetter__("c", function(v) { assertTrue(false); });
+try {
+  eval("const c = 23");
+  assertTrue(false);
+} catch(e) {
+  assertTrue(/TypeError/.test(e));
+}
+try {
+  eval("with({}) { eval('const c = 23') }");
+  assertTrue(false);
+} catch(e) {
+  assertTrue(/TypeError/.test(e));
+}