Bug 90678 - JavaScript: Drop the “escaped reserved words as identifiers” compatibility measure
Summary: JavaScript: Drop the “escaped reserved words as identifiers” compatibility me...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL: https://mathias.html5.org/specs/javas...
Keywords: InRadar
Depends on: 145844
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-06 05:44 PDT by Mathias Bynens
Modified: 2015-06-10 16:43 PDT (History)
14 users (show)

See Also:


Attachments
Patch (15.41 KB, patch)
2015-06-09 14:21 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (16.46 KB, patch)
2015-06-10 14:57 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mathias Bynens 2012-07-06 05:44:25 PDT
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.
Comment 1 Mathias Bynens 2012-07-06 05:49:28 PDT
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
Comment 2 Radar WebKit Bug Importer 2012-07-06 19:30:50 PDT
<rdar://problem/11824059>
Comment 3 Alexey Proskuryakov 2012-07-07 01:19:16 PDT
See also: bug 42471.
Comment 4 Mathias Bynens 2012-07-07 10:15:08 PDT
(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.
Comment 5 Mathias Bynens 2012-07-09 02:04:09 PDT
See also: https://bugs.ecmascript.org/show_bug.cgi?id=277
Comment 6 Mathias Bynens 2013-09-18 00:30:14 PDT
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.
Comment 7 Mathias Bynens 2013-12-12 04:13:52 PST
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
Comment 8 Mark S. Miller 2014-05-11 11:01:47 PDT
What is the status of this? It is security significant for systems that pre-filter or translate code in order to enforce restrictions.
Comment 9 Yusuke Suzuki 2015-06-09 14:21:06 PDT
Created attachment 254602 [details]
Patch
Comment 10 Darin Adler 2015-06-09 14:43:50 PDT
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 11 Yusuke Suzuki 2015-06-09 15:06:42 PDT
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 12 WebKit Commit Bot 2015-06-10 06:26:11 PDT
Comment on attachment 254602 [details]
Patch

Clearing flags on attachment: 254602

Committed r185414: <http://trac.webkit.org/changeset/185414>
Comment 13 WebKit Commit Bot 2015-06-10 06:26:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Csaba Osztrogonác 2015-06-10 08:32:59 PDT
(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.
Comment 15 Csaba Osztrogonác 2015-06-10 08:34:33 PDT
And it made 50+ assert (and exit early) on the debug bots.
Comment 16 WebKit Commit Bot 2015-06-10 09:39:31 PDT
Re-opened since this is blocked by bug 145844
Comment 17 Alex Christensen 2015-06-10 10:00:27 PDT
This caused problems like this:
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/4856
Comment 18 Yusuke Suzuki 2015-06-10 11:40:40 PDT
Oops. Thank you for creating the rolling back.
Comment 19 Yusuke Suzuki 2015-06-10 12:25:15 PDT
It seems that leveraged path is never executed before this patch. (m_buffer16.shrink(0); is missing)
Comment 20 Yusuke Suzuki 2015-06-10 14:57:35 PDT
Created attachment 254679 [details]
Patch
Comment 21 Darin Adler 2015-06-10 15:50:15 PDT
Comment on attachment 254679 [details]
Patch

OK, lets try this again.
Comment 22 Alex Christensen 2015-06-10 15:58:53 PDT
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 23 WebKit Commit Bot 2015-06-10 16:43:53 PDT
Comment on attachment 254679 [details]
Patch

Clearing flags on attachment: 254679

Committed r185437: <http://trac.webkit.org/changeset/185437>
Comment 24 WebKit Commit Bot 2015-06-10 16:43:59 PDT
All reviewed patches have been landed.  Closing bug.