Bug 90678 – JavaScript: Drop the “escaped reserved words as identifiers” compatibility measure

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mathias Bynens]

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.

[Mathias Bynens]

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

[Radar WebKit Bug Importer]

<rdar://problem/11824059>

[Alexey Proskuryakov]

See also: bug 42471.

[Mathias Bynens]

(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.

[Mathias Bynens]

See also: https://bugs.ecmascript.org/show_bug.cgi?id=277

[Mathias Bynens]

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.

[Mathias Bynens]

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

[Mark S. Miller]

What is the status of this? It is security significant for systems that pre-filter or translate code in order to enforce restrictions.

[Yusuke Suzuki]

Created attachment 254602 [details]
Patch

[Darin Adler]

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?

[Yusuke Suzuki]

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.

[WebKit Commit Bot]

Comment on attachment 254602 [details]
Patch

Clearing flags on attachment: 254602

Committed r185414: <http://trac.webkit.org/changeset/185414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

And it made 50+ assert (and exit early) on the debug bots.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145844

[Alex Christensen]

This caused problems like this:
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/4856

[Yusuke Suzuki]

Oops. Thank you for creating the rolling back.

[Yusuke Suzuki]

It seems that leveraged path is never executed before this patch. (m_buffer16.shrink(0); is missing)

[Yusuke Suzuki]

Created attachment 254679 [details]
Patch

[Darin Adler]

Comment on attachment 254679 [details]
Patch

OK, lets try this again.

[Alex Christensen]

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.

[WebKit Commit Bot]

Comment on attachment 254679 [details]
Patch

Clearing flags on attachment: 254679

Committed r185437: <http://trac.webkit.org/changeset/185437>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.