JSON.stringify should emit non own properties if second array argument includes
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jul 2018 19:01:34 +0000 (19:01 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Jul 2018 19:01:34 +0000 (19:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187724

Reviewed by Mark Lam.

JSTests:

* stress/json-stringify-getter-call.js: Added.
(shouldBe):
(A.prototype.get cocoa):
(A.prototype.get cappuccino):
(A):
(shouldBe.JSON.stringify):

Source/JavaScriptCore:

According to the spec[1], JSON.stringify needs to retrieve properties by using [[Get]],
instead of [[GetOwnProperty]]. It means that we would look up a properties defined
in [[Prototype]] or upper objects in the prototype chain. While enumeration is done
by using EnumerableOwnPropertyNames typically, we can pass replacer array including
property names which does not reside in the own properties. Or we can modify the
own properties by deleting properties while JSON.stringify is calling a getter. So,
using [[Get]] instead of [[GetOwnProperty]] is user-visible.

This patch changes getOwnPropertySlot to getPropertySlot to align the behavior to the spec.
The performance of Kraken/json-stringify-tinderbox is neutral.

[1]: https://tc39.github.io/ecma262/#sec-serializejsonproperty

* runtime/JSONObject.cpp:
(JSC::Stringifier::toJSON):
(JSC::Stringifier::toJSONImpl):
(JSC::Stringifier::appendStringifiedValue):
(JSC::Stringifier::Holder::Holder):
(JSC::Stringifier::Holder::appendNextProperty):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233924 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/json-stringify-getter-call.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSONObject.cpp

index 89d0f4f..da189c1 100644 (file)
@@ -1,5 +1,19 @@
 2018-07-18  Yusuke Suzuki  <utatane.tea@gmail.com>
 
+        JSON.stringify should emit non own properties if second array argument includes
+        https://bugs.webkit.org/show_bug.cgi?id=187724
+
+        Reviewed by Mark Lam.
+
+        * stress/json-stringify-getter-call.js: Added.
+        (shouldBe):
+        (A.prototype.get cocoa):
+        (A.prototype.get cappuccino):
+        (A):
+        (shouldBe.JSON.stringify):
+
+2018-07-18  Yusuke Suzuki  <utatane.tea@gmail.com>
+
         [JSC] JSON.stringify's replacer should use `isArray` instead of JSArray checks
         https://bugs.webkit.org/show_bug.cgi?id=187755
 
diff --git a/JSTests/stress/json-stringify-getter-call.js b/JSTests/stress/json-stringify-getter-call.js
new file mode 100644 (file)
index 0000000..30fdd19
--- /dev/null
@@ -0,0 +1,32 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+class A {
+    get cocoa() {
+        return "Cocoa";
+    }
+
+    get cappuccino() {
+        return "Cappuccino";
+    }
+}
+
+let a = new A();
+shouldBe(JSON.stringify(a), `{}`);
+shouldBe(JSON.stringify(a, ["cocoa", "cappuccino"]), `{"cocoa":"Cocoa","cappuccino":"Cappuccino"}`);
+
+let array = [0, 1, 2, 3, 4];
+Object.defineProperty(array.__proto__, 1, {
+    get: function () {
+        return "Cocoa";
+    }
+});
+Object.defineProperty(array, 0, {
+    get: function () {
+        delete array[1];
+        return "Cappuccino";
+    }
+});
+shouldBe(JSON.stringify(array), `["Cappuccino","Cocoa",2,3,4]`);
index 7bc5a88..ce22708 100644 (file)
@@ -1,5 +1,32 @@
 2018-07-18  Yusuke Suzuki  <utatane.tea@gmail.com>
 
+        JSON.stringify should emit non own properties if second array argument includes
+        https://bugs.webkit.org/show_bug.cgi?id=187724
+
+        Reviewed by Mark Lam.
+
+        According to the spec[1], JSON.stringify needs to retrieve properties by using [[Get]],
+        instead of [[GetOwnProperty]]. It means that we would look up a properties defined
+        in [[Prototype]] or upper objects in the prototype chain. While enumeration is done
+        by using EnumerableOwnPropertyNames typically, we can pass replacer array including
+        property names which does not reside in the own properties. Or we can modify the
+        own properties by deleting properties while JSON.stringify is calling a getter. So,
+        using [[Get]] instead of [[GetOwnProperty]] is user-visible.
+
+        This patch changes getOwnPropertySlot to getPropertySlot to align the behavior to the spec.
+        The performance of Kraken/json-stringify-tinderbox is neutral.
+
+        [1]: https://tc39.github.io/ecma262/#sec-serializejsonproperty
+
+        * runtime/JSONObject.cpp:
+        (JSC::Stringifier::toJSON):
+        (JSC::Stringifier::toJSONImpl):
+        (JSC::Stringifier::appendStringifiedValue):
+        (JSC::Stringifier::Holder::Holder):
+        (JSC::Stringifier::Holder::appendNextProperty):
+
+2018-07-18  Yusuke Suzuki  <utatane.tea@gmail.com>
+
         [JSC] JSON.stringify's replacer should use `isArray` instead of JSArray checks
         https://bugs.webkit.org/show_bug.cgi?id=187755
 
index b1ad412..e3cf498 100644 (file)
@@ -101,17 +101,17 @@ private:
 
     private:
         JSObject* m_object;
-        const bool m_isArray;
         const bool m_isJSArray;
-        unsigned m_index;
-        unsigned m_size;
+        const bool m_isArray;
+        unsigned m_index { 0 };
+        unsigned m_size { 0 };
         RefPtr<PropertyNameArrayData> m_propertyNames;
     };
 
     friend class Holder;
 
-    JSValue toJSON(JSValue, const PropertyNameForFunctionCall&);
-    JSValue toJSONImpl(VM&, JSValue, JSValue toJSONFunction, const PropertyNameForFunctionCall&);
+    JSValue toJSON(JSObject*, const PropertyNameForFunctionCall&);
+    JSValue toJSONImpl(VM&, JSObject*, JSValue toJSONFunction, const PropertyNameForFunctionCall&);
 
     enum StringifyResult { StringifyFailed, StringifySucceeded, StringifyFailedDueToUndefinedOrSymbolValue };
     StringifyResult appendStringifiedValue(StringBuilder&, JSValue, const Holder&, const PropertyNameForFunctionCall&);
@@ -284,38 +284,35 @@ JSValue Stringifier::stringify(JSValue value)
     return jsString(m_exec, result.toString());
 }
 
