From af27f2dc9ce5980f8da2d6197abc3c03083ff2ad Mon Sep 17 00:00:00 2001 From: Luke Wagner Date: Mon, 28 Mar 2011 18:07:06 -0700 Subject: [PATCH 1/3] Bug 587257 - Add missing StringBuffer null check (r=cdleary) --HG-- extra : rebase_source : 1e4684cbe79503cf3a9943ebefc8398be643b909 --- js/src/jsstrinlines.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/js/src/jsstrinlines.h b/js/src/jsstrinlines.h index 91fc2175fe0f..278e1f9f0ab8 100644 --- a/js/src/jsstrinlines.h +++ b/js/src/jsstrinlines.h @@ -189,6 +189,8 @@ inline bool StringBuffer::append(JSString *str) { JSLinearString *linear = str->ensureLinear(context()); + if (!linear) + return false; size_t strLen = linear->length(); if (!checkLength(cb.length() + strLen)) return false; From 85019c95f7fa58df0d5034ad4d8679e4a399b284 Mon Sep 17 00:00:00 2001 From: Luke Wagner Date: Tue, 29 Mar 2011 14:57:56 -0700 Subject: [PATCH 2/3] Bug 587257 - Tidy up array_toSource (r=waldo) --HG-- extra : rebase_source : 4f55cb2db8799d6d5366a5b73a9c56a5e2111a5d --- js/src/jsarray.cpp | 143 ++++++++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 61 deletions(-) diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp index 1744e5b216f4..d63fa5b8e0e6 100644 --- a/js/src/jsarray.cpp +++ b/js/src/jsarray.cpp @@ -1100,18 +1100,57 @@ JSObject::makeDenseArraySlow(JSContext *cx) return true; } -/* Transfer ownership of buffer to returned string. */ -static inline JSBool -BufferToString(JSContext *cx, StringBuffer &sb, Value *rval) -{ - JSString *str = sb.finishString(); - if (!str) - return false; - rval->setString(str); - return true; -} - #if JS_HAS_TOSOURCE +class ArraySharpDetector +{ + JSContext *cx; + jschar *chars; + JSHashEntry *he; + bool sharp; + + public: + ArraySharpDetector(JSContext *cx) + : cx(cx), + chars(NULL), + he(NULL), + sharp(false) + {} + + bool init(JSObject *obj) { + he = js_EnterSharpObject(cx, obj, NULL, &chars); + if (!he) + return false; + sharp = IS_SHARP(he); + return true; + } + + bool initiallySharp() const { + JS_ASSERT_IF(sharp, hasSharpChars()); + return sharp; + } + + void makeSharp() { + MAKE_SHARP(he); + } + + bool hasSharpChars() const { + return chars != NULL; + } + + jschar *takeSharpChars() { + jschar *ret = chars; + chars = NULL; + return ret; + } + + ~ArraySharpDetector() { + if (chars) + cx->free_(chars); + if (he && !sharp) + js_LeaveSharpObject(cx, NULL); + } +}; + static JSBool array_toSource(JSContext *cx, uintN argc, Value *vp) { @@ -1125,55 +1164,43 @@ array_toSource(JSContext *cx, uintN argc, Value *vp) return false; } - /* Find joins or cycles in the reachable object graph. */ - jschar *sharpchars; - JSHashEntry *he = js_EnterSharpObject(cx, obj, NULL, &sharpchars); - if (!he) + ArraySharpDetector detector(cx); + if (!detector.init(obj)) return false; - bool initiallySharp = IS_SHARP(he); - /* After this point, all paths exit through the 'out' label. */ - MUST_FLOW_THROUGH("out"); - bool ok = false; - - /* - * This object will take responsibility for the jschar buffer until the - * buffer is transferred to the returned JSString. - */ StringBuffer sb(cx); - /* Cycles/joins are indicated by sharp objects. */ #if JS_HAS_SHARP_VARS - if (IS_SHARP(he)) { - JS_ASSERT(sharpchars != 0); - sb.replaceRawBuffer(sharpchars, js_strlen(sharpchars)); + if (detector.initiallySharp()) { + jschar *chars = detector.takeSharpChars(); + sb.replaceRawBuffer(chars, js_strlen(chars)); goto make_string; - } else if (sharpchars) { - MAKE_SHARP(he); - sb.replaceRawBuffer(sharpchars, js_strlen(sharpchars)); + } else if (detector.hasSharpChars()) { + detector.makeSharp(); + jschar *chars = detector.takeSharpChars(); + sb.replaceRawBuffer(chars, js_strlen(chars)); } #else - if (IS_SHARP(he)) { + if (detector.initiallySharp()) { if (!sb.append("[]")) - goto out; - cx->free_(sharpchars); + return false; goto make_string; } #endif if (!sb.append('[')) - goto out; + return false; jsuint length; if (!js_GetLengthProperty(cx, obj, &length)) - goto out; + return false; for (jsuint index = 0; index < length; index++) { - /* Use vp to locally root each element value. */ JSBool hole; + Value tmp; if (!JS_CHECK_OPERATION_LIMIT(cx) || - !GetElement(cx, obj, index, &hole, vp)) { - goto out; + !GetElement(cx, obj, index, &hole, &tmp)) { + return false; } /* Get element's character string. */ @@ -1181,42 +1208,34 @@ array_toSource(JSContext *cx, uintN argc, Value *vp) if (hole) { str = cx->runtime->emptyString; } else { - str = js_ValueToSource(cx, *vp); + str = js_ValueToSource(cx, tmp); if (!str) - goto out; + return false; } - vp->setString(str); - - const jschar *chars = str->getChars(cx); - if (!chars) - goto out; /* Append element to buffer. */ - if (!sb.append(chars, chars + str->length())) - goto out; + if (!sb.append(str)) + return false; if (index + 1 != length) { if (!sb.append(", ")) - goto out; + return false; } else if (hole) { if (!sb.append(',')) - goto out; + return false; } } /* Finalize the buffer. */ if (!sb.append(']')) - goto out; + return false; make_string: - if (!BufferToString(cx, sb, vp)) - goto out; + JSString *str = sb.finishString(); + if (!str) + return false; - ok = true; - - out: - if (!initiallySharp) - js_LeaveSharpObject(cx, NULL); - return ok; + JS_SET_RVAL(cx, vp, StringValue(str)); + return true; } #endif @@ -1305,9 +1324,11 @@ array_toString_sub(JSContext *cx, JSObject *obj, JSBool locale, } /* Finalize the buffer. */ - if (!BufferToString(cx, sb, rval)) + JSString *str; + str = sb.finishString(); + if (!str) goto out; - + rval->setString(str); ok = true; out: From a2caff02c73d7e6d3f0383daf97bdad521a52825 Mon Sep 17 00:00:00 2001 From: Luke Wagner Date: Tue, 29 Mar 2011 15:46:51 -0700 Subject: [PATCH 3/3] Bug 587257 - Optimize Array.prototype.join (original patch by Rob Sayre) (r=waldo) --HG-- extra : rebase_source : 9ca67ab914bb604736f6758e7f382f57563baf8f --- js/src/jsarray.cpp | 155 +++++++++++++++----------- js/src/jscntxt.h | 8 +- js/src/jshashtable.h | 10 ++ js/src/json.cpp | 2 +- js/src/jsstr.cpp | 12 +- js/src/jsstr.h | 2 +- js/src/jsstrinlines.h | 12 ++ js/src/tests/ecma/Array/15.4.4.3-2.js | 39 +++++++ js/src/tests/ecma/Array/jstests.list | 1 + 9 files changed, 163 insertions(+), 78 deletions(-) create mode 100644 js/src/tests/ecma/Array/15.4.4.3-2.js diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp index d63fa5b8e0e6..7ca6192d5aa1 100644 --- a/js/src/jsarray.cpp +++ b/js/src/jsarray.cpp @@ -1239,13 +1239,58 @@ array_toSource(JSContext *cx, uintN argc, Value *vp) } #endif +class AutoArrayCycleDetector +{ + JSContext *cx; + JSObject *obj; + uint32 genBefore; + BusyArraysSet::AddPtr hashPointer; + bool cycle; + JS_DECL_USE_GUARD_OBJECT_NOTIFIER + + public: + AutoArrayCycleDetector(JSContext *cx, JSObject *obj JS_GUARD_OBJECT_NOTIFIER_PARAM) + : cx(cx), + obj(obj), + cycle(true) + { + JS_GUARD_OBJECT_NOTIFIER_INIT; + } + + bool init() + { + BusyArraysSet &set = cx->busyArrays; + hashPointer = set.lookupForAdd(obj); + if (!hashPointer) { + if (!set.add(hashPointer, obj)) + return false; + cycle = false; + genBefore = set.generation(); + } + return true; + } + + ~AutoArrayCycleDetector() + { + if (!cycle) { + if (genBefore == cx->busyArrays.generation()) + cx->busyArrays.remove(hashPointer); + else + cx->busyArrays.remove(obj); + } + } + + bool foundCycle() { return cycle; } + + protected: +}; + static JSBool array_toString_sub(JSContext *cx, JSObject *obj, JSBool locale, JSString *sepstr, Value *rval) { JS_CHECK_RECURSION(cx, return false); - /* Get characters to use for the separator. */ static const jschar comma = ','; const jschar *sep; size_t seplen; @@ -1259,84 +1304,68 @@ array_toString_sub(JSContext *cx, JSObject *obj, JSBool locale, seplen = 1; } - /* - * Use HashTable entry as the cycle indicator. On first visit, create the - * entry, and, when leaving, remove the entry. - */ - BusyArraysMap::AddPtr hashp = cx->busyArrays.lookupForAdd(obj); - uint32 genBefore; - if (!hashp) { - /* Not in hash table, so not a cycle. */ - if (!cx->busyArrays.add(hashp, obj)) - return false; - genBefore = cx->busyArrays.generation(); - } else { - /* Cycle, so return empty string. */ + AutoArrayCycleDetector detector(cx, obj); + if (!detector.init()) + return false; + + if (detector.foundCycle()) { rval->setString(cx->runtime->atomState.emptyAtom); return true; } - AutoObjectRooter tvr(cx, obj); - - /* After this point, all paths exit through the 'out' label. */ - MUST_FLOW_THROUGH("out"); - bool ok = false; - - /* - * This object will take responsibility for the jschar buffer until the - * buffer is transferred to the returned JSString. - */ - StringBuffer sb(cx); - jsuint length; if (!js_GetLengthProperty(cx, obj, &length)) - goto out; + return false; - for (jsuint index = 0; index < length; index++) { - /* Use rval to locally root each element value. */ - JSBool hole; - if (!JS_CHECK_OPERATION_LIMIT(cx) || - !GetElement(cx, obj, index, &hole, rval)) { - goto out; + StringBuffer sb(cx); + + if (!locale && !seplen && obj->isDenseArray() && !js_PrototypeHasIndexedProperties(cx, obj)) { + /* Elements beyond 'capacity' are 'undefined' and thus can be ignored. */ + Value *beg = obj->getDenseArrayElements(); + Value *end = beg + Min(length, obj->getDenseArrayCapacity()); + for (Value *vp = beg; vp != end; ++vp) { + if (!JS_CHECK_OPERATION_LIMIT(cx)) + return false; + + if (!vp->isMagic(JS_ARRAY_HOLE) && !vp->isNullOrUndefined()) { + if (!ValueToStringBuffer(cx, *vp, sb)) + return false; + } } + } else { + for (jsuint index = 0; index < length; index++) { + if (!JS_CHECK_OPERATION_LIMIT(cx)) + return false; - /* Get element's character string. */ - if (!hole && !rval->isNullOrUndefined()) { - if (locale) { - /* Work on obj.toLocalString() instead. */ - JSObject *robj = ToObject(cx, rval); - if (!robj) - goto out; - jsid id = ATOM_TO_JSID(cx->runtime->atomState.toLocaleStringAtom); - if (!robj->callMethod(cx, id, 0, NULL, rval)) - goto out; + JSBool hole; + if (!GetElement(cx, obj, index, &hole, rval)) + return false; + + if (!hole && !rval->isNullOrUndefined()) { + if (locale) { + JSObject *robj = ToObject(cx, rval); + if (!robj) + return false; + jsid id = ATOM_TO_JSID(cx->runtime->atomState.toLocaleStringAtom); + if (!robj->callMethod(cx, id, 0, NULL, rval)) + return false; + } + if (!ValueToStringBuffer(cx, *rval, sb)) + return false; } - if (!ValueToStringBuffer(cx, *rval, sb)) - goto out; - } - - /* Append the separator. */ - if (index + 1 != length) { - if (!sb.append(sep, seplen)) - goto out; + if (index + 1 != length) { + if (!sb.append(sep, seplen)) + return false; + } } } - /* Finalize the buffer. */ - JSString *str; - str = sb.finishString(); + JSString *str = sb.finishString(); if (!str) - goto out; + return false; rval->setString(str); - ok = true; - - out: - if (genBefore == cx->busyArrays.generation()) - cx->busyArrays.remove(hashp); - else - cx->busyArrays.remove(obj); - return ok; + return true; } /* ES5 15.4.4.2. NB: The algorithm here differs from the one in ES3. */ diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h index 75da5cde32e2..70fe473800b3 100644 --- a/js/src/jscntxt.h +++ b/js/src/jscntxt.h @@ -1618,9 +1618,9 @@ VersionIsKnown(JSVersion version) return VersionNumber(version) != JSVERSION_UNKNOWN; } -typedef js::HashSet, - js::SystemAllocPolicy> BusyArraysMap; +typedef HashSet, + SystemAllocPolicy> BusyArraysSet; } /* namespace js */ @@ -1714,7 +1714,7 @@ struct JSContext /* State for object and array toSource conversion. */ JSSharpObjectMap sharpObjectMap; - js::BusyArraysMap busyArrays; + js::BusyArraysSet busyArrays; /* Argument formatter support for JS_{Convert,Push}Arguments{,VA}. */ JSArgumentFormatMap *argumentFormatMap; diff --git a/js/src/jshashtable.h b/js/src/jshashtable.h index 725c4c7748eb..2dc2925938c2 100644 --- a/js/src/jshashtable.h +++ b/js/src/jshashtable.h @@ -120,6 +120,13 @@ class HashTable : private AllocPolicy Ptr(Entry &entry) : entry(&entry) {} public: + /* Leaves Ptr uninitialized. */ + Ptr() { +#ifdef DEBUG + entry = (Entry *)0xbad; +#endif + } + bool found() const { return entry->isLive(); } operator ConvertibleToBool() const { return found() ? &Ptr::nonNull : 0; } bool operator==(const Ptr &rhs) const { JS_ASSERT(found() && rhs.found()); return entry == rhs.entry; } @@ -142,6 +149,9 @@ class HashTable : private AllocPolicy #else AddPtr(Entry &entry, HashNumber hn) : Ptr(entry), keyHash(hn) {} #endif + public: + /* Leaves AddPtr uninitialized. */ + AddPtr() {} }; /* diff --git a/js/src/json.cpp b/js/src/json.cpp index e2c7c6b9ed6b..5c2379d47414 100644 --- a/js/src/json.cpp +++ b/js/src/json.cpp @@ -240,7 +240,7 @@ public: } if (gapValue.value().isString()) { - if (!ValueToStringBuffer(cx, gapValue.value(), gap)) + if (!gap.append(gapValue.value().toString())) return false; if (gap.length() > 10) gap.resize(10); diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp index 8470969c9d81..8f0350dc16aa 100644 --- a/js/src/jsstr.cpp +++ b/js/src/jsstr.cpp @@ -3859,20 +3859,14 @@ js_ValueToString(JSContext *cx, const Value &arg) /* This function implements E-262-3 section 9.8, toString. */ bool -js::ValueToStringBuffer(JSContext *cx, const Value &arg, StringBuffer &sb) +js::ValueToStringBufferSlow(JSContext *cx, const Value &arg, StringBuffer &sb) { Value v = arg; if (v.isObject() && !DefaultValue(cx, &v.toObject(), JSTYPE_STRING, &v)) return false; - if (v.isString()) { - JSString *str = v.toString(); - size_t length = str->length(); - const jschar *chars = str->getChars(cx); - if (!chars) - return false; - return sb.append(chars, length); - } + if (v.isString()) + return sb.append(v.toString()); if (v.isNumber()) return NumberValueToStringBuffer(cx, v, sb); if (v.isBoolean()) diff --git a/js/src/jsstr.h b/js/src/jsstr.h index 33aac3125ab5..2ae5eb605932 100644 --- a/js/src/jsstr.h +++ b/js/src/jsstr.h @@ -1009,7 +1009,7 @@ ValueToString_TestForStringInline(JSContext *cx, const Value &v) * value to a string of jschars appended to the given buffer. On error, the * passed buffer may have partial results appended. */ -extern bool +inline bool ValueToStringBuffer(JSContext *cx, const Value &v, StringBuffer &sb); } /* namespace js */ diff --git a/js/src/jsstrinlines.h b/js/src/jsstrinlines.h index 278e1f9f0ab8..5b72a734b8ac 100644 --- a/js/src/jsstrinlines.h +++ b/js/src/jsstrinlines.h @@ -243,6 +243,18 @@ StringBuffer::checkLength(size_t length) return CheckStringLength(context(), length); } +extern bool +ValueToStringBufferSlow(JSContext *cx, const Value &v, StringBuffer &sb); + +inline bool +ValueToStringBuffer(JSContext *cx, const Value &v, StringBuffer &sb) +{ + if (v.isString()) + return sb.append(v.toString()); + + return ValueToStringBufferSlow(cx, v, sb); +} + } /* namespace js */ JS_ALWAYS_INLINE void diff --git a/js/src/tests/ecma/Array/15.4.4.3-2.js b/js/src/tests/ecma/Array/15.4.4.3-2.js new file mode 100644 index 000000000000..9b01737c2f66 --- /dev/null +++ b/js/src/tests/ecma/Array/15.4.4.3-2.js @@ -0,0 +1,39 @@ +var arr = [0,1,,3,4]; +Object.prototype[2] = 2; + +assertEq(arr.join(""), "01234"); +assertEq(arr.join(","), "0,1,2,3,4"); + +arr[2] = "porkchops"; +assertEq(arr.join("*"), "0*1*porkchops*3*4"); + +delete Object.prototype[2]; +assertEq(arr.join("*"), "0*1*porkchops*3*4"); + +delete arr[2]; +assertEq(arr.join("*"), "0*1**3*4"); + +Object.prototype[2] = null; +assertEq(arr.join("*"), "0*1**3*4"); +Object.prototype[2] = undefined; +assertEq(arr.join("*"), "0*1**3*4"); +arr[2] = null; +assertEq(arr.join("*"), "0*1**3*4"); +arr[2] = undefined; +assertEq(arr.join("*"), "0*1**3*4"); + +var arr = new Array(10); +assertEq(arr.join(""), ""); +assertEq(arr.join(), ",,,,,,,,,"); +assertEq(arr.join("|"), "|||||||||"); + +arr[2] = "doubt"; +assertEq(arr.join(","), ",,doubt,,,,,,,"); + +arr[9] = "failure"; +assertEq(arr.join(","), ",,doubt,,,,,,,failure"); + +delete arr[2]; +assertEq(arr.join(","), ",,,,,,,,,failure"); + +reportCompare(true, true); diff --git a/js/src/tests/ecma/Array/jstests.list b/js/src/tests/ecma/Array/jstests.list index f3f05e3c0268..e992ece4602a 100644 --- a/js/src/tests/ecma/Array/jstests.list +++ b/js/src/tests/ecma/Array/jstests.list @@ -16,6 +16,7 @@ script 15.4.3.2.js script 15.4.4.1.js script 15.4.4.2.js script 15.4.4.3-1.js +script 15.4.4.3-2.js script 15.4.4.4-1.js script 15.4.4.4-2.js script 15.4.4.5-1.js