inline bool StringBuffer::append(JSLinearString *str) is missing a checkLength before appending, oops. :-( I think adding one there would fix this. But ideally I think we'd do something special to make sure that all appends to the internal vector are always checkLength()'d, and have the type system and/or access controls enforce this.

Created attachment 605904[details][diff][review]
Patch
Basically we weren't checking the length of the string we'd generate enough. There are really only two points where this length-checking matters: when finishing a buffer into a string, and when finishing a buffer into an atom. The atom path includes a check in js_AtomizeChars, necessarily. The other path had a check, but I removed it because it looked like every appending path included a check -- but they didn't.
As there are a not-entirely-trivial number of ways to append to a string, making sure every one of them contains a check is a little tricky and demonstrably error-prone. So let's just do the check once when attempting to create the final string/atom, and remove the checks from everything else (also coincidentally speeding up appends). If someone buffers up a really huge sequence of characters they'll consume a lot more memory than if we checked on every append, but this is the rare case, so it doesn't seem too bad penalizing it.

Created attachment 605970[details][diff][review]
Aurora patch
The trunk patch had a little extra cleanup that's not strictly necessary and makes things trickier to verify. This is the absolute minimal fix. Branch code will be slightly less efficient without the extra changes in the trunk patch, but pretty much negligibly so.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b24813652b1
Backed out, because the assertion in StringBuffer::length() is now invalid. (It was invalid before, of course, because not all appends were properly checkLength-ing. But only with the landing here did it start biting jstests on tinderbox.) Probably removing that's enough, but I'm going to tryserver to be sure that allowing StringBuffers to grow to whatever size they want doesn't cause OOM failures on tinderbox or whatever.

Created attachment 606022[details][diff][review]
Don't assert in length() either, get rid of StringBuffer-inl.h entirely
I removed the assertion from length(). Then I was thinking and realized all the methods that no longer have checkLength() calls could just be defined in the header, too, and things snowballed. To make a long story short, I don't even know why we have StringBuffer-inl.h at all, and with a little work we can just remove it, so I did that. (Branch will obviously take a much smaller patch.)

Created attachment 606730[details][diff][review]
Aurora patch with small tweak suggested by trunk patch results
[Approval Request Comment]
Regression caused by (bug #): Bug 733602.
User impact if declined: Not a whole lot, because js_NewString contains the same length-validation check and will prevent the problem there as well, but it'll keep fuzzers happy not to have to tiptoe around this assertion.
Testing completed (on m-c, etc.): Fixed on trunk, fix is straightforward to verify.
Risk to taking this patch (and alternatives if risky): Very little, simply readding an assertion that had been removed.
String changes made by this patch: None.

Comment on attachment 606730[details][diff][review]
Aurora patch with small tweak suggested by trunk patch results
[Triage Comment]
Given the fact that this may be sg:crit, we have a low risk fix in hand, and there's enough time remaining in the cycle to shake out any regressions, approving for Aurora 13.