Bug 714614: don't create call objects with duplicated property names,…

… r=bhackett, a=akeybl
mozilla · Jan 27, 2012 · 83b79be · 83b79be
1 parent 9c44977
commit 83b79be
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 16 deletions.
diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
@@ -1332,6 +1332,7 @@ Parser::functionArguments(TreeContext &funtc, FunctionBox *funbox, ParseNode **l
                  *     until after all arguments have been parsed.
                  */
                 if (funtc.decls.lookupFirst(name)) {
+                    funtc.bindings.noteDup();
                     duplicatedArg = name;
                     if (destructuringArg)
                         goto report_dup_and_destructuring;

diff --git a/js/src/jit-test/tests/basic/bug714614.js b/js/src/jit-test/tests/basic/bug714614.js
@@ -0,0 +1,5 @@
+function testForVarInWith(foo, foo) {
+  return eval("with ({}) { for (var x = 0; x < 5; x++); } (function() { return delete x; })");
+}
+f = testForVarInWith()();
+
diff --git a/js/src/jsscope.h b/js/src/jsscope.h
@@ -466,7 +466,6 @@ struct Shape : public js::gc::Cell
     friend class js::Bindings;
     friend struct js::StackShape;
     friend struct js::StackBaseShape;
-    friend bool IsShapeAboutToBeFinalized(JSContext *cx, const js::Shape *shape);
 
   protected:
     HeapPtrBaseShape    base_;

diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp
@@ -184,6 +184,47 @@ Bindings::add(JSContext *cx, JSAtom *name, BindingKind kind)
     return true;
 }
 
+Shape *
+Bindings::callObjectShape(JSContext *cx) const
+{
+    if (!hasDup())
+        return lastShape();
+
+    /*
+     * Build a vector of non-duplicate properties in order from last added
+     * to first (i.e., the order we normally have iterate over Shapes). Choose
+     * the last added property in each set of dups.
+     */
+    Vector<const Shape *> shapes(cx);
+    HashSet<jsid> seen(cx);
+    if (!seen.init())
+        return false;
+
+    for (Shape::Range r = lastShape()->all(); !r.empty(); r.popFront()) {
+        const Shape &s = r.front();
+        HashSet<jsid>::AddPtr p = seen.lookupForAdd(s.propid());
+        if (!p) {
+            if (!seen.add(p, s.propid()))
+                return NULL;
+            if (!shapes.append(&s))
+                return NULL;
+        }
+    }
+
+    /*
+     * Now build the Shape without duplicate properties.
+     */
+    RootedVarShape shape(cx);
+    shape = initialShape(cx);
+    for (int i = shapes.length() - 1; i >= 0; --i) {
+        shape = shape->getChildBinding(cx, shapes[i]);
+        if (!shape)
+            return NULL;
+    }
+
+    return shape;
+}
+
 bool
 Bindings::getLocalNameArray(JSContext *cx, Vector<JSAtom *> *namesp)
 {

diff --git a/js/src/jsscript.h b/js/src/jsscript.h
@@ -175,7 +175,9 @@ class Bindings {
     uint16_t nargs;
     uint16_t nvars;
     uint16_t nupvars;
+    bool     hasDup_:1;     // true if there are duplicate argument names
 
+    inline Shape *initialShape(JSContext *cx) const;
   public:
     inline Bindings(JSContext *cx);
 
@@ -207,9 +209,15 @@ class Bindings {
     /* Ensure these bindings have a shape lineage. */
     inline bool ensureShape(JSContext *cx);
 
-    /* Returns the shape lineage generated for these bindings. */
+    /* Return the shape lineage generated for these bindings. */
     inline Shape *lastShape() const;
 
+    /*
+     * Return the shape to use to create a call object for these bindings.
+     * The result is guaranteed not to have duplicate property names.
+     */
+    Shape *callObjectShape(JSContext *cx) const;
+
     /* See Scope::extensibleParents */
     inline bool extensibleParents();
     bool setExtensibleParents(JSContext *cx);
@@ -262,6 +270,9 @@ class Bindings {
         return add(cx, NULL, ARGUMENT);
     }
 
+    void noteDup() { hasDup_ = true; }
+    bool hasDup() const { return hasDup_; }
+
     /*
      * Look up an argument or variable name, returning its kind when found or
      * NONE when no such name exists. When indexp is not null and the name

diff --git a/js/src/jsscriptinlines.h b/js/src/jsscriptinlines.h
@@ -58,7 +58,7 @@ namespace js {
 
 inline
 Bindings::Bindings(JSContext *cx)
-    : lastBinding(NULL), nargs(0), nvars(0), nupvars(0)
+    : lastBinding(NULL), nargs(0), nvars(0), nupvars(0), hasDup_(false)
 {}
 
 inline void
@@ -90,16 +90,22 @@ Bindings::lastShape() const
     return lastBinding;
 }
 
+Shape *
+Bindings::initialShape(JSContext *cx) const
+{
+    /* Get an allocation kind to match an empty call object. */
+    gc::AllocKind kind = gc::FINALIZE_OBJECT4;
+    JS_ASSERT(gc::GetGCKindSlots(kind) == CallObject::RESERVED_SLOTS + 1);
+
+    return EmptyShape::getInitialShape(cx, &CallClass, NULL, NULL, kind,
+                                       BaseShape::VAROBJ);
+}
+
 bool
 Bindings::ensureShape(JSContext *cx)
 {
     if (!lastBinding) {
-        /* Get an allocation kind to match an empty call object. */
+        lastBinding = initialShape(cx);
-        gc::AllocKind kind = gc::FINALIZE_OBJECT4;
-        JS_ASSERT(gc::GetGCKindSlots(kind) == CallObject::RESERVED_SLOTS + 1);
-
-        lastBinding = EmptyShape::getInitialShape(cx, &CallClass, NULL, NULL, kind,
-                                                  BaseShape::VAROBJ);
         if (!lastBinding)
             return false;
     }

diff --git a/js/src/vm/ScopeObject.cpp b/js/src/vm/ScopeObject.cpp
@@ -1,4 +1,4 @@
-/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
+/* -*- Mode: C++; tab-width: 6; indent-tabs-mode: nil; c-basic-offset: 4 -*-
  * vim: set ts=8 sw=4 et tw=78:
  *
  * ***** BEGIN LICENSE BLOCK *****
@@ -65,8 +65,12 @@ using namespace js::types;
 CallObject *
 CallObject::create(JSContext *cx, JSScript *script, JSObject &enclosing, JSObject *callee)
 {
-    Bindings &bindings = script->bindings;
+    RootedVarShape shape(cx);
-    gc::AllocKind kind = gc::GetGCObjectKind(bindings.lastShape()->numFixedSlots() + 1);
+    shape = script->bindings.callObjectShape(cx);
+    if (shape == NULL)
+        return NULL;
+
+    gc::AllocKind kind = gc::GetGCObjectKind(shape->numFixedSlots() + 1);
 
     RootedVarTypeObject type(cx);
 
@@ -75,12 +79,9 @@ CallObject::create(JSContext *cx, JSScript *script, JSObject &enclosing, JSObjec
         return NULL;
 
     HeapValue *slots;
-    if (!PreallocateObjectDynamicSlots(cx, bindings.lastShape(), &slots))
+    if (!PreallocateObjectDynamicSlots(cx, shape, &slots))
         return NULL;
 
-    RootedVarShape shape(cx);
-    shape = bindings.lastShape();
-
     JSObject *obj = JSObject::create(cx, kind, shape, type, slots);
     if (!obj)
         return NULL;