* 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>
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.
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.
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?
<rdar://problem/21032823>
Comment on attachment 253363 [details] [PATCH] Proposed Fix Clearing flags on attachment: 253363 Committed r184640: <http://trac.webkit.org/changeset/184640>
All reviewed patches have been landed. Closing bug.
(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.
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"?
(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.
I addressed the test issue in bug 145219: <http://trac.webkit.org/changeset/184647>