-ALWAYS_INLINE JSValue Stringifier::toJSON(JSValue value, const PropertyNameForFunctionCall& propertyName)
+ALWAYS_INLINE JSValue Stringifier::toJSON(JSObject* object, const PropertyNameForFunctionCall& propertyName)
 {
     VM& vm = m_exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
     scope.assertNoException();
-    if (!value.isObject())
-        return value;
-    
-    JSObject* object = asObject(value);
+
     PropertySlot slot(object, PropertySlot::InternalMethodType::Get);
     bool hasProperty = object->getPropertySlot(m_exec, vm.propertyNames->toJSON, slot);
     EXCEPTION_ASSERT(!scope.exception() || !hasProperty);
     if (!hasProperty)
-        return value;
+        return object;
 
     JSValue toJSONFunction = slot.getValue(m_exec, vm.propertyNames->toJSON);
     RETURN_IF_EXCEPTION(scope, { });
     scope.release();
-    return toJSONImpl(vm, value, toJSONFunction, propertyName);
+    return toJSONImpl(vm, object, toJSONFunction, propertyName);
 }
 
-JSValue Stringifier::toJSONImpl(VM& vm, JSValue value, JSValue toJSONFunction, const PropertyNameForFunctionCall& propertyName)
+JSValue Stringifier::toJSONImpl(VM& vm, JSObject* object, JSValue toJSONFunction, const PropertyNameForFunctionCall& propertyName)
 {
     CallType callType;
     CallData callData;
     if (!toJSONFunction.isCallable(vm, callType, callData))
-        return value;
+        return object;
 
     MarkedArgumentBuffer args;
     args.append(propertyName.value(m_exec));
     ASSERT(!args.hasOverflowed());
-    return call(m_exec, asObject(toJSONFunction), callType, callData, value, args);
+    return call(m_exec, asObject(toJSONFunction), callType, callData, object, args);
 }
 
 Stringifier::StringifyResult Stringifier::appendStringifiedValue(StringBuilder& builder, JSValue value, const Holder& holder, const PropertyNameForFunctionCall& propertyName)
@@ -324,8 +321,10 @@ Stringifier::StringifyResult Stringifier::appendStringifiedValue(StringBuilder&
     auto scope = DECLARE_THROW_SCOPE(vm);
 
     // Call the toJSON function.
-    value = toJSON(value, propertyName);
-    RETURN_IF_EXCEPTION(scope, StringifyFailed);
+    if (value.isObject()) {
+        value = toJSON(asObject(value), propertyName);
+        RETURN_IF_EXCEPTION(scope, StringifyFailed);
+    }
 
     // Call the replacer function.
     if (isCallableReplacer()) {
@@ -448,23 +447,15 @@ inline void Stringifier::startNewLine(StringBuilder& builder) const
 
 inline Stringifier::Holder::Holder(ExecState* exec, JSObject* object)
     : m_object(object)
+    , m_isJSArray(isJSArray(object))
     , m_isArray(JSC::isArray(exec, object))
-    , m_isJSArray(m_isArray && isJSArray(object))
-    , m_index(0)
-#ifndef NDEBUG
-    , m_size(0)
-#endif
 {
 }
 
 inline Stringifier::Holder::Holder(RootHolderTag, JSObject* object)
     : m_object(object)
-    , m_isArray(false)
     , m_isJSArray(false)
-    , m_index(0)
-#ifndef NDEBUG
-    , m_size(0)
-#endif
+    , m_isArray(false)
 {
 }
 
@@ -523,7 +514,7 @@ bool Stringifier::Holder::appendNextProperty(Stringifier& stringifier, StringBui
             value = asArray(m_object)->getIndexQuickly(index);
         else {
             PropertySlot slot(m_object, PropertySlot::InternalMethodType::Get);
-            if (m_object->methodTable(vm)->getOwnPropertySlotByIndex(m_object, exec, index, slot))
+            if (m_object->getPropertySlot(exec, index, slot))
                 value = slot.getValue(exec, index);
             else
                 value = jsUndefined();
@@ -542,7 +533,7 @@ bool Stringifier::Holder::appendNextProperty(Stringifier& stringifier, StringBui
         // Get the value.
         PropertySlot slot(m_object, PropertySlot::InternalMethodType::Get);
         Identifier& propertyName = m_propertyNames->propertyNameVector()[index];
-        if (!m_object->methodTable(vm)->getOwnPropertySlot(m_object, exec, propertyName, slot))
+        if (!m_object->getPropertySlot(exec, propertyName, slot))
             return true;
         JSValue value = slot.getValue(exec, propertyName);
         RETURN_IF_EXCEPTION(scope, false);