Bug 714614: don't create call objects with duplicated property names, r=bhackett
authorDavid Mandelin <dmandelin@mozilla.com>
Fri, 27 Jan 2012 14:33:27 -0800
changeset 86475 2322fe48476ef4191a2bff07d6717211d9660f5d
parent 86474 2c49ea176da2f419c180bc7c52d4442ea3c8e77e
child 86476 af0ab64cb9477fad33d2ef89fce02f488487e376
push id22021
push userbmo@edmorley.co.uk
push dateThu, 09 Feb 2012 17:55:20 +0000
treeherdermozilla-central@7b1ae3535886 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbhackett
bugs714614
milestone13.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 714614: don't create call objects with duplicated property names, r=bhackett
js/src/frontend/Parser.cpp
js/src/jit-test/tests/basic/bug714614.js
js/src/jsscope.h
js/src/jsscript.cpp
js/src/jsscript.h
js/src/jsscriptinlines.h
js/src/vm/ScopeObject.cpp
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -1326,16 +1326,17 @@ Parser::functionArguments(TreeContext &f
                  * below, after having parsed the body in case it begins with a
                  * "use strict"; directive.
                  *
                  * NB: Check funtc.decls rather than funtc.bindings, because
                  *     destructuring bindings aren't added to funtc.bindings
                  *     until after all arguments have been parsed.
                  */
                 if (funtc.decls.lookupFirst(name)) {
+                    funtc.bindings.noteDup();
                     duplicatedArg = name;
                     if (destructuringArg)
                         goto report_dup_and_destructuring;
                 }
 #endif
 
                 uint16_t slot;
                 if (!funtc.bindings.addArgument(context, name, &slot))
new file mode 100644
--- /dev/null
+++ 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()();
+
--- a/js/src/jsscope.h
+++ b/js/src/jsscope.h
@@ -461,17 +461,16 @@ struct Shape : public js::gc::Cell
 {
     friend struct ::JSObject;
     friend struct ::JSFunction;
     friend class js::StaticBlockObject;
     friend class js::PropertyTree;
     friend class js::Bindings;
     friend struct js::StackShape;
     friend struct js::StackBaseShape;
-    friend bool IsShapeAboutToBeFinalized(JSContext *cx, const js::Shape *shape);
 
   protected:
     HeapPtrBaseShape    base_;
     HeapId              propid_;
 
     JS_ENUM_HEADER(SlotInfo, uint32_t)
     {
         /* Number of fixed slots in objects with this shape. */
--- a/js/src/jsscript.cpp
+++ b/js/src/jsscript.cpp
@@ -179,16 +179,57 @@ Bindings::add(JSContext *cx, JSAtom *nam
     if (!shape)
         return false;
 
     lastBinding = shape;
     ++*indexp;
     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)
 {
     JS_ASSERT(lastBinding);
     JS_ASSERT(hasLocalNames());
 
     Vector<JSAtom *> &names = *namesp;
     JS_ASSERT(names.empty());
--- a/js/src/jsscript.h
+++ b/js/src/jsscript.h
@@ -171,17 +171,19 @@ enum BindingKind { NONE, ARGUMENT, VARIA
  * both function and top-level scripts (the latter is needed to track names in
  * strict mode eval code, to give such code its own lexical environment).
  */
 class Bindings {
     HeapPtr<Shape> lastBinding;
     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);
 
     /*
      * Transfers ownership of bindings data from bindings into this fresh
      * Bindings instance. Once such a transfer occurs, the old bindings must
      * not be used again.
      */
@@ -203,19 +205,25 @@ class Bindings {
     uintN countLocalNames() const { return nargs + nvars + nupvars; }
 
     bool hasUpvars() const { return nupvars > 0; }
     bool hasLocalNames() const { return countLocalNames() > 0; }
 
     /* 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);
 
     bool setParent(JSContext *cx, JSObject *obj);
 
     enum {
         /*
@@ -258,16 +266,19 @@ class Bindings {
         *slotp = nargs;
         return add(cx, name, ARGUMENT);
     }
     bool addDestructuring(JSContext *cx, uint16_t *slotp) {
         *slotp = nargs;
         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
      * exists, *indexp will receive the index of the corresponding argument or
      * variable.
      */
     BindingKind lookup(JSContext *cx, JSAtom *name, uintN *indexp) const;
 
--- a/js/src/jsscriptinlines.h
+++ b/js/src/jsscriptinlines.h
@@ -53,17 +53,17 @@
 #include "vm/RegExpObject.h"
 
 #include "jsscopeinlines.h"
 
 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
 Bindings::transfer(JSContext *cx, Bindings *bindings)
 {
     JS_ASSERT(!lastBinding);
     JS_ASSERT(!bindings->lastBinding || !bindings->lastBinding->inDictionary());
 
@@ -85,26 +85,32 @@ Bindings::clone(JSContext *cx, Bindings 
 Shape *
 Bindings::lastShape() const
 {
     JS_ASSERT(lastBinding);
     JS_ASSERT(!lastBinding->inDictionary());
     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. */
-        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);
+        lastBinding = initialShape(cx);
         if (!lastBinding)
             return false;
     }
     return true;
 }
 
 bool
 Bindings::extensibleParents()
--- a/js/src/vm/ScopeObject.cpp
+++ b/js/src/vm/ScopeObject.cpp
@@ -1,9 +1,9 @@
-/* -*- 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 *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
@@ -60,32 +60,33 @@ using namespace js::types;
  * Construct a call object for the given bindings.  If this is a call object
  * for a function invocation, callee should be the function being called.
  * Otherwise it must be a call object for eval of strict mode code, and callee
  * must be null.
  */
 CallObject *
 CallObject::create(JSContext *cx, JSScript *script, JSObject &enclosing, JSObject *callee)
 {
-    Bindings &bindings = script->bindings;
-    gc::AllocKind kind = gc::GetGCObjectKind(bindings.lastShape()->numFixedSlots() + 1);
+    RootedVarShape shape(cx);
+    shape = script->bindings.callObjectShape(cx);
+    if (shape == NULL)
+        return NULL;
+
+    gc::AllocKind kind = gc::GetGCObjectKind(shape->numFixedSlots() + 1);
 
     RootedVarTypeObject type(cx);
 
     type = cx->compartment->getEmptyType(cx);
     if (!type)
         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;
 
     /*
      * Update the parent for bindings associated with non-compileAndGo scripts,
      * whose call objects do not have a consistent global variable and need
      * to be updated dynamically.