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

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

diff --git a/src/runtime.cc b/src/runtime.cc
index 48ff69f..2a503e3 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,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();
 }
 

diff --git a/test/cctest/test-decls.cc b/test/cctest/test-decls.cc
index 88fa79b..6ea4c84 100644 (file)
--- a/test/cctest/test-decls.cc
+++ b/test/cctest/test-decls.cc
@@ -223,7 +223,7 @@ TEST(Unknown) {
   { DeclarationContext context;
     context.Check("function x() { }; x",
                   1,  // access
-                  1,  // declaration
+                  0,
                   0,
                   EXPECT_RESULT);
   }
@@ -278,7 +278,7 @@ TEST(Present) {
   { PresentPropertyContext context;
     context.Check("function x() { }; x",
                   1,  // access
-                  1,  // declaration
+                  0,
                   0,
                   EXPECT_RESULT);
   }
@@ -332,7 +332,7 @@ TEST(Absent) {
   { AbsentPropertyContext context;
     context.Check("function x() { }; x",
                   1,  // access
-                  1,  // declaration
+                  0,
                   0,
                   EXPECT_RESULT);
   }
@@ -422,7 +422,7 @@ TEST(Appearing) {
   { AppearingPropertyContext context;
     context.Check("function x() { }; x",
                   1,  // access
-                  1,  // declaration
+                  0,
                   0,
                   EXPECT_RESULT);
   }

diff --git a/test/mjsunit/regress/regress-1105.js b/test/mjsunit/regress/regress-1170.js
similarity index 63%
rename from test/mjsunit/regress/regress-1105.js
rename to test/mjsunit/regress/regress-1170.js
index cfe2bd3..8a5a9cf 100644 (file)
--- a/test/mjsunit/regress/regress-1105.js
+++ b/test/mjsunit/regress/regress-1170.js
@@ -25,14 +25,42 @@
 // (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.
+var setter_value = 0;
 
-__defineSetter__("x", function(){ throw 42; });
+__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 {
-   this.eval('function x(){}');
-   assertUnreachable();
-} catch (e) {
-   assertEquals(42, e);
+  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"));
+}
+