Bug 145138 – ES6: Should not allow duplicate basic __proto__ properties in Object Literals

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Should not allow duplicate basic __proto__ properties in Object Literals.

* SPEC
ES6 Annex B.3.1 <https://people.mozilla.org/~jorendorff/es6-draft.html#sec-__proto__-property-names-in-object-initializers>

> It is a Syntax Error if PropertyNameList of PropertyDefinitionList contains any duplicate entries
> for "__proto__" and at least two of those entries were obtained from productions of the form
> 
>     PropertyDefinition : PropertyName : AssignmentExpression .
>
> NOTE: The List returned by PropertyNameList does not include string literal property names defined
> as using a ComputedPropertyName.

Note that shorthand property names ({__proto__}), computed properties ({["__proto__"]:{}}), and getter/setter ({get __proto__(){}}) are ignored for the purposes of this duplicate check. Those don't affect the prototype chain anyways, they are just properties set directly on the object.

* STEPS TO REPRODUCE
1. js> ({__proto__:null, __proto__:null})
  => should throw a SyntaxError, does not.

* NOTES
- Other browsers behave correctly here.

  Chrome:
  js> o = {__proto__:null, __proto__:null}
  Uncaught SyntaxError: Duplicate __proto__ fields are not allowed in object literals

  Firefox:
  js> o = {__proto__:null, __proto__:null}
  SyntaxError: property name __proto__ appears more than once in object literal

  Safari:
  js> o = {__proto__:null, __proto__:null}
  Object

- This is covered by the ES6 Compat Table:
<https://kangax.github.io/compat-table/es6/#__proto___in_object_literals_multiple___proto___is_an_error>

[Joseph Pecoraro]

Interesting! I have this working for everything except eval:

    eval("({ __proto__:[], __proto__:{} })")

Which actually exposes a separate issue.

In the eval, JavaScriptCore goes through the LiteralParser::tryLiteralParse, similar to a JSON.parse(). In the LiteralParser, __proto__ is treated as a direct property, not a prototype changing put.

So there is a second bug here that needs to get fixed:

  js> eval("({__proto__:[]})") instanceof Array
    => should be true (Chrome, Firefox), but is false (Safari)

We correctly handle the JSON.parse case:

  js> JSON.parse("({__proto__:[]})") instanceof Array
    => false (Safari, Chrome, Firefox)

I don't know what kind of performance impact we might see from removing the LiteralParser optimization for eval, so I'll look at just bailing if we see a __proto__ in eval LiteralParsing.

[Joseph Pecoraro]

Further investigation exposes another bizarre compatibility issue:

----

    1. Strict JSON object literal in eval
    2. Non-Strict JSON object literal in eval
    3. Strict JSON object literal 
    4. Non-Strict JSON object literal

Firefox:

    js> eval('({"__proto__":[]})') instanceof Array
    false
    js> eval('({__proto__:[]})') instanceof Array
    true
    js> ({"__proto__":[]}) instanceof Array
    true
    js> ({__proto__:[]}) instanceof Array
    true

Chrome: 

    js> eval('({"__proto__":[]})') instanceof Array
    true
    js> eval('({__proto__:[]})') instanceof Array
    true
    js> ({"__proto__":[]}) instanceof Array
    true
    js> ({__proto__:[]}) instanceof Array
    true

Safari: (before fix)

    js> eval('({"__proto__":[]})') instanceof Array
    false
    js> eval('({__proto__:[]})') instanceof Array
    false
    js> ({"__proto__":[]}) instanceof Array
    true
    js> ({__proto__:[]}) instanceof Array
    true

----

Based on the ES6 spec, I expect these all to return true. The object literal syntax always satisfies the case where we should change the object's prototype.

Unfortunately this changes the behavior of the legacy json2-es5-compat pre-native-JSON parsing of JSON with __proto__ properties. The old JSON2 library relied on `eval` to implement JSON.parse. Since eval now actually sets the prototype, and JSON.parse was just setting a direct property I'm seeing some failures on the js/dom/JSON-parse.html test due to json2-es5-compat.

Firefox's special case here is interesting, because for eval with strict JSON (quoted property names) it gets the old behavior of not modifying the prototype.

However, my opinion would be to not worry about the legacy case. Browsers have had native JSON objects for quite a while now. Using JSON.parse instead of eval will always get you consistent behavior, and should be what we recommend. Also, I would suspect __proto__ in JSON is rare.

[Joseph Pecoraro]

Created attachment 253363 [details]
[PATCH] Proposed Fix

Some further improvements to look into:

  1. Investigate eliminating the LiteralParser path for eval?
    - eval("...") of JSON/JSONP input gets a special parser. It is easy to miss runtime semantic changes (like this).
    - How much of a benefit is it?
  2. Investigate eliminating the parseStrictObjectLiteral / parseObjectLiteral difference.
    - So far as I can see the difference is purely whether or not we build strings while parsing.
    - How much of a benefit is it?

