new RegExp('\n').toString() returns / / , so this result is including LineTerminator. And, according to ECMA262 section 15.10.6.4 "The returned String has the form of a RegularExpressionLiteral that evaluates to another RegExp object with the same behaviour as this object." http://es5.github.com/#x15.10.6.4 And, because of RegularExpressionNonTerminator, LineTerminator is not allowed in RegularExpressionLiteral. So RegExp.source should be escaped. Furthermore, new RegExp('\\\n').toString() should provide /\n/, because /\\\n/ doesn't have the same behaviour to new RegExp('\\\n').
Created attachment 113682 [details] Patch attached patch
reported issue and sent patch to V8 http://code.google.com/p/v8/issues/detail?id=1813
Comment on attachment 113682 [details] Patch Good catch & nice fix. But I think we may want to make one tweak to this patch before landing. Most regular expressions don't contain forwards slashes or newlines, but this patch will always produce a fresh copy of the string every time source is called. Currently the code has an early return for strings that don't contain a forwards slash. We could expand this to also check for newline characters, and early return if the string contains neither forward slashes or newlines. r- for the loss of this optimization, but r+ in principle, we should definitely take this fix & the code looks great otherwise. :-)
(In reply to comment #3) > (From update of attachment 113682 [details]) > Good catch & nice fix. But I think we may want to make one tweak to this patch before landing. Most regular expressions don't contain forwards slashes or newlines, but this patch will always produce a fresh copy of the string every time source is called. Currently the code has an early return for strings that don't contain a forwards slash. We could expand this to also check for newline characters, and early return if the string contains neither forward slashes or newlines. r- for the loss of this optimization, but r+ in principle, we should definitely take this fix & the code looks great otherwise. :-) Thanks for your review. You're right. I'll add early return to this patch.
Created attachment 115232 [details] Patch attached patch v2. First, checking this pattern should be escaped, and if it is false return early
Comment on attachment 115232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115232&action=review Looks OK. Needs a couple small changes. > Source/JavaScriptCore/runtime/RegExpObject.cpp:124 > + bool previousIsBackSlash = false; backslash is one word, not two, so this should be named previousCharacterWasBackslash > Source/JavaScriptCore/runtime/RegExpObject.cpp:145 > + if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) { This should use the inline isLineTerminator function instead of repeating its code. > Source/JavaScriptCore/runtime/RegExpObject.cpp:157 > + if (!shouldEscape) > + return jsString(exec, pattern); This is indented wrong. Should be indented four spaces. > Source/JavaScriptCore/runtime/RegExpObject.cpp:177 > + if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) { This should use the inline isLineTerminator function instead of repeating its code.
(In reply to comment #6) Thank you for your review. > backslash is one word, not two, so this should be named previousCharacterWasBackslash Yes, I'll rename it. > > Source/JavaScriptCore/runtime/RegExpObject.cpp:145 > > + if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) { > This should use the inline isLineTerminator function instead of repeating its code. Right, I'll change it to using Lexer<UChar>::isLineTerminator. > This is indented wrong. Should be indented four spaces. Oops. This is mistake. I'll upload revised patch soon.
Created attachment 115244 [details] Patch attached patch v3
Comment on attachment 115244 [details] Patch Looks great.
Comment on attachment 115244 [details] Patch Clearing flags on attachment: 115244 Committed r100469: <http://trac.webkit.org/changeset/100469>
All reviewed patches have been landed. Closing bug.