Attachment 373595 Details for Bug 163446 – Patch

[patch] Patch

bug-163446-20190707160059.patch (text/plain), 18.41 KB, created by Alexey Shvayka on 2019-07-07 06:01:01 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alexey Shvayka

Created: 2019-07-07 06:01:01 PDT

Size: 18.41 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 247197)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,20 @@
>+2019-07-07  Alexey Shvayka  <shvaikalesh@gmail.com>
>+
>+        JSON.parse should not modify non-configurable properties.
>+        https://bugs.webkit.org/show_bug.cgi?id=163446
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Due to bug in [[DefineOwnProperty]] (that is now used by JSON.parse), revived objects
>+        have incorrect property order. Adjust ChakraCore test expectations to match it.
>+
>+        * ChakraCore/test/JSON/jx2.baseline-jsc:
>+        * microbenchmarks/json-parse-array-reviver-same-value.js: Added.
>+        * microbenchmarks/json-parse-array-reviver.js: Added.
>+        * microbenchmarks/json-parse-object-reviver-same-value.js: Added.
>+        * microbenchmarks/json-parse-object-reviver.js: Added.
>+        * stress/json-parse-reviver-non-configurable.js: Added.
>+
> 2019-07-06  Michael Saboff  <msaboff@apple.com>
> 
>         switch(String) needs to check for exceptions when resolving the string
>Index: JSTests/ChakraCore/test/JSON/jx2.baseline-jsc
>===================================================================
>--- JSTests/ChakraCore/test/JSON/jx2.baseline-jsc	(revision 247164)
>+++ JSTests/ChakraCore/test/JSON/jx2.baseline-jsc	(working copy)
>@@ -88,7 +88,7 @@ null
> +++out reviver
> 
>  __Parsed with tracing reviver and stringify back = 
>-{"":7,"memberNullFirst":null,"memberNegNum":-98765,"memberStr":"StringJSON","memberBool":99,"memberObj":{"mm":99,"mb":false},"memberX":{},"memberArray":[33,"StringTst",null,{}],"memberNull":null}
>+{"":7,"memberNullFirst":null,"memberNegNum":-98765,"memberStr":"StringJSON","memberObj":{"mb":false,"mm":99},"memberX":{},"memberArray":[33,"StringTst",null,{}],"memberNull":null,"memberBool":99}
> 
> 
> 
>@@ -99,7 +99,7 @@ JSON Parse__  original= 
> {"" : 7, "memberNullFirst" : null, "dateMember" : "2008-05-30T07:00:59Z", "memberNum" : 3, "memberStr"  : "StringJSON", "memberBool" : true , "memberObj" : { "mm" : 1, "mb" : false}, "memberX" : {}, "memberArray" : [33, "StringTst",null,{}], "memberNull" : null}
> 
>  __Parsed and stringify back with Date rev = 
>-{"":7,"memberNullFirst":null,"dateMember":"2008-05-30T07:00:59.000Z","memberNum":3,"memberStr":"StringJSON","memberBool":true,"memberObj":{"mm":1,"mb":false},"memberX":{},"memberArray":[33,"StringTst",null,{}],"memberNull":null}
>+{"":7,"memberNullFirst":null,"memberNum":3,"memberStr":"StringJSON","memberBool":true,"memberObj":{"mm":1,"mb":false},"memberX":{},"memberArray":[33,"StringTst",null,{}],"memberNull":null,"dateMember":"2008-05-30T07:00:59.000Z"}
> VERIFICATION:  restored date year(if this doesn't throw we know the filter worked)= 
> 2008
> 
>@@ -112,7 +112,7 @@ JSON Parse__  original= 
> {"" : 7, "memberNullFirst" : null, "dateMember" : "2008-05-30T07:00:59Z", "memberNum" : 3, "memberStr"  : "StringJSON", "memberBool" : true , "memberObj" : { "mm" : 1, "mb" : false}, "memberX" : {}, "memberArray" : [33, "StringTst",null,{}], "memberNull" : null}
> 
>  __Parsed with reviver2 and stringify back= 
>-{"":7,"memberNullFirst":null,"dateMember":"2008-05-30T07:00:59Z","memberNum":3,"memberStr":"StringJSON","memberBool":99,"memberObj":{"mm":99,"mb":false},"memberX":{},"memberArray":[33,"StringTst",null,{}],"memberNull":null}
>+{"":7,"memberNullFirst":null,"dateMember":"2008-05-30T07:00:59Z","memberNum":3,"memberStr":"StringJSON","memberObj":{"mb":false,"mm":99},"memberX":{},"memberArray":[33,"StringTst",null,{}],"memberNull":null,"memberBool":99}
> 
> 
> 
>@@ -229,7 +229,7 @@ true
> === Parsed and restringified :
> {"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"a":true,"b":false}},"a":{"a":{},"nestedM2":1234,"b":{"a":true,"b":false,"c":3.14}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
> === Parsed with reviver and restringified :
>-{"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"a":99,"b":false}},"a":{"a":{},"nestedM2":1234,"b":{"a":99,"b":false}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
>+{"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"b":false,"a":99}},"a":{"a":{},"nestedM2":1234,"b":{"b":false,"a":99}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
> 
> ------ JSON test stringify: Simple array  ------
> ["document.location","foolish"]
>@@ -363,7 +363,7 @@ true
> === Parsed with no reviver and restringified :
> {"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"a":true,"b":false}},"a":{"a":{},"nestedM2":1234,"b":{"a":true,"b":false,"c":3.14}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
> === Parsed with reviver2 and restringified :
>-{"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"a":99,"b":false}},"a":{"a":{},"nestedM2":1234,"b":{"a":99,"b":false}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
>+{"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"b":false,"a":99}},"a":{"a":{},"nestedM2":1234,"b":{"b":false,"a":99}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
> 
> ------ JSON test stringify: Simple array  ------
> ["document.location","foolish"]
>@@ -528,7 +528,7 @@ true
> === Parsed with no reviver and restringified :
> {"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"a":true,"b":false}},"a":{"a":{},"nestedM2":1234,"b":{"a":true,"b":false,"c":3.14}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
> === Parsed with reviver2 and restringified :
>-{"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"a":99,"b":false}},"a":{"a":{},"nestedM2":1234,"b":{"a":99,"b":false}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
>+{"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"b":false,"a":99}},"a":{"a":{},"nestedM2":1234,"b":{"b":false,"a":99}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
> 
> ------ JSON test stringify: Simple array  ------
> [
>@@ -721,7 +721,7 @@ true
> === Parsed with no reviver and restringified :
> {"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"a":true,"b":false}},"a":{"a":{},"nestedM2":1234,"b":{"a":true,"b":false,"c":3.14}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
> === Parsed with reviver2 and restringified :
>-{"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"a":99,"b":false}},"a":{"a":{},"nestedM2":1234,"b":{"a":99,"b":false}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
>+{"MemberNo1":"\"data\"","dateMember":"2008-04-01T07:00:00.000Z","nullMember":null,"arr":["document.location","foolish"],"nested":{"nestedM1":{},"nestedM2":1234,"nestedM3":{"b":false,"a":99}},"a":{"a":{},"nestedM2":1234,"b":{"b":false,"a":99}},"stringmember":"this string ends the obj. You should not see functionMember and undefinedMember"}
> 
> ------ JSON test stringify: Simple array  ------
> [
>@@ -1660,7 +1660,7 @@ true
> === Parsed with no reviver and restringified :
> {"a":{"a":{},"b":{"a":true,"b":false}}}
> === Parsed with reviver2 and restringified :
>-{"a":{"a":{},"b":{"a":99,"b":false}}}
>+{"a":{"a":{},"b":{"b":false,"a":99}}}
> 
> ------ JSON test stringify: Simple array  ------
> ["document.location","foolish"]
>@@ -1802,7 +1802,7 @@ true
> === Parsed with no reviver and restringified :
> {"a":{"a":{},"b":{"a":true,"b":false}}}
> === Parsed with reviver2 and restringified :
>-{"a":{"a":{},"b":{"a":99,"b":false}}}
>+{"a":{"a":{},"b":{"b":false,"a":99}}}
> 
> ------ JSON test stringify: Simple array  ------
> [
>@@ -1968,7 +1968,7 @@ true
> === Parsed with no reviver and restringified :
> {"a":{"a":{},"b":{"a":true,"b":false}}}
> === Parsed with reviver2 and restringified :
>-{"a":{"a":{},"b":{"a":99,"b":false}}}
>+{"a":{"a":{},"b":{"b":false,"a":99}}}
> 
> ------ JSON test stringify: Simple array  ------
> [
>@@ -2283,7 +2283,7 @@ true
> === Parsed with no reviver and restringified :
> {"a":{"a":{},"b":{"a":true,"b":false}}}
> === Parsed with reviver2 and restringified :
>-{"a":{"a":{},"b":{"a":99,"b":false}}}
>+{"a":{"a":{},"b":{"b":false,"a":99}}}
> 
> ------ JSON test stringify: Simple array  ------
> ["document.location","foolish"]
>@@ -2425,7 +2425,7 @@ true
> === Parsed with no reviver and restringified :
> {"a":{"a":{},"b":{"a":true,"b":false}}}
> === Parsed with reviver2 and restringified :
>-{"a":{"a":{},"b":{"a":99,"b":false}}}
>+{"a":{"a":{},"b":{"b":false,"a":99}}}
> 
> ------ JSON test stringify: Simple array  ------
> [
>@@ -2591,7 +2591,7 @@ true
> === Parsed with no reviver and restringified :
> {"a":{"a":{},"b":{"a":true,"b":false}}}
> === Parsed with reviver2 and restringified :
>-{"a":{"a":{},"b":{"a":99,"b":false}}}
>+{"a":{"a":{},"b":{"b":false,"a":99}}}
> 
> ------ JSON test stringify: Simple array  ------
> [
>Index: JSTests/microbenchmarks/json-parse-array-reviver-same-value.js
>===================================================================
>--- JSTests/microbenchmarks/json-parse-array-reviver-same-value.js	(nonexistent)
>+++ JSTests/microbenchmarks/json-parse-array-reviver-same-value.js	(working copy)
>@@ -0,0 +1,3 @@
>+const json = JSON.stringify([0,1,2,3,4,5,6,7,8,9]);
>+for (let i = 0; i < 1e5; ++i)
>+    JSON.parse(json, (_key, val) => val);
>Index: JSTests/microbenchmarks/json-parse-array-reviver.js
>===================================================================
>--- JSTests/microbenchmarks/json-parse-array-reviver.js	(nonexistent)
>+++ JSTests/microbenchmarks/json-parse-array-reviver.js	(working copy)
>@@ -0,0 +1,3 @@
>+const json = JSON.stringify([0,1,2,3,4,5,6,7,8,9]);
>+for (let i = 0; i < 1e5; ++i)
>+    JSON.parse(json, (_key, val) => val + 1);
>Index: JSTests/microbenchmarks/json-parse-object-reviver-same-value.js
>===================================================================
>--- JSTests/microbenchmarks/json-parse-object-reviver-same-value.js	(nonexistent)
>+++ JSTests/microbenchmarks/json-parse-object-reviver-same-value.js	(working copy)
>@@ -0,0 +1,3 @@
>+const json = JSON.stringify({a:0,b:1,c:2,d:3,e:4,f:5,g:6,h:7,i:8,j:9});
>+for (let i = 0; i < 1e5; ++i)
>+    JSON.parse(json, (_key, val) => val);
>Index: JSTests/microbenchmarks/json-parse-object-reviver.js
>===================================================================
>--- JSTests/microbenchmarks/json-parse-object-reviver.js	(nonexistent)
>+++ JSTests/microbenchmarks/json-parse-object-reviver.js	(working copy)
>@@ -0,0 +1,3 @@
>+const json = JSON.stringify({a:0,b:1,c:2,d:3,e:4,f:5,g:6,h:7,i:8,j:9});
>+for (let i = 0; i < 1e5; ++i)
>+    JSON.parse(json, (_key, val) => val + 1);
>Index: JSTests/stress/json-parse-reviver-non-configurable.js
>===================================================================
>--- JSTests/stress/json-parse-reviver-non-configurable.js	(nonexistent)
>+++ JSTests/stress/json-parse-reviver-non-configurable.js	(working copy)
>@@ -0,0 +1,35 @@
>+function shouldBe(actual, expected) {
>+    if (actual !== expected)
>+        throw new Error('bad value: ' + actual);
>+}
>+
>+function objReviver(key, value) {
>+    if (key === 'a')
>+        Object.defineProperty(this, 'b', {configurable: false});
>+    if (key === 'b') return 12;
>+
>+    return value;
>+}
>+
>+function arrReviver(key, value) {
>+    if (key === '0')
>+        Object.defineProperty(this, '1', {configurable: false});
>+    if (key === '1') return 24;
>+
>+    return value;
>+}
>+
>+const objJSON = '{"a": 1, "b": 2}';
>+const arrJSON = '[3, 4]';
>+
>+for (let i = 1; i < 10000; i++) {
>+    let obj = JSON.parse(objJSON, objReviver);
>+    shouldBe(obj.a, 1);
>+    shouldBe(obj.b, 2);
>+}
>+
>+for (let i = 1; i < 10000; i++) {
>+    let arr = JSON.parse(arrJSON, arrReviver);
>+    shouldBe(arr[0], 3);
>+    shouldBe(arr[1], 4);
>+}
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 247164)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2019-07-07  Alexey Shvayka  <shvaikalesh@gmail.com>
>+
>+        JSON.parse should not modify non-configurable properties.
>+        https://bugs.webkit.org/show_bug.cgi?id=163446
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Spec: https://tc39.es/ecma262/#sec-internalizejsonproperty, step 2.
>+
>+        * runtime/JSONObject.cpp: Use [[DefineOwnProperty]] instead of [[Set]].
>+        * runtime/JSObject.h: Remove inaccurate comments regarding [[DefineOwnProperty]].
>+
> 2019-07-05  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r247115.
>Index: Source/JavaScriptCore/runtime/JSONObject.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/JSONObject.cpp	(revision 247164)
>+++ Source/JavaScriptCore/runtime/JSONObject.cpp	(working copy)
>@@ -709,8 +709,11 @@ NEVER_INLINE JSValue Walker::walk(JSValu
>                 RETURN_IF_EXCEPTION(scope, { });
>                 if (filteredValue.isUndefined())
>                     array->methodTable(vm)->deletePropertyByIndex(array, m_exec, indexStack.last());
>-                else
>-                    array->putDirectIndex(m_exec, indexStack.last(), filteredValue, 0, PutDirectIndexShouldNotThrow);
>+                else {
>+                    PropertyDescriptor descriptor(filteredValue, static_cast<unsigned>(PropertyAttribute::None));
>+                    bool shouldThrow = false;
>+                    array->defineOwnIndexedProperty(m_exec, indexStack.last(), descriptor, shouldThrow);
>+                }
>                 RETURN_IF_EXCEPTION(scope, { });
>                 indexStack.last()++;
>                 goto arrayStartVisitMember;
>@@ -761,13 +764,15 @@ NEVER_INLINE JSValue Walker::walk(JSValu
>             case ObjectEndVisitMember: {
>                 JSObject* object = jsCast<JSObject*>(markedStack.last());
>                 Identifier prop = propertyStack.last()[indexStack.last()];
>-                PutPropertySlot slot(object);
>                 JSValue filteredValue = callReviver(object, jsString(m_exec, prop.string()), outValue);
>                 RETURN_IF_EXCEPTION(scope, { });
>                 if (filteredValue.isUndefined())
>                     object->methodTable(vm)->deleteProperty(object, m_exec, prop);
>-                else
>-                    object->methodTable(vm)->put(object, m_exec, prop, filteredValue, slot);
>+                else {
>+                    PropertyDescriptor descriptor(filteredValue, static_cast<unsigned>(PropertyAttribute::None));
>+                    bool shouldThrow = false;
>+                    object->methodTable(vm)->defineOwnProperty(object, m_exec, prop, descriptor, shouldThrow);
>+                }
>                 RETURN_IF_EXCEPTION(scope, { });
>                 indexStack.last()++;
>                 goto objectStartVisitMember;
>Index: Source/JavaScriptCore/runtime/JSObject.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSObject.h	(revision 247164)
>+++ Source/JavaScriptCore/runtime/JSObject.h	(working copy)
>@@ -209,7 +209,7 @@ public:
>     //  - accessors are not called.
>     //  - it will ignore extensibility and read-only properties if PutDirectIndexLikePutDirect is passed as the mode (the default).
>     // This method creates a property with attributes writable, enumerable and configurable all set to true if attributes is zero,
>-    // otherwise, it creates a property with the provided attributes. Semantically, this is performing defineOwnProperty.
>+    // otherwise, it creates a property with the provided attributes.
>     bool putDirectIndex(ExecState* exec, unsigned propertyName, JSValue value, unsigned attributes, PutDirectIndexMode mode)
>     {
>         ASSERT(!value.isCustomGetterSetter());
>@@ -237,7 +237,7 @@ public:
>         }
>         return putDirectIndexSlowOrBeyondVectorLength(exec, propertyName, value, attributes, mode);
>     }
>-    // This is semantically equivalent to performing defineOwnProperty(propertyName, {configurable:true, writable:true, enumerable:true, value:value}).
>+
>     bool putDirectIndex(ExecState* exec, unsigned propertyName, JSValue value)
>     {
>         return putDirectIndex(exec, propertyName, value, 0, PutDirectIndexLikePutDirect);
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 247164)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2019-07-07  Alexey Shvayka  <shvaikalesh@gmail.com>
>+
>+        JSON.parse should not modify non-configurable properties.
>+        https://bugs.webkit.org/show_bug.cgi?id=163446
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * js/resources/JSON-parse.js:
>+        (createTests.logOrder): Always return `value` so that [[DefineOwnProperty]] would be no-op.
>+
> 2019-07-05  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r247115.
>Index: LayoutTests/js/resources/JSON-parse.js
>===================================================================
>--- LayoutTests/js/resources/JSON-parse.js	(revision 247164)
>+++ LayoutTests/js/resources/JSON-parse.js	(working copy)
>@@ -414,7 +414,7 @@ function createTests() {
>     var logOrderString;
>     function logOrder(key, value) {
>         logOrderString += key +":"+JSON.stringify(value);
>-        return null;
>+        return value;
>     }
>     result.push(function(jsonObject){
>         logOrderString = "";

Actions: View | Formatted Diff | Diff

Attachments on bug 163446: 373115 | 373116 | 373119 | 373120 | 373121 | 373128 | 373136 | 373178 | 373180 | 373199 | 373595 | 404651