From d10e9a8943f06cbf6165b5f072f1ef0a28798d13 Mon Sep 17 00:00:00 2001 From: Igor Bukanov Date: Fri, 11 Feb 2011 14:58:53 +0100 Subject: [PATCH] bug 632003 - var declarations should ignore prototype properties, r=jwalden --- js/src/jsemit.cpp | 7 +- js/src/jsinterp.cpp | 151 +++++++----------- js/src/jsinterp.h | 5 +- js/src/jsobj.cpp | 4 +- js/src/jsutil.h | 3 + js/src/methodjit/StubCalls.cpp | 39 ++--- js/src/tests/ecma_5/misc/jstests.list | 1 + js/src/tests/ecma_5/misc/regress-bug632003.js | 63 ++++++++ 8 files changed, 145 insertions(+), 128 deletions(-) create mode 100644 js/src/tests/ecma_5/misc/regress-bug632003.js diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp index 05c60d8fb578..df4125777e8b 100644 --- a/js/src/jsemit.cpp +++ b/js/src/jsemit.cpp @@ -6984,10 +6984,9 @@ js_EmitTree(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn) if (obj) { JS_ASSERT(!obj->inDictionaryMode()); - JSProperty *prop = NULL; - if (!js_DefineNativeProperty(cx, obj, - ATOM_TO_JSID(pn3->pn_atom), UndefinedValue(), NULL, NULL, - JSPROP_ENUMERATE, 0, 0, &prop, 0)) { + if (!js_DefineNativeProperty(cx, obj, ATOM_TO_JSID(pn3->pn_atom), + UndefinedValue(), NULL, NULL, + JSPROP_ENUMERATE, 0, 0, NULL)) { return JS_FALSE; } if (obj->inDictionaryMode()) diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp index b6d221075854..507ba3e64ee1 100644 --- a/js/src/jsinterp.cpp +++ b/js/src/jsinterp.cpp @@ -1013,30 +1013,14 @@ Execute(JSContext *cx, JSObject *chain, JSScript *script, } bool -CheckRedeclaration(JSContext *cx, JSObject *obj, jsid id, uintN attrs, - JSObject **objp, JSProperty **propp) +CheckRedeclaration(JSContext *cx, JSObject *obj, jsid id, uintN attrs) { JSObject *obj2; JSProperty *prop; - uintN oldAttrs, report; + uintN oldAttrs; bool isFunction; const char *type, *name; - /* - * Both objp and propp must be either null or given. When given, *propp - * must be null. This way we avoid an extra "if (propp) *propp = NULL" for - * the common case of a nonexistent property. - */ - JS_ASSERT(!objp == !propp); - JS_ASSERT_IF(propp, !*propp); - - /* The JSPROP_INITIALIZER case below may generate a warning. Since we must - * drop the property before reporting it, we insists on !propp to avoid - * looking up the property again after the reporting is done. - */ - JS_ASSERT_IF(attrs & JSPROP_INITIALIZER, attrs == JSPROP_INITIALIZER); - JS_ASSERT_IF(attrs == JSPROP_INITIALIZER, !propp); - if (!obj->lookupProperty(cx, id, &obj2, &prop)) return false; if (!prop) @@ -1048,64 +1032,40 @@ CheckRedeclaration(JSContext *cx, JSObject *obj, jsid id, uintN attrs, return false; } - if (!propp) { - prop = NULL; - } else { - *objp = obj2; - *propp = prop; + /* We allow redeclaring some non-readonly properties. */ + if (((oldAttrs | attrs) & JSPROP_READONLY) == 0) { + /* Allow redeclaration of variables and functions. */ + if (!(attrs & (JSPROP_GETTER | JSPROP_SETTER))) + return true; + + /* + * Allow adding a getter only if a property already has a setter + * but no getter and similarly for adding a setter. That is, we + * allow only the following transitions: + * + * no-property --> getter --> getter + setter + * no-property --> setter --> getter + setter + */ + if ((~(oldAttrs ^ attrs) & (JSPROP_GETTER | JSPROP_SETTER)) == 0) + return true; + + /* + * Allow redeclaration of an impermanent property (in which case + * anyone could delete it and redefine it, willy-nilly). + */ + if (!(oldAttrs & JSPROP_PERMANENT)) + return true; } - if (attrs == JSPROP_INITIALIZER) { - /* Allow the new object to override properties. */ - if (obj2 != obj) - return JS_TRUE; - - /* The property must be dropped already. */ - JS_ASSERT(!prop); - report = JSREPORT_WARNING | JSREPORT_STRICT; - -#ifdef __GNUC__ - isFunction = false; /* suppress bogus gcc warnings */ -#endif - } else { - /* We allow redeclaring some non-readonly properties. */ - if (((oldAttrs | attrs) & JSPROP_READONLY) == 0) { - /* Allow redeclaration of variables and functions. */ - if (!(attrs & (JSPROP_GETTER | JSPROP_SETTER))) - return JS_TRUE; - - /* - * Allow adding a getter only if a property already has a setter - * but no getter and similarly for adding a setter. That is, we - * allow only the following transitions: - * - * no-property --> getter --> getter + setter - * no-property --> setter --> getter + setter - */ - if ((~(oldAttrs ^ attrs) & (JSPROP_GETTER | JSPROP_SETTER)) == 0) - return JS_TRUE; - - /* - * Allow redeclaration of an impermanent property (in which case - * anyone could delete it and redefine it, willy-nilly). - */ - if (!(oldAttrs & JSPROP_PERMANENT)) - return JS_TRUE; - } - - report = JSREPORT_ERROR; - isFunction = (oldAttrs & (JSPROP_GETTER | JSPROP_SETTER)) != 0; - if (!isFunction) { - Value value; - if (!obj->getProperty(cx, id, &value)) - return JS_FALSE; - isFunction = IsFunctionObject(value); - } + isFunction = (oldAttrs & (JSPROP_GETTER | JSPROP_SETTER)) != 0; + if (!isFunction) { + Value value; + if (!obj->getProperty(cx, id, &value)) + return JS_FALSE; + isFunction = IsFunctionObject(value); } - type = (attrs == JSPROP_INITIALIZER) - ? "property" - : (oldAttrs & attrs & JSPROP_GETTER) + type = (oldAttrs & attrs & JSPROP_GETTER) ? js_getter_str : (oldAttrs & attrs & JSPROP_SETTER) ? js_setter_str @@ -1117,11 +1077,10 @@ CheckRedeclaration(JSContext *cx, JSObject *obj, jsid id, uintN attrs, JSAutoByteString bytes; name = js_ValueToPrintable(cx, IdToValue(id), &bytes); if (!name) - return JS_FALSE; - return !!JS_ReportErrorFlagsAndNumber(cx, report, - js_GetErrorMessage, NULL, - JSMSG_REDECLARED_VAR, - type, name); + return false; + JS_ALWAYS_FALSE(JS_ReportErrorFlagsAndNumber(cx, JSREPORT_ERROR, js_GetErrorMessage, NULL, + JSMSG_REDECLARED_VAR, type, name)); + return false; } JSBool @@ -5324,33 +5283,38 @@ BEGIN_CASE(JSOP_DEFVAR) uintN attrs = JSPROP_ENUMERATE; if (!regs.fp->isEvalFrame()) attrs |= JSPROP_PERMANENT; - if (op == JSOP_DEFCONST) - attrs |= JSPROP_READONLY; /* Lookup id in order to check for redeclaration problems. */ jsid id = ATOM_TO_JSID(atom); - JSProperty *prop = NULL; - JSObject *obj2; + bool shouldDefine; if (op == JSOP_DEFVAR) { /* * Redundant declaration of a |var|, even one for a non-writable * property like |undefined| in ES5, does nothing. */ + JSProperty *prop; + JSObject *obj2; if (!obj->lookupProperty(cx, id, &obj2, &prop)) goto error; + shouldDefine = (!prop || obj2 != obj); } else { - if (!CheckRedeclaration(cx, obj, id, attrs, &obj2, &prop)) + JS_ASSERT(op == JSOP_DEFCONST); + attrs |= JSPROP_READONLY; + if (!CheckRedeclaration(cx, obj, id, attrs)) goto error; + + /* + * As attrs includes readonly, CheckRedeclaration can succeed only + * if prop does not exist. + */ + shouldDefine = true; } /* Bind a variable only if it's not yet defined. */ - if (!prop) { - if (!js_DefineNativeProperty(cx, obj, id, UndefinedValue(), - PropertyStub, StrictPropertyStub, attrs, 0, 0, &prop)) { - goto error; - } - JS_ASSERT(prop); - obj2 = obj; + if (shouldDefine && + !js_DefineNativeProperty(cx, obj, id, UndefinedValue(), + PropertyStub, StrictPropertyStub, attrs, 0, 0, NULL)) { + goto error; } } END_CASE(JSOP_DEFVAR) @@ -5398,7 +5362,6 @@ BEGIN_CASE(JSOP_DEFFUN) goto error; } - /* * ECMA requires functions defined when entering Eval code to be * impermanent. @@ -5486,7 +5449,7 @@ BEGIN_CASE(JSOP_DEFFUN_DBGFC) JSObject &parent = regs.fp->varobj(cx); jsid id = ATOM_TO_JSID(fun->atom); - if (!CheckRedeclaration(cx, &parent, id, attrs, NULL, NULL)) + if (!CheckRedeclaration(cx, &parent, id, attrs)) goto error; if ((attrs == JSPROP_ENUMERATE) @@ -5826,7 +5789,7 @@ BEGIN_CASE(JSOP_SETTER) attrs |= JSPROP_ENUMERATE | JSPROP_SHARED; /* Check for a readonly or permanent property of the same name. */ - if (!CheckRedeclaration(cx, obj, id, attrs, NULL, NULL)) + if (!CheckRedeclaration(cx, obj, id, attrs)) goto error; if (!obj->defineProperty(cx, id, UndefinedValue(), getter, setter, attrs)) @@ -5960,8 +5923,6 @@ BEGIN_CASE(JSOP_INITMETHOD) LOAD_ATOM(0, atom); jsid id = ATOM_TO_JSID(atom); - /* No need to check for duplicate property; the compiler already did. */ - uintN defineHow = (op == JSOP_INITMETHOD) ? JSDNP_CACHE_RESULT | JSDNP_SET_METHOD : JSDNP_CACHE_RESULT; @@ -5994,8 +5955,6 @@ BEGIN_CASE(JSOP_INITELEM) jsid id; FETCH_ELEMENT_ID(obj, -2, id); - /* No need to check for duplicate property; the compiler already did. */ - /* * If rref is a hole, do not call JSObject::defineProperty. In this case, * obj must be an array, so if the current op is the last element diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h index 948f1779f6a6..6e6761c65cf5 100644 --- a/js/src/jsinterp.h +++ b/js/src/jsinterp.h @@ -997,11 +997,8 @@ Interpret(JSContext *cx, JSStackFrame *stopFp, uintN inlineCallCount = 0, JSInte extern JS_REQUIRES_STACK bool RunScript(JSContext *cx, JSScript *script, JSStackFrame *fp); -#define JSPROP_INITIALIZER 0x100 /* NB: Not a valid property attribute. */ - extern bool -CheckRedeclaration(JSContext *cx, JSObject *obj, jsid id, uintN attrs, - JSObject **objp, JSProperty **propp); +CheckRedeclaration(JSContext *cx, JSObject *obj, jsid id, uintN attrs); extern bool StrictlyEqual(JSContext *cx, const Value &lval, const Value &rval, JSBool *equal); diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp index 14db8ecfa66e..68c30377387c 100644 --- a/js/src/jsobj.cpp +++ b/js/src/jsobj.cpp @@ -1601,7 +1601,7 @@ js_obj_defineGetter(JSContext *cx, uintN argc, Value *vp) jsid id; if (!ValueToId(cx, vp[2], &id)) return JS_FALSE; - if (!CheckRedeclaration(cx, obj, id, JSPROP_GETTER, NULL, NULL)) + if (!CheckRedeclaration(cx, obj, id, JSPROP_GETTER)) return JS_FALSE; /* * Getters and setters are just like watchpoints from an access @@ -1634,7 +1634,7 @@ js_obj_defineSetter(JSContext *cx, uintN argc, Value *vp) jsid id; if (!ValueToId(cx, vp[2], &id)) return JS_FALSE; - if (!CheckRedeclaration(cx, obj, id, JSPROP_SETTER, NULL, NULL)) + if (!CheckRedeclaration(cx, obj, id, JSPROP_SETTER)) return JS_FALSE; /* * Getters and setters are just like watchpoints from an access diff --git a/js/src/jsutil.h b/js/src/jsutil.h index 936ae4b1cfaa..9da2674704c4 100644 --- a/js/src/jsutil.h +++ b/js/src/jsutil.h @@ -78,6 +78,8 @@ JS_Assert(const char *s, const char *file, JSIntn ln); #define JS_ALWAYS_TRUE(expr) JS_ASSERT(expr) +#define JS_ALWAYS_FALSE(expr) JS_ASSERT(!(expr)) + # ifdef JS_THREADSAFE # define JS_THREADSAFE_ASSERT(expr) JS_ASSERT(expr) # else @@ -90,6 +92,7 @@ JS_Assert(const char *s, const char *file, JSIntn ln); #define JS_ASSERT_IF(cond,expr) ((void) 0) #define JS_NOT_REACHED(reason) #define JS_ALWAYS_TRUE(expr) ((void) (expr)) +#define JS_ALWAYS_FALSE(expr) ((void) (expr)) #define JS_THREADSAFE_ASSERT(expr) ((void) 0) #endif /* defined(DEBUG) */ diff --git a/js/src/methodjit/StubCalls.cpp b/js/src/methodjit/StubCalls.cpp index 7f62b76dd51d..abaffd825355 100644 --- a/js/src/methodjit/StubCalls.cpp +++ b/js/src/methodjit/StubCalls.cpp @@ -1339,13 +1339,6 @@ stubs::InitElem(VMFrame &f, uint32 last) if (!FetchElementId(f, obj, idval, id, ®s.sp[-2])) THROW(); - /* - * Check for property redeclaration strict warning (we may be in an object - * initialiser, not an array initialiser). - */ - if (!CheckRedeclaration(cx, obj, id, JSPROP_INITIALIZER, NULL, NULL)) - THROW(); - /* * If rref is a hole, do not call JSObject::defineProperty. In this case, * obj must be an array, so if the current op is the last element @@ -2128,8 +2121,6 @@ InitPropOrMethod(VMFrame &f, JSAtom *atom, JSOp op) /* Get the immediate property name into id. */ jsid id = ATOM_TO_JSID(atom); - /* No need to check for duplicate property; the compiler already did. */ - uintN defineHow = (op == JSOP_INITMETHOD) ? JSDNP_CACHE_RESULT | JSDNP_SET_METHOD : JSDNP_CACHE_RESULT; @@ -2589,34 +2580,38 @@ stubs::DefVarOrConst(VMFrame &f, JSAtom *atom) uintN attrs = JSPROP_ENUMERATE; if (!fp->isEvalFrame()) attrs |= JSPROP_PERMANENT; - if (JSOp(*f.regs.pc) == JSOP_DEFCONST) - attrs |= JSPROP_READONLY; /* Lookup id in order to check for redeclaration problems. */ jsid id = ATOM_TO_JSID(atom); - JSProperty *prop = NULL; - JSObject *obj2; - + bool shouldDefine; if (JSOp(*f.regs.pc) == JSOP_DEFVAR) { /* * Redundant declaration of a |var|, even one for a non-writable * property like |undefined| in ES5, does nothing. */ + JSProperty *prop; + JSObject *obj2; if (!obj->lookupProperty(cx, id, &obj2, &prop)) THROW(); + shouldDefine = (!prop || obj2 != obj); } else { - if (!CheckRedeclaration(cx, obj, id, attrs, &obj2, &prop)) + JS_ASSERT(JSOp(*f.regs.pc) == JSOP_DEFCONST); + attrs |= JSPROP_READONLY; + if (!CheckRedeclaration(cx, obj, id, attrs)) THROW(); + + /* + * As attrs includes readonly, CheckRedeclaration can succeed only + * if prop does not exist. + */ + shouldDefine = true; } /* Bind a variable only if it's not yet defined. */ - if (!prop) { - if (!js_DefineNativeProperty(cx, obj, id, UndefinedValue(), - PropertyStub, StrictPropertyStub, attrs, 0, 0, &prop)) { - THROW(); - } - JS_ASSERT(prop); - obj2 = obj; + if (shouldDefine && + !js_DefineNativeProperty(cx, obj, id, UndefinedValue(), PropertyStub, StrictPropertyStub, + attrs, 0, 0, NULL)) { + THROW(); } } diff --git a/js/src/tests/ecma_5/misc/jstests.list b/js/src/tests/ecma_5/misc/jstests.list index c9e15136970a..3cf2050b788d 100644 --- a/js/src/tests/ecma_5/misc/jstests.list +++ b/js/src/tests/ecma_5/misc/jstests.list @@ -8,3 +8,4 @@ script function-definition-eval.js skip-if(!xulRuntime.shell) script function-definition-evaluate.js # needs evaluate() script future-reserved-words.js script builtin-methods-reject-null-undefined-this.js +script regress-bug632003.js diff --git a/js/src/tests/ecma_5/misc/regress-bug632003.js b/js/src/tests/ecma_5/misc/regress-bug632003.js new file mode 100644 index 000000000000..8663eb68830d --- /dev/null +++ b/js/src/tests/ecma_5/misc/regress-bug632003.js @@ -0,0 +1,63 @@ +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* + * Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/licenses/publicdomain/ + * Contributor: + */ +var BUGNUMBER = 632003; +var summary = 'The var statement should add the property to the global if it exists on the prototype'; + +// Define properties on Object.prototype with various attributes and +// value-getter-setter combinations then check that a var statement +// can always define a variable with the same name in the global object. + +if (typeof evaluate != "undefined") { + var global_case = def_all("global_case"); + evaluate(global_case.source); + check_values(this, global_case.var_list); +} + +var eval_case = def_all("eval_case"); +eval(eval_case.source); +check_values(this, eval_case.var_list); + +function def_all(prefix) +{ + var builder, index, i, j; + + builder = {source: "", var_list: []}; + index = 0; + for (i = 0; i <= 1; ++i) { + for (j = 0; j <= 1; ++j) { + def({value: index}); + def({value: index, writable: true}); + def({get: Function("return "+index+";")}); + def({set: function() { }}); + def({get: Function("return "+index+";"), set: function() { }}); + } + } + return builder; + + function def(descriptor_seed) + { + var var_name = prefix + index; + descriptor_seed.configurable = !!i; + descriptor_seed.enumerable = !!j; + Object.defineProperty(Object.prototype, var_name, descriptor_seed); + var var_value = index + 0.5; + builder.source += "var "+var_name+" = "+var_value+";\n"; + builder.var_list.push({name: var_name, expected_value: var_value}); + ++index; + } +} + +function check_values(obj, var_list) +{ + for (i = 0; i != var_list.length; ++i) { + var name = var_list[i].name; + assertEq(obj.hasOwnProperty(name), true); + assertEq(obj[name], var_list[i].expected_value); + } +} + +reportCompare(true, true);