<!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>[207178] 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/207178">207178</a></dd>
<dt>Author</dt> <dd>mark.lam@apple.com</dd>
<dt>Date</dt> <dd>2016-10-11 16:25:38 -0700 (Tue, 11 Oct 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>Array.prototype.concat should not modify frozen objects.
https://bugs.webkit.org/show_bug.cgi?id=163302
Reviewed by Filip Pizlo.
JSTests:
* stress/array-concat-on-frozen-object.js: Added.
Source/JavaScriptCore:
The ES6 spec for Array.prototype.concat states that it uses the
CreateDataPropertyOrThrow() to add items to the result array. The spec for
CreateDataPropertyOrThrow states:
"This abstract operation creates a property whose attributes are set to the same
defaults used for properties created by the ECMAScript language assignment
operator. Normally, the property will not already exist. If it does exist and is
not configurable or if O is not extensible, [[DefineOwnProperty]] will return
false causing this operation to throw a TypeError exception."
Since the properties of frozen objects are not extensible, not configurable, and
not writable, Array.prototype.concat should fail to write to the result array if
it is frozen.
Ref: https://tc39.github.io/ecma262/#sec-array.prototype.concat,
https://tc39.github.io/ecma262/#sec-createdatapropertyorthrow, and
https://tc39.github.io/ecma262/#sec-createdataproperty.
The fix consists of 2 parts:
1. moveElement() should use the PutDirectIndexShouldThrow mode when invoking
putDirectIndex(), and
2. SparseArrayValueMap::putDirect() should check for the case where the property
is read only.
(2) ensures that we don't write into a non-writable property.
(1) ensures that we throw a TypeError for attempts to write to a non-writeable
property.
* runtime/ArrayPrototype.cpp:
(JSC::moveElements):
* runtime/SparseArrayValueMap.cpp:
(JSC::SparseArrayValueMap::putDirect):</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="#trunkSourceJavaScriptCoreruntimeArrayPrototypecpp">trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeSparseArrayValueMapcpp">trunk/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsstressarrayconcatonfrozenobjectjs">trunk/JSTests/stress/array-concat-on-frozen-object.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (207177 => 207178)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog 2016-10-11 23:24:46 UTC (rev 207177)
+++ trunk/JSTests/ChangeLog 2016-10-11 23:25:38 UTC (rev 207178)
</span><span class="lines">@@ -1,3 +1,12 @@
</span><ins>+2016-10-11 Mark Lam <mark.lam@apple.com>
+
+ Array.prototype.concat should not modify frozen objects.
+ https://bugs.webkit.org/show_bug.cgi?id=163302
+
+ Reviewed by Filip Pizlo.
+
+ * stress/array-concat-on-frozen-object.js: Added.
+
</ins><span class="cx"> 2016-10-11 Saam Barati <sbarati@apple.com>
</span><span class="cx">
</span><span class="cx"> ValueAdd should be constant folded if the operands are constant String,Primitive or Primitive,String
</span></span></pre></div>
<a id="trunkJSTestsstressarrayconcatonfrozenobjectjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/array-concat-on-frozen-object.js (0 => 207178)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/array-concat-on-frozen-object.js (rev 0)
+++ trunk/JSTests/stress/array-concat-on-frozen-object.js 2016-10-11 23:25:38 UTC (rev 207178)
</span><span class="lines">@@ -0,0 +1,72 @@
</span><ins>+//@ runFTLNoCJIT
+
+let totalFailed = 0;
+
+function shouldEqual(testId, actual, expected) {
+ if (actual != expected) {
+ throw testId + ": ERROR: expect " + expected + ", actual " + actual;
+ }
+}
+
+function makeArray() {
+ return ['unmodifiable'];
+}
+
+function makeArrayLikeObject() {
+ var obj = {};
+ obj[0] = 'unmodifiable';
+ obj.length = 1;
+ return obj;
+}
+
+function emptyArraySourceMaker() {
+ return [];
+}
+
+function singleElementArraySourceMaker() {
+ return ['modified_1'];
+}
+
+// Make test functions with unique codeblocks.
+function makeConcatTest(testId) {
+ return new Function("arr", "return arr.concat(['" + testId + "'])");
+}
+function makeConcatOnHoleyArrayTest(testId) {
+ return new Function("arr", "var other = ['" + testId + "']; other[1000] = '" + testId + "'; return arr.concat(other);");
+}
+
+let numIterations = 10000;
+
+function runTest(testId, testMaker, targetMaker, sourceMaker, expectedValue, expectedException) {
+ var test = testMaker(testId);
+ noInline(test);
+
+ for (var i = 0; i < numIterations; i++) {
+ var exception = undefined;
+
+ var obj = targetMaker();
+ obj = Object.freeze(obj);
+
+ var arr = sourceMaker();
+ arr.constructor = { [Symbol.species]: function() { return obj; } };
+
+ try {
+ test(arr);
+ } catch (e) {
+ exception = "" + e;
+ exception = exception.substr(0, 10); // Search for "TypeError:".
+ }
+ shouldEqual(testId, exception, expectedException);
+ shouldEqual(testId, obj[0], expectedValue);
+ }
+}
+
+runTest(10010, makeConcatTest, makeArray, emptyArraySourceMaker, "unmodifiable", "TypeError:");
+runTest(10011, makeConcatTest, makeArray, singleElementArraySourceMaker, "unmodifiable", "TypeError:");
+runTest(10020, makeConcatTest, makeArrayLikeObject, emptyArraySourceMaker, "unmodifiable", "TypeError:");
+runTest(10021, makeConcatTest, makeArrayLikeObject, singleElementArraySourceMaker, "unmodifiable", "TypeError:");
+
+runTest(10110, makeConcatOnHoleyArrayTest, makeArray, emptyArraySourceMaker, "unmodifiable", "TypeError:");
+runTest(10111, makeConcatOnHoleyArrayTest, makeArray, singleElementArraySourceMaker, "unmodifiable", "TypeError:");
+runTest(10120, makeConcatOnHoleyArrayTest, makeArrayLikeObject, emptyArraySourceMaker, "unmodifiable", "TypeError:");
+runTest(10121, makeConcatOnHoleyArrayTest, makeArrayLikeObject, singleElementArraySourceMaker, "unmodifiable", "TypeError:");
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (207177 => 207178)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-10-11 23:24:46 UTC (rev 207177)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-10-11 23:25:38 UTC (rev 207178)
</span><span class="lines">@@ -1,3 +1,43 @@
</span><ins>+2016-10-11 Mark Lam <mark.lam@apple.com>
+
+ Array.prototype.concat should not modify frozen objects.
+ https://bugs.webkit.org/show_bug.cgi?id=163302
+
+ Reviewed by Filip Pizlo.
+
+ The ES6 spec for Array.prototype.concat states that it uses the
+ CreateDataPropertyOrThrow() to add items to the result array. The spec for
+ CreateDataPropertyOrThrow states:
+
+ "This abstract operation creates a property whose attributes are set to the same
+ defaults used for properties created by the ECMAScript language assignment
+ operator. Normally, the property will not already exist. If it does exist and is
+ not configurable or if O is not extensible, [[DefineOwnProperty]] will return
+ false causing this operation to throw a TypeError exception."
+
+ Since the properties of frozen objects are not extensible, not configurable, and
+ not writable, Array.prototype.concat should fail to write to the result array if
+ it is frozen.
+
+ Ref: https://tc39.github.io/ecma262/#sec-array.prototype.concat,
+ https://tc39.github.io/ecma262/#sec-createdatapropertyorthrow, and
+ https://tc39.github.io/ecma262/#sec-createdataproperty.
+
+ The fix consists of 2 parts:
+ 1. moveElement() should use the PutDirectIndexShouldThrow mode when invoking
+ putDirectIndex(), and
+ 2. SparseArrayValueMap::putDirect() should check for the case where the property
+ is read only.
+
+ (2) ensures that we don't write into a non-writable property.
+ (1) ensures that we throw a TypeError for attempts to write to a non-writeable
+ property.
+
+ * runtime/ArrayPrototype.cpp:
+ (JSC::moveElements):
+ * runtime/SparseArrayValueMap.cpp:
+ (JSC::SparseArrayValueMap::putDirect):
+
</ins><span class="cx"> 2016-10-11 Yusuke Suzuki <utatane.tea@gmail.com>
</span><span class="cx">
</span><span class="cx"> [DOMJIT] DOMJIT::Patchpoint should have a way to receive constant folded arguments
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeArrayPrototypecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp (207177 => 207178)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp 2016-10-11 23:24:46 UTC (rev 207177)
+++ trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp 2016-10-11 23:25:38 UTC (rev 207178)
</span><span class="lines">@@ -1078,7 +1078,7 @@
</span><span class="cx"> for (unsigned i = 0; i < sourceLength; ++i) {
</span><span class="cx"> JSValue value = source->tryGetIndexQuickly(i);
</span><span class="cx"> if (value) {
</span><del>- target->putDirectIndex(exec, targetOffset + i, value);
</del><ins>+ target->putDirectIndex(exec, targetOffset + i, value, 0, PutDirectIndexShouldThrow);
</ins><span class="cx"> RETURN_IF_EXCEPTION(scope, false);
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="lines">@@ -1087,7 +1087,7 @@
</span><span class="cx"> JSValue value = getProperty(exec, source, i);
</span><span class="cx"> RETURN_IF_EXCEPTION(scope, false);
</span><span class="cx"> if (value) {
</span><del>- target->putDirectIndex(exec, targetOffset + i, value);
</del><ins>+ target->putDirectIndex(exec, targetOffset + i, value, 0, PutDirectIndexShouldThrow);
</ins><span class="cx"> RETURN_IF_EXCEPTION(scope, false);
</span><span class="cx"> }
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeSparseArrayValueMapcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp (207177 => 207178)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp 2016-10-11 23:24:46 UTC (rev 207177)
+++ trunk/Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp 2016-10-11 23:25:38 UTC (rev 207178)
</span><span class="lines">@@ -117,14 +117,17 @@
</span><span class="cx"> AddResult result = add(array, i);
</span><span class="cx"> SparseArrayEntry& entry = result.iterator->value;
</span><span class="cx">
</span><del>- // To save a separate find & add, we first always add to the sparse map.
- // In the uncommon case that this is a new property, and the array is not
- // extensible, this is not the right thing to have done - so remove again.
- if (mode != PutDirectIndexLikePutDirect && result.isNewEntry && !array->isStructureExtensible()) {
- remove(result.iterator);
- return reject(exec, mode == PutDirectIndexShouldThrow, "Attempting to define property on object that is not extensible.");
</del><ins>+ if (mode != PutDirectIndexLikePutDirect && !array->isStructureExtensible()) {
+ // To save a separate find & add, we first always add to the sparse map.
+ // In the uncommon case that this is a new property, and the array is not
+ // extensible, this is not the right thing to have done - so remove again.
+ if (result.isNewEntry) {
+ remove(result.iterator);
+ return reject(exec, mode == PutDirectIndexShouldThrow, "Attempting to define property on object that is not extensible.");
+ }
+ if (entry.attributes & ReadOnly)
+ return reject(exec, mode == PutDirectIndexShouldThrow, ReadonlyPropertyWriteError);
</ins><span class="cx"> }
</span><del>-
</del><span class="cx"> entry.attributes = attributes;
</span><span class="cx"> entry.set(exec->vm(), this, value);
</span><span class="cx"> return true;
</span></span></pre>
</div>
</div>
</body>
</html>