Changeset 233918 in webkit

Timestamp:

Jul 18, 2018 11:31:09 AM (6 years ago)

Author:

Yusuke Suzuki

Message:

[JSC] JSON.stringify's replacer should use isArray instead of JSArray checks
​https://bugs.webkit.org/show_bug.cgi?id=187755

Reviewed by Mark Lam.

JSTests:

• stress/json-stringify-gap-calculation-should-be-after-replacer-check.js: Added.

(shouldThrow):
(shouldThrow.string.toString):

• test262/expectations.yaml:

Source/JavaScriptCore:

JSON.stringify used inherits<JSArray>(vm) to determine whether the given replacer is an array replacer.
But this is wrong. According to the spec, we should use isArray[1], which accepts Proxies. This difference
makes one test262 test failed.

This patch changes the code to using isArray(). And we reorder the evaluations of replacer check and ident space check
to align these checks to the spec's order.

[1]: ​https://tc39.github.io/ecma262/#sec-json.stringify

• runtime/JSONObject.cpp:

(JSC::Stringifier::Stringifier):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/json-stringify-gap-calculation-should-be-after-replacer-check.js (added)
• JSTests/test262/expectations.yaml (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/JSONObject.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+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
+
+        Reviewed by Mark Lam.
+
+        * stress/json-stringify-gap-calculation-should-be-after-replacer-check.js: Added.
+        (shouldThrow):
+        (shouldThrow.string.toString):
+        * test262/expectations.yaml:
+
 2018-07-12  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/JSTests/test262/expectations.yaml

@@ -1035 +1035 @@
   default: 'Test262Error: Expected a Test262Error to be thrown but no exception was thrown at all'
   strict mode: 'Test262Error: Expected a Test262Error to be thrown but no exception was thrown at all'
-test/built-ins/JSON/stringify/replacer-proxy-revoked.js:
-  default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
-  strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
 test/built-ins/Map/proto-from-ctor-realm.js:
   default: 'Test262Error: Expected SameValue(«[object Map]», «[object Map]») to be true'

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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
+
+        Reviewed by Mark Lam.
+
+        JSON.stringify used `inherits<JSArray>(vm)` to determine whether the given replacer is an array replacer.
+        But this is wrong. According to the spec, we should use `isArray`[1], which accepts Proxies. This difference
+        makes one test262 test failed.
+
+        This patch changes the code to using `isArray()`. And we reorder the evaluations of replacer check and ident space check
+        to align these checks to the spec's order.
+
+        [1]: https://tc39.github.io/ecma262/#sec-json.stringify
+
+        * runtime/JSONObject.cpp:
+        (JSC::Stringifier::Stringifier):
+
 2018-07-18  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/Source/JavaScriptCore/runtime/JSONObject.cpp

@@ -227 +227 @@
     auto scope = DECLARE_THROW_SCOPE(vm);
 
+    if (m_replacer.isObject()) {
+        JSObject* replacerObject = asObject(m_replacer);
+
+        m_replacerCallType = CallType::None;
+        if (!replacerObject->isCallable(vm, m_replacerCallType, m_replacerCallData)) {
+            bool isArrayReplacer = JSC::isArray(exec, replacerObject);
+            RETURN_IF_EXCEPTION(scope, );
+            if (isArrayReplacer) {
+                m_usingArrayReplacer = true;
+                unsigned length = replacerObject->get(exec, vm.propertyNames->length).toUInt32(exec);
+                RETURN_IF_EXCEPTION(scope, );
+                for (unsigned i = 0; i < length; ++i) {
+                    JSValue name = replacerObject->get(exec, i);
+                    RETURN_IF_EXCEPTION(scope, );
+                    if (name.isObject()) {
+                        auto* nameObject = jsCast<JSObject*>(name);
+                        if (!nameObject->inherits<NumberObject>(vm) && !nameObject->inherits<StringObject>(vm))
+                            continue;
+                    } else if (!name.isNumber() && !name.isString())
+                        continue;
+                    m_arrayReplacerPropertyNames.add(name.toString(exec)->toIdentifier(exec));
+                    RETURN_IF_EXCEPTION(scope, );
+                }
+            }
+        }
+    }
+
+    scope.release();
     m_gap = gap(exec, space);
-    if (UNLIKELY(scope.exception()))
-        return;
-
-    if (!m_replacer.isObject())
-        return;
-
-    JSObject* replacerObject = asObject(m_replacer);
-    if (replacerObject->inherits<JSArray>(vm)) {
-        m_usingArrayReplacer = true;
-        JSArray* array = jsCast<JSArray*>(replacerObject);
-        unsigned length = array->get(exec, vm.propertyNames->length).toUInt32(exec);
-        if (UNLIKELY(scope.exception()))
-            return;
-        for (unsigned i = 0; i < length; ++i) {
-            JSValue name = array->get(exec, i);
-            if (UNLIKELY(scope.exception()))
-                break;
-
-            if (auto* nameObject = jsDynamicCast<JSObject*>(vm, name)) {
-                if (!nameObject->inherits<NumberObject>(vm) && !nameObject->inherits<StringObject>(vm))
-                    continue;
-            } else if (!name.isNumber() && !name.isString())
-                continue;
-
-            m_arrayReplacerPropertyNames.add(name.toString(exec)->toIdentifier(exec));
-        }
-        return;
-    }
-
-    m_replacerCallType = replacerObject->methodTable(vm)->getCallData(replacerObject, m_replacerCallData);
 }