[Radar WebKit Bug Importer]

<rdar://problem/21032823>

[WebKit Commit Bot]

Comment on attachment 253363 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 253363

Committed r184640: <http://trac.webkit.org/changeset/184640>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #5)
> Comment on attachment 253363 [details]
> [PATCH] Proposed Fix
> 
> Clearing flags on attachment: 253363
> 
> Committed r184640: <http://trac.webkit.org/changeset/184640>

It made a jsc test fail, please fix it.

[Joseph Pecoraro]

This caused a JSC test failure. Only one:

FAIL: jsc-layout-tests.yaml/js/script-tests/object-literal-duplicate-properties.js.layout-no-llint

With issues like:

jsc-layout-tests.yaml/js/script-tests/object-literal-duplicate-properties.js.layout-no-llint: -PASS (function(){({__proto__:[], __proto__:{}})})() threw exception SyntaxError: Attempted to redefine __proto__ property..
jsc-layout-tests.yaml/js/script-tests/object-literal-duplicate-properties.js.layout-no-llint: +PASS (function(){({__proto__:[], __proto__:{}})})() threw exception RangeError: Maximum call stack size exceeded..

Curious. Also why only "no-llint"?

[Joseph Pecoraro]

(In reply to comment #8)
> This caused a JSC test failure. Only one:
> 
> FAIL:
> jsc-layout-tests.yaml/js/script-tests/object-literal-duplicate-properties.js.
> layout-no-llint
> 
> With issues like:
> 
> jsc-layout-tests.yaml/js/script-tests/object-literal-duplicate-properties.js.
> layout-no-llint: -PASS (function(){({__proto__:[], __proto__:{}})})() threw
> exception SyntaxError: Attempted to redefine __proto__ property..
> jsc-layout-tests.yaml/js/script-tests/object-literal-duplicate-properties.js.
> layout-no-llint: +PASS (function(){({__proto__:[], __proto__:{}})})() threw
> exception RangeError: Maximum call stack size exceeded..
> 
> Curious. Also why only "no-llint"?

Reproduced with:
shell> DYLD_FRAMEWORK_PATH=$build/Debug JSC_useLLInt=0 $build/Debug/jsc LayoutTests/resources/standalone-pre.js LayoutTests/js/script-tests/object-literal-duplicate-properties.js LayoutTests/resources/standalone-post.js

Looks like the issue comes from:

(lldb) bt
* thread #1: tid = 0x44b1c9, 0x000000010ce82737 JavaScriptCore`JSC::createStackOverflowError(exec=0x00007fff5322f180) + 23 at ExceptionHelpers.cpp:75, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x000000010ce82737 JavaScriptCore`JSC::createStackOverflowError(exec=0x00007fff5322f180) + 23 at ExceptionHelpers.cpp:75
    frame #1: 0x000000010ce839a2 JavaScriptCore`JSC::throwStackOverflowError(exec=0x00007fff5322f180) + 66 at ExceptionHelpers.cpp:291
    frame #2: 0x000000010d07b33e JavaScriptCore`::linkFor(execCallee=0x00007fff5322f130, callLinkInfo=0x0000000111eca120, kind=CodeForCall, registers=RegisterPreservationNotRequired) + 574 at JITOperations.cpp:749
    frame #3: 0x000000010d075946 JavaScriptCore`::operationLinkCall(execCallee=0x00007fff5322f130, callLinkInfo=0x0000000111eca120) + 38 at JITOperations.cpp:770
...

With:

frame #2: 0x000000010d07b33e JavaScriptCore`::linkFor(execCallee=0x00007fff5322f130, callLinkInfo=0x0000000111eca120, kind=CodeForCall, registers=RegisterPreservationNotRequired) + 574 at JITOperations.cpp:749
   746 	
   747 	        JSObject* error = functionExecutable->prepareForExecution(execCallee, callee, scope, kind);
   748 	        if (error) {
-> 749 	            throwStackOverflowError(exec);
   750 	            return reinterpret_cast<char*>(vm->getCTIStub(throwExceptionFromCallSlowPathGenerator).code().executableAddress());
   751 	        }
   752 	        codeBlock = functionExecutable->codeBlockFor(kind);

If this throws the "error" instead forcing a throwStackOverflowError, then it behaves correctly. Checking if there is any other fallout from this change (which should undergo review).

If there is fallout, I'll separate out this portion of the test and skip it so the bots return to green.

[Joseph Pecoraro]

I addressed the test issue in bug 145219:
<http://trac.webkit.org/changeset/184647>