For example, `var var;` throws a syntax error, but e.g. `var v\u0061r;` works fine, currently. This is a violation of the ECMAScript 5.1 spec, and used to be a compatibility requirement: http://mathias.html5.org/specs/javascript/#escaped-reserved-words One year ago, all browsers except IE fulfilled this. Half a year ago Firefox dropped this non-standard addition (https://bugzilla.mozilla.org/show_bug.cgi?id=694360) and hasn’t seen any compatibility issues since. Please align with Firefox and IE by removing this non-standard extension.
I’ve filed the same bug here: * Opera/Carakan: https://bugs.opera.com/browse/DSK-369398 * Chrome/V8: http://code.google.com/p/v8/issues/detail?id=2222
<rdar://problem/11824059>
See also: bug 42471.
(In reply to comment #3) > See also: bug 42471. Note that bug 42471 deals with IdentifierNames, while this bug (90678) is about Identifiers. Identifiers are IdentifierNames that are not reserved words.
See also: https://bugs.ecmascript.org/show_bug.cgi?id=277
FWIW, https://bugs.ecmascript.org/show_bug.cgi?id=277 is accepted, and Allen says he will make it clear (in ES6) that escaped reserved words are not allowed. The same goes for escaped regular expression flags. Now would be a good time to drop this non-standard behavior that is apparently not a backwards compatibility concern.
Confirmed: ES6 drops the “escaped reserved words as identifiers” compatibility measure. This will be fixed in v8: https://code.google.com/p/v8/issues/detail?id=2222#c4 SpiderMonkey already has ES6-compliant behavior: https://bugzilla.mozilla.org/show_bug.cgi?id=744784
What is the status of this? It is security significant for systems that pre-filter or translate code in order to enforce restrictions.
Created attachment 254602 [details] Patch
Comment on attachment 254602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254602&action=review > Source/JavaScriptCore/parser/Lexer.cpp:-1092 > - if (remaining < maxTokenLength) { I don’t understand what the purpose of this check was, nor why it’s OK to remove it now. > Source/JavaScriptCore/parser/Lexer.cpp:1090 > + const HashTableValue* entry = m_vm->keywords->getKeyword(*ident); If shouldCreateIdentifier is false, then ident will be null, and this will crash. Is it not legal to call this function with shouldCreateIdentifier false and without LexerFlagsIgnoreReservedWords? Isn’t there any code that calls it that way?
Comment on attachment 254602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254602&action=review thank you for your review. >> Source/JavaScriptCore/parser/Lexer.cpp:-1092 >> - if (remaining < maxTokenLength) { > > I don’t understand what the purpose of this check was, nor why it’s OK to remove it now. As far as I checked the Lexer's code, I think this `remaining < maxTokenLength` is micro-optimization code. `maxTokenLength` indicates the maximum keyword length + 1 (according to the KeywordLookupGenerator.py). (currently 11 for `instanceof`) For remaining >= maxTokenLength case (& if the code doesn't contain escape sequences), we checked keyword in `parseIdentifier`. So here, we just checked `remaining < maxTokenLength` case. (!bufferRequired ensured that this branch is executed for identifiers not containing escapes). But now, this branch is also executed for identifiers containing escape sequence. So the above short circuit is no longer effective. However, I think `parseIdentifierSlowCase` is always executed for identifiers containing escape sequence. Anyway, this guard is needed to be removed. >> Source/JavaScriptCore/parser/Lexer.cpp:1090 >> + const HashTableValue* entry = m_vm->keywords->getKeyword(*ident); > > If shouldCreateIdentifier is false, then ident will be null, and this will crash. Is it not legal to call this function with shouldCreateIdentifier false and without LexerFlagsIgnoreReservedWords? Isn’t there any code that calls it that way? Good question. `shouldCreateIdentifier` becomes false when `lexerFlags` has `LexexFlagsDontBuildKeywords`. And `LexexFlagsDontBuildKeywords` is used in `SyntaxChecker::DontBuildKeywords`. When using `DontBuildKeywords`, the parse always use `LexerFlagsIgnoreReservedWords`. So when `LexexFlagsDontBuildKeywords` is specified, `LexerFlagsIgnoreReservedWords` is always specified. As a result, if `!(lexerFlags & LexerFlagsIgnoreReservedWords)` becomes true, `LexexFlagsDontBuildKeywords` is never specified. (so, `shouldCreateIdentifier` is always true, this is the reason why `ASSERT(shouldCreateIdentifier);` is inserted.
Comment on attachment 254602 [details] Patch Clearing flags on attachment: 254602 Committed r185414: <http://trac.webkit.org/changeset/185414>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > Comment on attachment 254602 [details] > Patch > > Clearing flags on attachment: 254602 > > Committed r185414: <http://trac.webkit.org/changeset/185414> It broke mozilla-tests.yaml/ecma_3/Unicode/uc-003.js everywhere.
And it made 50+ assert (and exit early) on the debug bots.
Re-opened since this is blocked by bug 145844
This caused problems like this: https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/4856
Oops. Thank you for creating the rolling back.
It seems that leveraged path is never executed before this patch. (m_buffer16.shrink(0); is missing)
Created attachment 254679 [details] Patch
Comment on attachment 254679 [details] Patch OK, lets try this again.
I spot-checked js/kde/parse.html crashed with the first patch but not with the second, which leads me to believe that this won't break anything.
Comment on attachment 254679 [details] Patch Clearing flags on attachment: 254679 Committed r185437: <http://trac.webkit.org/changeset/185437>