These appear to mostly be ordered oddly: e.g. we call ToInteger after unpacking the chars from the string. If we just reversed these steps then we would be fine.

This basically turned into updating the 4 affected methods to newer (and presumably better) spec steps.

Comment on attachment 670190 [details] [diff] [review] v0 Review of attachment 670190 [details] [diff] [review]: ----------------------------------------------------------------- contains/startsWith/endsWith are using uint32_t for string lengths. I think those should be |size_t|. ::: js/src/jsapi.h @@ +49,5 @@ > namespace JS { > > +typedef mozilla::RangedPtr<const jschar> CharPtr; > + > +class StableCharPtr : public CharPtr { AFAICT, this thing is never used. ::: js/src/jsstr.cpp @@ +1132,5 @@ > +static inline bool > +GetPositionFromArg(JSContext *cx, const CallArgs &args, int argNum, uint32_t *pos) > +{ > + AssertCanGC(); > + JS_ASSERT(pos); Why not just use a reference for pos? @@ +1136,5 @@ > + JS_ASSERT(pos); > + if (args.hasDefined(argNum)) { > + if (args[argNum].isInt32()) { > + int i = args[argNum].toInt32(); > + *pos = (i < 0) ? 0U : uint32_t(i); Maybe this is more clearly written in terms of Max? @@ +1178,5 @@ > + > + AutoAssertNoGC nogc; > + > + // Step 9 > + uint32_t start = Min(Max(pos, 0U), textLen); Doesn't GetPositionFromArg already force pos >= 0? @@ +1352,5 @@ > + uint32_t searchLen = searchStr->length(); > + UnstableCharPtr searchChars(searchStr->chars(), searchLen); > + > + // Step 11 > + if (searchLen + start < searchLen || searchLen + start > textLen) { This looks wrong. How is searchLen + [unsigned int] every going to be less than searchLen? The second condition could be an overflow danger. @@ +1375,5 @@ > if (!str) > return false; > > + // Steps 4 and 5 > + Rooted<JSLinearString *> searchStr(cx, ArgToRootedString(cx, args, 0)); You moved the "*" onto the type here but not elsewhere.

(In reply to Benjamin Peterson [:benjamin] from comment #2) > Review of attachment 670190 [details] [diff] [review]: > ----------------------------------------------------------------- > > contains/startsWith/endsWith are using uint32_t for string lengths. I think > those should be |size_t|. The size of a string is substantially less than uint32_t: (from vm/String.h): static const size_t MAX_LENGTH = JS_BIT(32 - LENGTH_SHIFT) - 1; We store the lengthAndFlags in a size_t because (I presume) we want the jschar* after it to be word aligned on both 32bit and 64bit. For this particular usage, I prefer the sized type to minimize the possibility of running into platform-specific spec violations caused by overflows. > ::: js/src/jsapi.h > @@ +49,5 @@ > > namespace JS { > > > > +typedef mozilla::RangedPtr<const jschar> CharPtr; > > + > > +class StableCharPtr : public CharPtr { > > AFAICT, this thing is never used. Ah, I copied that hunk in from a patch that was not landed at the time. Thanks for reminding me to rebase. > ::: js/src/jsstr.cpp > @@ +1132,5 @@ > > +static inline bool > > +GetPositionFromArg(JSContext *cx, const CallArgs &args, int argNum, uint32_t *pos) > > +{ > > + AssertCanGC(); > > + JS_ASSERT(pos); > > Why not just use a reference for pos? I'm a bit torn on this. On one hand, I want to make it obvious at the call sites that |pos| is an out-param. On the other hand, I'd like to make it obvious at the implementation site that |pos| cannot be NULL. Waldo, what is your preference? > @@ +1136,5 @@ > > + JS_ASSERT(pos); > > + if (args.hasDefined(argNum)) { > > + if (args[argNum].isInt32()) { > > + int i = args[argNum].toInt32(); > > + *pos = (i < 0) ? 0U : uint32_t(i); > > Maybe this is more clearly written in terms of Max? Hmm. Good thought. > @@ +1178,5 @@ > > + > > + AutoAssertNoGC nogc; > > + > > + // Step 9 > > + uint32_t start = Min(Max(pos, 0U), textLen); > > Doesn't GetPositionFromArg already force pos >= 0? The goal here is to follow spec steps as closely as possible where we can do so without crippling performance. This spot is okay because all the args are unsigned: all compilers will elide the unsigned comparison with 0. > @@ +1352,5 @@ > > + uint32_t searchLen = searchStr->length(); > > + UnstableCharPtr searchChars(searchStr->chars(), searchLen); > > + > > + // Step 11 > > + if (searchLen + start < searchLen || searchLen + start > textLen) { > > This looks wrong. How is searchLen + [unsigned int] every going to be less > than searchLen? The second condition could be an overflow danger. This is an overflow check. Note: this does not trigger undefined behavior because the arguments are unsigned. If the first operation overflows, then we want to skip the rest of the function for one of two reasons: (1) if MatchString is coded poorly then this will overflow and be a sec-crit bug or (2) we will have a spec violation when we successfully recognized strings that should not be recognized. > @@ +1375,5 @@ > > if (!str) > > return false; > > > > + // Steps 4 and 5 > > + Rooted<JSLinearString *> searchStr(cx, ArgToRootedString(cx, args, 0)); > > You moved the "*" onto the type here but not elsewhere. Good catch.

(In reply to Terrence Cole [:terrence] from comment #3) > > > +static inline bool > > > +GetPositionFromArg(JSContext *cx, const CallArgs &args, int argNum, uint32_t *pos) > > > +{ > > > + AssertCanGC(); > > > + JS_ASSERT(pos); > > > > Why not just use a reference for pos? > > I'm a bit torn on this. On one hand, I want to make it obvious at the call > sites that |pos| is an out-param. On the other hand, I'd like to make it > obvious at the implementation site that |pos| cannot be NULL. Waldo, what > is your preference? I believe SpiderMonkey preference is for |pos| as an outparam to be a pointer, for the obvious-at-call-sites reason.

Comment on attachment 670190 [details] [diff] [review] v0 Review of attachment 670190 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay on this, I've been kind of swamped lately. Didn't help I had most of my comments written on the laptop currently getting serviced, stored in my browser profile. I wonder why we can't store this stuff server-side as well as client-side, or something... ::: js/src/jsapi.h @@ +49,5 @@ > namespace JS { > > +typedef mozilla::RangedPtr<const jschar> CharPtr; > + > +class StableCharPtr : public CharPtr { { on new line. @@ +59,5 @@ > + : CharPtr(pos, start, len) > + {} > +}; > + > +class UnstableCharPtr : public CharPtr { { on new line. ::: js/src/jsstr.cpp @@ +1132,5 @@ > +static inline bool > +GetPositionFromArg(JSContext *cx, const CallArgs &args, int argNum, uint32_t *pos) > +{ > + AssertCanGC(); > + JS_ASSERT(pos); Too bad C++ doesn't have an annotation for non-null pointers, or C#'s (?) ref foo syntax for passing references, or something so that we could have non-null without using a pointer. @@ +1166,5 @@ > return false; > > + // Steps 6 and 7 > + uint32_t pos = 0; > + if (!GetPositionFromArg(cx, args, 1, &pos)) This method seems to obfuscate rather more than it helps, alas. See, for example, Benjamin's pos >= 0 comment. I would rather not see this helper method added for any of these methods, for all that it's usually nice to share code. @@ +1216,5 @@ > return false; > > + // Step 8 > + uint32_t textLen = str->length(); > + UnstableCharPtr textChars(str->getChars(cx), textLen); Yeah, we definitely want that Range concept luke mentioned here, so str->getChars() could return a Range, and not have two separate variables to juggle, and pass around mostly redundantly. @@ +1217,5 @@ > > + // Step 8 > + uint32_t textLen = str->length(); > + UnstableCharPtr textChars(str->getChars(cx), textLen); > + if (!textChars.get()) We should add a private typedef void* RangedPtr::* ConvertibleToBool; and a public operator ConvertibleToBool() const { return ptr ? reinterpret_cast<ConvertibleToBool>(1) : nullptr; } to RangedPtr. There's no reason that class need not have bool conversion semantics. @@ +1232,5 @@ > + > + // Step 11 > + textChars += start; > + textLen -= start; > + int match = StringMatch(textChars.get(), textLen, searchChars.get(), searchLen); StringMatch would definitely be nicer if it took Ranges rather than raw pointers and lengths. We need Range! Please do this as a followup bug. @@ +1314,5 @@ > args.rval().setInt32(-1); > return true; > } > > +/* ES6 20120927 draft 15.5.4.22. */ You can update this to 20121026 now, and in the other place. @@ +1359,4 @@ > } > > + // Steps 12 and 13 > + args.rval().setBoolean(PodEqual((textChars + start).get(), searchChars.get(), searchLen)); PodEqual would be good with range support, too. The more we propagate this, the more we get in-range assertion checks. :-) ::: mfbt/RangedPtr.h @@ +127,5 @@ > } > > RangedPtr<T> operator+(size_t inc) { > MOZ_ASSERT(inc <= size_t(-1) / sizeof(T)); > + MOZ_ASSERT(ptr + inc >= ptr); Hmm, it occurs to me that there are no defined overflow semantics on pointers, because pointers are only valid within the range of their pointee. So this (and what existed before it) doesn't actually work. How about adding this: uintptr_t asUintptr() const { return uintptr_t(ptr); } as a private method in RangedPtr, then having MOZ_ASSERT(asUintptr() + inc * sizeof(T) >= asUintptr()); here? And likewise, mutatis mutandis, for operator-.

Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=ae93decfc52b Pushed at: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad453c39bd03