Log Message
JSONObject Walker::walk must save array length before processing array elements. https://bugs.webkit.org/show_bug.cgi?id=153485
Reviewed by Darin Adler and Michael Saboff. Source/_javascript_Core: According to https://tc39.github.io/ecma262/#sec-internalizejsonproperty, JSON.parse() should cache the length of an array and use the cached length when iterating array elements (see section 24.3.1.1 2.b.iii). * runtime/JSONObject.cpp: (JSC::Walker::walk): * tests/stress/JSON-parse-should-cache-array-lengths.js: Added. (toString): (shouldBe): (test): (test2): LayoutTests: * js/JSON-parse-reviver-expected.txt: * js/script-tests/JSON-parse-reviver.js: - Fixed a bug in arrayReviver() where it was setting the array length to 3, but was immediately returning a value from the reviver for index 3. This effectively forces array.length to 4. As a result, case 4 always failed silently, and case 5 never executed. - Added tracking of cases visited by the revivers so that they can be verified.
Modified Paths
- trunk/LayoutTests/ChangeLog
- trunk/LayoutTests/js/JSON-parse-reviver-expected.txt
- trunk/LayoutTests/js/script-tests/JSON-parse-reviver.js
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/runtime/JSONObject.cpp
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (203228 => 203229)
--- trunk/LayoutTests/ChangeLog 2016-07-14 17:59:17 UTC (rev 203228)
+++ trunk/LayoutTests/ChangeLog 2016-07-14 18:18:55 UTC (rev 203229)
@@ -1,3 +1,18 @@
+2016-07-14 Mark Lam <[email protected]>
+
+ JSONObject Walker::walk must save array length before processing array elements.
+ https://bugs.webkit.org/show_bug.cgi?id=153485
+
+ Reviewed by Darin Adler and Michael Saboff.
+
+ * js/JSON-parse-reviver-expected.txt:
+ * js/script-tests/JSON-parse-reviver.js:
+ - Fixed a bug in arrayReviver() where it was setting the array length to 3,
+ but was immediately returning a value from the reviver for index 3. This
+ effectively forces array.length to 4. As a result, case 4 always failed
+ silently, and case 5 never executed.
+ - Added tracking of cases visited by the revivers so that they can be verified.
+
2016-07-14 Youenn Fablet <[email protected]>
DOM value iterable interfaces should use Array prototype methods
Modified: trunk/LayoutTests/js/JSON-parse-reviver-expected.txt (203228 => 203229)
--- trunk/LayoutTests/js/JSON-parse-reviver-expected.txt 2016-07-14 17:59:17 UTC (rev 203228)
+++ trunk/LayoutTests/js/JSON-parse-reviver-expected.txt 2016-07-14 18:18:55 UTC (rev 203229)
@@ -51,6 +51,15 @@
Ensure that when visiting a deleted property value is undefined
PASS value is undefined.
+Ensure the holder for our array is indeed an array
+PASS Array.isArray(currentHolder) is true
+PASS currentHolder.length is 4
+
+Ensure that we always get the same holder
+PASS currentHolder is lastHolder
+PASS Ensured that property was visited despite Array length being reduced.
+PASS value is undefined.
+
Ensure that we created the root holder as specified in ES5
PASS '' in lastHolder is true
PASS result is lastHolder['']
@@ -57,6 +66,7 @@
Ensure that a deleted value is revived if the reviver function returns a value
PASS result.hasOwnProperty(3) is true
+PASS casesVisited is [0,1,2,3,4,5]
Test behaviour of revivor used in conjunction with an object
PASS currentHolder != globalObject is true
@@ -98,6 +108,7 @@
PASS result.hasOwnProperty('a property') is false
PASS result.hasOwnProperty('to delete') is true
PASS result is lastHolder['']
+PASS casesVisited is ['a property', 'another property', 'and another property', 'to delete']
Test behaviour of revivor that introduces a cycle
PASS JSON.parse("[0,1]", reviveAddsCycle) threw exception RangeError: Maximum call stack size exceeded..
Modified: trunk/LayoutTests/js/script-tests/JSON-parse-reviver.js (203228 => 203229)
--- trunk/LayoutTests/js/script-tests/JSON-parse-reviver.js 2016-07-14 17:59:17 UTC (rev 203228)
+++ trunk/LayoutTests/js/script-tests/JSON-parse-reviver.js 2016-07-14 18:18:55 UTC (rev 203229)
@@ -2,6 +2,7 @@
if (!Array.isArray)
Array.isArray = function(o) { return o.constructor === Array; }
+let casesVisited = [];
function arrayReviver(i,v) {
if (i != "") {
currentHolder = this;
@@ -14,6 +15,7 @@
debug("Ensure that we always get the same holder");
shouldBe("currentHolder", "lastHolder");
}
+ casesVisited.push(Number(i));
switch (Number(i)) {
case 0:
v = undefined;
@@ -53,17 +55,19 @@
debug("Ensure that when visiting a deleted property value is undefined");
shouldBeUndefined("value");
v = "undelete the property";
- expectedLength = this.length = 3;
+ this.length = 3;
+ expectedLength = 4; // undeleting the value at index 3 should bump the length back to 4.
break;
case 4:
- if (this.length != 3) {
+ if (this.length != 4) {
testFailed("Did not call reviver for deleted property");
- expectedLength = this.length = 3;
+ expectedLength = this.length = 4;
break;
}
case 5:
+ casesVisited.push(5);
testPassed("Ensured that property was visited despite Array length being reduced.");
value = v;
shouldBeUndefined("value");
@@ -86,7 +90,9 @@
debug("");
debug("Ensure that a deleted value is revived if the reviver function returns a value");
shouldBeTrue("result.hasOwnProperty(3)");
+shouldBe("casesVisited", "[0,1,2,3,4,5]");
+casesVisited = [];
function objectReviver(i,v) {
if (i != "") {
currentHolder = this;
@@ -97,6 +103,7 @@
shouldBe("currentHolder", "lastHolder");
}
seen = true;
+ casesVisited.push(i);
switch (i) {
case "a property":
v = undefined;
@@ -155,6 +162,7 @@
shouldBeFalse("result.hasOwnProperty('a property')");
shouldBeTrue("result.hasOwnProperty('to delete')");
shouldBe("result", "lastHolder['']");
+shouldBe("casesVisited", "['a property', 'another property', 'and another property', 'to delete']");
debug("");
debug("Test behaviour of revivor that introduces a cycle");
Modified: trunk/Source/_javascript_Core/ChangeLog (203228 => 203229)
--- trunk/Source/_javascript_Core/ChangeLog 2016-07-14 17:59:17 UTC (rev 203228)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-07-14 18:18:55 UTC (rev 203229)
@@ -1,3 +1,22 @@
+2016-07-14 Mark Lam <[email protected]>
+
+ JSONObject Walker::walk must save array length before processing array elements.
+ https://bugs.webkit.org/show_bug.cgi?id=153485
+
+ Reviewed by Darin Adler and Michael Saboff.
+
+ According to https://tc39.github.io/ecma262/#sec-internalizejsonproperty,
+ JSON.parse() should cache the length of an array and use the cached length when
+ iterating array elements (see section 24.3.1.1 2.b.iii).
+
+ * runtime/JSONObject.cpp:
+ (JSC::Walker::walk):
+ * tests/stress/JSON-parse-should-cache-array-lengths.js: Added.
+ (toString):
+ (shouldBe):
+ (test):
+ (test2):
+
2016-07-14 Julien Brianceau <[email protected]>
[mips] Handle properly unaligned halfword load
Modified: trunk/Source/_javascript_Core/runtime/JSONObject.cpp (203228 => 203229)
--- trunk/Source/_javascript_Core/runtime/JSONObject.cpp 2016-07-14 17:59:17 UTC (rev 203228)
+++ trunk/Source/_javascript_Core/runtime/JSONObject.cpp 2016-07-14 18:18:55 UTC (rev 203229)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2009 Apple Inc. All rights reserved.
+ * Copyright (C) 2009, 2016 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -593,6 +593,7 @@
Vector<uint32_t, 16, UnsafeVectorOverflow> indexStack;
LocalStack<JSObject, 16> objectStack(m_exec->vm());
LocalStack<JSArray, 16> arrayStack(m_exec->vm());
+ Vector<unsigned, 16, UnsafeVectorOverflow> arrayLengthStack;
Vector<WalkerState, 16, UnsafeVectorOverflow> stateStack;
WalkerState state = StateUnknown;
@@ -610,6 +611,7 @@
JSArray* array = asArray(inValue);
arrayStack.push(array);
+ arrayLengthStack.append(array->length());
indexStack.append(0);
}
arrayStartVisitMember:
@@ -617,9 +619,11 @@
case ArrayStartVisitMember: {
JSArray* array = arrayStack.peek();
uint32_t index = indexStack.last();
- if (index == array->length()) {
+ unsigned arrayLength = arrayLengthStack.last();
+ if (index == arrayLength) {
outValue = array;
arrayStack.pop();
+ arrayLengthStack.removeLast();
indexStack.removeLast();
break;
}
Added: trunk/Source/_javascript_Core/tests/stress/JSON-parse-should-cache-array-lengths.js (0 => 203229)
--- trunk/Source/_javascript_Core/tests/stress/JSON-parse-should-cache-array-lengths.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/JSON-parse-should-cache-array-lengths.js 2016-07-14 18:18:55 UTC (rev 203229)
@@ -0,0 +1,100 @@
+// This test should not hang, and should call the reviver function the expected number of times.
+
+function shouldBe(actualExpr, expectedExpr) {
+ function toString(x) {
+ return "" + x;
+ }
+
+ let actual = eval(actualExpr);
+ let expected = eval(expectedExpr);
+ if (typeof actual != typeof expected)
+ throw Error("expected type " + typeof expected + " actual type " + typeof actual);
+ if (toString(actual) != toString(expected))
+ throw Error("expected: " + expected + " actual: " + actual);
+}
+
+let result;
+let visited;
+
+function test(parseString, clearLength) {
+ visited = [];
+ var result = JSON.parse(parseString, function (key, value) {
+ visited.push('{' + key + ':' + value + '}');
+ if (clearLength)
+ this.length = 0;
+ return; // returning undefined deletes the value.
+ });
+ return result;
+}
+
+result = test('[10]', false);
+shouldBe("result", "undefined");
+shouldBe("visited", "['{0:10}','{:}']");
+shouldBe("visited.length", "2");
+
+result = test('[10]', true);
+shouldBe("result", "undefined");
+shouldBe("visited", "['{0:10}','{:}']");
+shouldBe("visited.length", "2");
+
+result = test('[ [ 10, 11 ], 12, [13, 14, 15], 16, 17]', false);
+shouldBe("result", "undefined");
+shouldBe("visited", "['{0:10}','{1:11}','{0:,}','{1:12}','{0:13}','{1:14}','{2:15}','{2:,,}','{3:16}','{4:17}','{:,,,,}']");
+shouldBe("visited.length", "11");
+
+result = test('[ [ 10, 11 ], 12, [13, 14, 15], 16, 17]', true);
+shouldBe("result", "undefined");
+shouldBe("visited", "['{0:10}','{1:undefined}','{0:}','{1:undefined}','{2:undefined}','{3:undefined}','{4:undefined}','{:}']");
+shouldBe("visited.length", "8");
+
+result = test('[ { "a": [ 10, 11 ], "b": 12 } , [ 13, { "c": 14 }, 15], 16, 17]', false);
+shouldBe("result", "undefined");
+shouldBe("visited", "['{0:10}','{1:11}','{a:,}','{b:12}','{0:[object Object]}','{0:13}','{c:14}','{1:[object Object]}','{2:15}','{1:,,}','{2:16}','{3:17}','{:,,,}']");
+shouldBe("visited.length", "13");
+
+result = test('[ { "a": [ 10, 11 ], "b": 12 } , [ 13, { "c": 14 }, 15], 16, 17]', true);
+shouldBe("result", "undefined");
+shouldBe("visited", "['{0:10}','{1:undefined}','{a:}','{b:12}','{0:[object Object]}','{1:undefined}','{2:undefined}','{3:undefined}','{:}']");
+shouldBe("visited.length", "9");
+
+
+function test2(parseString, clearLength) {
+ visited = [];
+ var result = JSON.parse(parseString, function (key, value) {
+ visited.push('{' + key + ':' + value + '}');
+ if (clearLength)
+ this.length = 0;
+ return (typeof value === "number") ? value * 2 : value;
+ });
+ return result;
+}
+
+result = test2('[10]', false);
+shouldBe("result", "[20]");
+shouldBe("visited", "['{0:10}','{:20}']");
+shouldBe("visited.length", "2");
+
+result = test2('[10]', true);
+shouldBe("result", "[20]");
+shouldBe("visited", "['{0:10}','{:20}']");
+shouldBe("visited.length", "2");
+
+result = test2('[ [ 10, 11 ], 12, [13, 14, 15], 16, 17]', false);
+shouldBe("result", "[20,22,24,26,28,30,32,34]");
+shouldBe("visited", "['{0:10}','{1:11}','{0:20,22}','{1:12}','{0:13}','{1:14}','{2:15}','{2:26,28,30}','{3:16}','{4:17}','{:20,22,24,26,28,30,32,34}']");
+shouldBe("visited.length", "11");
+
+result = test2('[ [ 10, 11 ], 12, [13, 14, 15], 16, 17]', true);
+shouldBe("result", "[]");
+shouldBe("visited", "['{0:10}','{1:undefined}','{0:}','{1:undefined}','{2:undefined}','{3:undefined}','{4:undefined}','{:}']");
+shouldBe("visited.length", "8");
+
+result = test2('[ { "a": [ 10, 11 ], "b": 12 } , [ 13, { "c": 14 }, 15], 16, 17]', false);
+shouldBe("result", "['[object Object]',26,'[object Object]',30,32,34]");
+shouldBe("visited", "['{0:10}','{1:11}','{a:20,22}','{b:12}','{0:[object Object]}','{0:13}','{c:14}','{1:[object Object]}','{2:15}','{1:26,[object Object],30}','{2:16}','{3:17}','{:[object Object],26,[object Object],30,32,34}']");
+shouldBe("visited.length", "13");
+
+result = test2('[ { "a": [ 10, 11 ], "b": 12 } , [ 13, { "c": 14 }, 15], 16, 17]', true);
+shouldBe("result", "[]");
+shouldBe("visited", "['{0:10}','{1:undefined}','{a:}','{b:12}','{0:[object Object]}','{1:undefined}','{2:undefined}','{3:undefined}','{:}']");
+shouldBe("visited.length", "9");
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
