<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[208123] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/208123">208123</a></dd>
<dt>Author</dt> <dd>utatane.tea@gmail.com</dd>
<dt>Date</dt> <dd>2016-10-30 02:14:37 -0700 (Sun, 30 Oct 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>[JSC] JSON.stringify should handle Proxy which is non JSArray but isArray is true
https://bugs.webkit.org/show_bug.cgi?id=164123
Reviewed by Mark Lam.
JSTests:
* stress/json-stringify-with-non-jsarray-array.js: Added.
(shouldBe):
(shouldBe.JSON.stringify.new.Proxy):
Source/JavaScriptCore:
When JSON.stringify encounter the undefined value, the result depends
on the context. If it is produced under the object property context, we ignore
that property. On the other hand, if it is produced under the array element
context, we produce "null".
For example,
// https://tc39.github.io/ecma262/#sec-serializejsonobject section 8.b.
// Skip the property that value is undefined.
JSON.stringify({ value: undefined }); // => "{}"
// https://tc39.github.io/ecma262/#sec-serializejsonarray section 8.b.
// Write "null" when the element is undefined.
JSON.stringify([undefined]); // => "[null]"
At that time, we decide the context based on the `holder->inherits(JSArray::info())`.
But it is not correct since we have a holder that creates the array element context
but it is not JSArray subtype. ES6 Proxy to an array is one example. In that case,
`isArray(exec, proxy)` returns `true`, but `inherits(JSArray::info())` returns false.
Since we already have this `isArray()` value in Stringifier::Holder, we should reuse
this here. And this is the correct behavior in the ES6 spec.
* runtime/JSONObject.cpp:
(JSC::Stringifier::Holder::isArray):
(JSC::Stringifier::stringify):
(JSC::Stringifier::appendStringifiedValue):
(JSC::Stringifier::Holder::Holder):
(JSC::Stringifier::Holder::appendNextProperty):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkJSTestsChangeLog">trunk/JSTests/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeJSONObjectcpp">trunk/Source/JavaScriptCore/runtime/JSONObject.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsstressjsonstringifywithnonjsarrayarrayjs">trunk/JSTests/stress/json-stringify-with-non-jsarray-array.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (208122 => 208123)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog 2016-10-30 09:14:03 UTC (rev 208122)
+++ trunk/JSTests/ChangeLog 2016-10-30 09:14:37 UTC (rev 208123)
</span><span class="lines">@@ -1,3 +1,14 @@
</span><ins>+2016-10-29 Yusuke Suzuki <utatane.tea@gmail.com>
+
+ [JSC] JSON.stringify should handle Proxy which is non JSArray but isArray is true
+ https://bugs.webkit.org/show_bug.cgi?id=164123
+
+ Reviewed by Mark Lam.
+
+ * stress/json-stringify-with-non-jsarray-array.js: Added.
+ (shouldBe):
+ (shouldBe.JSON.stringify.new.Proxy):
+
</ins><span class="cx"> 2016-10-29 Saam Barati <sbarati@apple.com>
</span><span class="cx">
</span><span class="cx"> We should have a way of profiling when a get_by_id is pure and to emit a PureGetById in the DFG/FTL
</span></span></pre></div>
<a id="trunkJSTestsstressjsonstringifywithnonjsarrayarrayjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/json-stringify-with-non-jsarray-array.js (0 => 208123)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/json-stringify-with-non-jsarray-array.js (rev 0)
+++ trunk/JSTests/stress/json-stringify-with-non-jsarray-array.js 2016-10-30 09:14:37 UTC (rev 208123)
</span><span class="lines">@@ -0,0 +1,36 @@
</span><ins>+function shouldBe(actual, expected)
+{
+ if (actual !== expected)
+ throw new Error(`bad value: ${String(actual)}`);
+}
+
+function shouldThrow(func, errorMessage)
+{
+ var errorThrown = false;
+ var error = null;
+ try {
+ func();
+ } catch (e) {
+ errorThrown = true;
+ error = e;
+ }
+ if (!errorThrown)
+ throw new Error('not thrown');
+ if (String(error) !== errorMessage)
+ throw new Error(`bad error: ${String(error)}`);
+}
+
+shouldBe(JSON.stringify(new Proxy([undefined], {})), `[null]`);
+shouldBe(JSON.stringify(new Proxy([function() { }], {})), `[null]`);
+shouldBe(JSON.stringify(new Proxy({ value: undefined }, {})), `{}`);
+
+shouldThrow(function () {
+ let proxy = new Proxy([], {
+ get(theTarget, propName) {
+ if (propName === 'length')
+ throw new Error('ok');
+ return 42;
+ }
+ });
+ JSON.stringify(proxy);
+}, `Error: ok`);
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (208122 => 208123)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-10-30 09:14:03 UTC (rev 208122)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-10-30 09:14:37 UTC (rev 208123)
</span><span class="lines">@@ -1,3 +1,38 @@
</span><ins>+2016-10-29 Yusuke Suzuki <utatane.tea@gmail.com>
+
+ [JSC] JSON.stringify should handle Proxy which is non JSArray but isArray is true
+ https://bugs.webkit.org/show_bug.cgi?id=164123
+
+ Reviewed by Mark Lam.
+
+ When JSON.stringify encounter the undefined value, the result depends
+ on the context. If it is produced under the object property context, we ignore
+ that property. On the other hand, if it is produced under the array element
+ context, we produce "null".
+
+ For example,
+ // https://tc39.github.io/ecma262/#sec-serializejsonobject section 8.b.
+ // Skip the property that value is undefined.
+ JSON.stringify({ value: undefined }); // => "{}"
+
+ // https://tc39.github.io/ecma262/#sec-serializejsonarray section 8.b.
+ // Write "null" when the element is undefined.
+ JSON.stringify([undefined]); // => "[null]"
+
+ At that time, we decide the context based on the `holder->inherits(JSArray::info())`.
+ But it is not correct since we have a holder that creates the array element context
+ but it is not JSArray subtype. ES6 Proxy to an array is one example. In that case,
+ `isArray(exec, proxy)` returns `true`, but `inherits(JSArray::info())` returns false.
+ Since we already have this `isArray()` value in Stringifier::Holder, we should reuse
+ this here. And this is the correct behavior in the ES6 spec.
+
+ * runtime/JSONObject.cpp:
+ (JSC::Stringifier::Holder::isArray):
+ (JSC::Stringifier::stringify):
+ (JSC::Stringifier::appendStringifiedValue):
+ (JSC::Stringifier::Holder::Holder):
+ (JSC::Stringifier::Holder::appendNextProperty):
+
</ins><span class="cx"> 2016-10-29 Saam Barati <sbarati@apple.com>
</span><span class="cx">
</span><span class="cx"> We should have a way of profiling when a get_by_id is pure and to emit a PureGetById in the DFG/FTL
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeJSONObjectcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/JSONObject.cpp (208122 => 208123)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/JSONObject.cpp 2016-10-30 09:14:03 UTC (rev 208122)
+++ trunk/Source/JavaScriptCore/runtime/JSONObject.cpp 2016-10-30 09:14:37 UTC (rev 208123)
</span><span class="lines">@@ -93,9 +93,12 @@
</span><span class="cx"> private:
</span><span class="cx"> class Holder {
</span><span class="cx"> public:
</span><ins>+ enum RootHolderTag { RootHolder };
</ins><span class="cx"> Holder(VM&, ExecState*, JSObject*);
</span><ins>+ Holder(RootHolderTag, VM&, JSObject*);
</ins><span class="cx">
</span><span class="cx"> JSObject* object() const { return m_object.get(); }
</span><ins>+ bool isArray() const { return m_isArray; }
</ins><span class="cx">
</span><span class="cx"> bool appendNextProperty(Stringifier&, StringBuilder&);
</span><span class="cx">
</span><span class="lines">@@ -102,7 +105,7 @@
</span><span class="cx"> private:
</span><span class="cx"> Local<JSObject> m_object;
</span><span class="cx"> const bool m_isArray;
</span><del>- bool m_isJSArray;
</del><ins>+ const bool m_isJSArray;
</ins><span class="cx"> unsigned m_index;
</span><span class="cx"> unsigned m_size;
</span><span class="cx"> RefPtr<PropertyNameArrayData> m_propertyNames;
</span><span class="lines">@@ -114,7 +117,7 @@
</span><span class="cx"> JSValue toJSONImpl(JSValue value, JSValue toJSONFunction, const PropertyNameForFunctionCall&);
</span><span class="cx">
</span><span class="cx"> enum StringifyResult { StringifyFailed, StringifySucceeded, StringifyFailedDueToUndefinedOrSymbolValue };
</span><del>- StringifyResult appendStringifiedValue(StringBuilder&, JSValue, JSObject* holder, const PropertyNameForFunctionCall&);
</del><ins>+ StringifyResult appendStringifiedValue(StringBuilder&, JSValue, const Holder&, const PropertyNameForFunctionCall&);
</ins><span class="cx">
</span><span class="cx"> bool willIndent() const;
</span><span class="cx"> void indent();
</span><span class="lines">@@ -259,7 +262,8 @@
</span><span class="cx"> object->putDirect(vm, vm.propertyNames->emptyIdentifier, value.get());
</span><span class="cx">
</span><span class="cx"> StringBuilder result;
</span><del>- if (appendStringifiedValue(result, value.get(), object, emptyPropertyName) != StringifySucceeded)
</del><ins>+ Holder root(Holder::RootHolder, vm, object);
+ if (appendStringifiedValue(result, value.get(), root, emptyPropertyName) != StringifySucceeded)
</ins><span class="cx"> return Local<Unknown>(vm, jsUndefined());
</span><span class="cx"> RETURN_IF_EXCEPTION(scope, Local<Unknown>(vm, jsNull()));
</span><span class="cx">
</span><span class="lines">@@ -296,7 +300,7 @@
</span><span class="cx"> return call(m_exec, asObject(toJSONFunction), callType, callData, value, args);
</span><span class="cx"> }
</span><span class="cx">
</span><del>-Stringifier::StringifyResult Stringifier::appendStringifiedValue(StringBuilder& builder, JSValue value, JSObject* holder, const PropertyNameForFunctionCall& propertyName)
</del><ins>+Stringifier::StringifyResult Stringifier::appendStringifiedValue(StringBuilder& builder, JSValue value, const Holder& holder, const PropertyNameForFunctionCall& propertyName)
</ins><span class="cx"> {
</span><span class="cx"> VM& vm = m_exec->vm();
</span><span class="cx"> auto scope = DECLARE_THROW_SCOPE(vm);
</span><span class="lines">@@ -310,11 +314,11 @@
</span><span class="cx"> MarkedArgumentBuffer args;
</span><span class="cx"> args.append(propertyName.value(m_exec));
</span><span class="cx"> args.append(value);
</span><del>- value = call(m_exec, m_replacer.get(), m_replacerCallType, m_replacerCallData, holder, args);
</del><ins>+ value = call(m_exec, m_replacer.get(), m_replacerCallType, m_replacerCallData, holder.object(), args);
</ins><span class="cx"> RETURN_IF_EXCEPTION(scope, StringifyFailed);
</span><span class="cx"> }
</span><span class="cx">
</span><del>- if ((value.isUndefined() || value.isSymbol()) && !holder->inherits(JSArray::info()))
</del><ins>+ if ((value.isUndefined() || value.isSymbol()) && !holder.isArray())
</ins><span class="cx"> return StringifyFailedDueToUndefinedOrSymbolValue;
</span><span class="cx">
</span><span class="cx"> if (value.isNull()) {
</span><span class="lines">@@ -359,7 +363,7 @@
</span><span class="cx">
</span><span class="cx"> CallData callData;
</span><span class="cx"> if (object->methodTable()->getCallData(object, callData) != CallType::None) {
</span><del>- if (holder->inherits(JSArray::info())) {
</del><ins>+ if (holder.isArray()) {
</ins><span class="cx"> builder.appendLiteral("null");
</span><span class="cx"> return StringifySucceeded;
</span><span class="cx"> }
</span><span class="lines">@@ -419,7 +423,8 @@
</span><span class="cx">
</span><span class="cx"> inline Stringifier::Holder::Holder(VM& vm, ExecState* exec, JSObject* object)
</span><span class="cx"> : m_object(vm, object)
</span><del>- , m_isArray(isArray(exec, object))
</del><ins>+ , m_isArray(JSC::isArray(exec, object))
+ , m_isJSArray(m_isArray && isJSArray(object))
</ins><span class="cx"> , m_index(0)
</span><span class="cx"> #ifndef NDEBUG
</span><span class="cx"> , m_size(0)
</span><span class="lines">@@ -427,6 +432,17 @@
</span><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+inline Stringifier::Holder::Holder(RootHolderTag, VM& vm, JSObject* object)
+ : m_object(vm, object)
+ , m_isArray(false)
+ , m_isJSArray(false)
+ , m_index(0)
+#ifndef NDEBUG
+ , m_size(0)
+#endif
+{
+}
+
</ins><span class="cx"> bool Stringifier::Holder::appendNextProperty(Stringifier& stringifier, StringBuilder& builder)
</span><span class="cx"> {
</span><span class="cx"> ASSERT(m_index <= m_size);
</span><span class="lines">@@ -438,11 +454,12 @@
</span><span class="cx"> // First time through, initialize.
</span><span class="cx"> if (!m_index) {
</span><span class="cx"> if (m_isArray) {
</span><del>- m_isJSArray = isJSArray(m_object.get());
</del><span class="cx"> if (m_isJSArray)
</span><span class="cx"> m_size = asArray(m_object.get())->length();
</span><del>- else
</del><ins>+ else {
</ins><span class="cx"> m_size = m_object->get(exec, vm.propertyNames->length).toUInt32(exec);
</span><ins>+ RETURN_IF_EXCEPTION(scope, false);
+ }
</ins><span class="cx"> builder.append('[');
</span><span class="cx"> } else {
</span><span class="cx"> if (stringifier.m_usingArrayReplacer)
</span><span class="lines">@@ -492,7 +509,8 @@
</span><span class="cx"> stringifier.startNewLine(builder);
</span><span class="cx">
</span><span class="cx"> // Append the stringified value.
</span><del>- stringifyResult = stringifier.appendStringifiedValue(builder, value, m_object.get(), index);
</del><ins>+ stringifyResult = stringifier.appendStringifiedValue(builder, value, *this, index);
+ ASSERT(stringifyResult != StringifyFailedDueToUndefinedOrSymbolValue);
</ins><span class="cx"> } else {
</span><span class="cx"> // Get the value.
</span><span class="cx"> PropertySlot slot(m_object.get(), PropertySlot::InternalMethodType::Get);
</span><span class="lines">@@ -516,7 +534,7 @@
</span><span class="cx"> builder.append(' ');
</span><span class="cx">
</span><span class="cx"> // Append the stringified value.
</span><del>- stringifyResult = stringifier.appendStringifiedValue(builder, value, m_object.get(), propertyName);
</del><ins>+ stringifyResult = stringifier.appendStringifiedValue(builder, value, *this, propertyName);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // From this point on, no access to the this pointer or to any members, because the
</span></span></pre>
</div>
</div>
</body>
</html>