- Revision
- 198592
- Author
- [email protected]
- Date
- 2016-03-23 14:03:02 -0700 (Wed, 23 Mar 2016)
Log Message
_javascript_Core ArrayPrototype::join shouldn't cache butterfly when it makes effectful calls
https://bugs.webkit.org/show_bug.cgi?id=155776
Reviewed by Saam Barati.
Source/_javascript_Core:
Array.join ends up calling toString, possibly on some object. Since these calls
could be effectful and could change the array itself, we can't hold the butterfly
pointer while making effectful calls. Changed the code to fall back to the general
case when an effectful toString() call might be made.
* runtime/ArrayPrototype.cpp:
(JSC::join):
* runtime/JSStringJoiner.h:
(JSC::JSStringJoiner::appendWithoutSideEffects): New helper that doesn't make effectful
toString() calls.
(JSC::JSStringJoiner::append): Built upon appendWithoutSideEffects.
LayoutTests:
New test.
* js/regress-155776-expected.txt: Added.
* js/regress-155776.html: Added.
* js/script-tests/regress-155776.js: Added.
(fillBigArrayViaToString):
(Function.prototype.toString):
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (198591 => 198592)
--- trunk/LayoutTests/ChangeLog 2016-03-23 20:58:40 UTC (rev 198591)
+++ trunk/LayoutTests/ChangeLog 2016-03-23 21:03:02 UTC (rev 198592)
@@ -1,3 +1,18 @@
+2016-03-23 Michael Saboff <[email protected]>
+
+ _javascript_Core ArrayPrototype::join shouldn't cache butterfly when it makes effectful calls
+ https://bugs.webkit.org/show_bug.cgi?id=155776
+
+ Reviewed by Saam Barati.
+
+ New test.
+
+ * js/regress-155776-expected.txt: Added.
+ * js/regress-155776.html: Added.
+ * js/script-tests/regress-155776.js: Added.
+ (fillBigArrayViaToString):
+ (Function.prototype.toString):
+
2016-03-23 Daniel Bates <[email protected]>
CSP: Make violation console messages concise and consistent
Added: trunk/LayoutTests/js/regress-155776-expected.txt (0 => 198592)
--- trunk/LayoutTests/js/regress-155776-expected.txt (rev 0)
+++ trunk/LayoutTests/js/regress-155776-expected.txt 2016-03-23 21:03:02 UTC (rev 198592)
@@ -0,0 +1,10 @@
+Regresion test for 155776. This test should pass and not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS stringResult is expectedString
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/js/regress-155776.html (0 => 198592)
--- trunk/LayoutTests/js/regress-155776.html (rev 0)
+++ trunk/LayoutTests/js/regress-155776.html 2016-03-23 21:03:02 UTC (rev 198592)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Added: trunk/LayoutTests/js/script-tests/regress-155776.js (0 => 198592)
--- trunk/LayoutTests/js/script-tests/regress-155776.js (rev 0)
+++ trunk/LayoutTests/js/script-tests/regress-155776.js 2016-03-23 21:03:02 UTC (rev 198592)
@@ -0,0 +1,50 @@
+description("Regresion test for 155776. This test should pass and not crash.");
+
+var bigArray = [];
+var bigNum = 123456789;
+var smallNum = 123;
+var toStringCount = 0;
+
+function fillBigArrayViaToString(n) {
+ var results = [];
+
+ for (var i = 0; i < n; i++)
+ fillBigArrayViaToString.toString();
+
+ return results;
+}
+
+Function.prototype.toString = function(x) {
+ toStringCount++;
+ bigArray.push(smallNum);
+
+ if (toStringCount == 2000) {
+ var newArray = new Uint32Array(8000);
+ for (var i = 0; i < newArray.length; i++)
+ newArray[i] = 0x10000000;
+ }
+
+ bigArray.push(fillBigArrayViaToString);
+ bigArray.push(fillBigArrayViaToString);
+ bigArray.push(fillBigArrayViaToString);
+ return bigNum;
+};
+
+fillBigArrayViaToString(4000).join();
+
+bigArray.length = 4000;
+
+var stringResult = bigArray.join(":");
+
+var expectedArray = [];
+
+for (var i = 0; i < 1000; i++) {
+ expectedArray.push(smallNum);
+ expectedArray.push(bigNum);
+ expectedArray.push(bigNum);
+ expectedArray.push(bigNum);
+}
+
+var expectedString = expectedArray.join(":");
+
+shouldBe('stringResult', 'expectedString');
Modified: trunk/Source/_javascript_Core/ChangeLog (198591 => 198592)
--- trunk/Source/_javascript_Core/ChangeLog 2016-03-23 20:58:40 UTC (rev 198591)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-03-23 21:03:02 UTC (rev 198592)
@@ -1,3 +1,22 @@
+2016-03-23 Michael Saboff <[email protected]>
+
+ _javascript_Core ArrayPrototype::join shouldn't cache butterfly when it makes effectful calls
+ https://bugs.webkit.org/show_bug.cgi?id=155776
+
+ Reviewed by Saam Barati.
+
+ Array.join ends up calling toString, possibly on some object. Since these calls
+ could be effectful and could change the array itself, we can't hold the butterfly
+ pointer while making effectful calls. Changed the code to fall back to the general
+ case when an effectful toString() call might be made.
+
+ * runtime/ArrayPrototype.cpp:
+ (JSC::join):
+ * runtime/JSStringJoiner.h:
+ (JSC::JSStringJoiner::appendWithoutSideEffects): New helper that doesn't make effectful
+ toString() calls.
+ (JSC::JSStringJoiner::append): Built upon appendWithoutSideEffects.
+
2016-03-23 Keith Miller <[email protected]>
Array.prototype native functions' species constructors should work with proxies
Modified: trunk/Source/_javascript_Core/runtime/ArrayPrototype.cpp (198591 => 198592)
--- trunk/Source/_javascript_Core/runtime/ArrayPrototype.cpp 2016-03-23 20:58:40 UTC (rev 198591)
+++ trunk/Source/_javascript_Core/runtime/ArrayPrototype.cpp 2016-03-23 21:03:02 UTC (rev 198592)
@@ -495,9 +495,8 @@
bool holesKnownToBeOK = false;
for (unsigned i = 0; i < length; ++i) {
if (JSValue value = data[i].get()) {
- joiner.append(state, value);
- if (state.hadException())
- return jsUndefined();
+ if (!joiner.appendWithoutSideEffects(state, value))
+ goto generalCase;
} else {
if (!holesKnownToBeOK) {
if (holesMustForwardToPrototype(state, thisObject))
@@ -545,9 +544,8 @@
auto data = ""
for (unsigned i = 0; i < length; ++i) {
if (JSValue value = data[i].get()) {
- joiner.append(state, value);
- if (state.hadException())
- return jsUndefined();
+ if (!joiner.appendWithoutSideEffects(state, value))
+ goto generalCase;
} else
joiner.appendEmptyString();
}
Modified: trunk/Source/_javascript_Core/runtime/JSStringJoiner.h (198591 => 198592)
--- trunk/Source/_javascript_Core/runtime/JSStringJoiner.h 2016-03-23 20:58:40 UTC (rev 198591)
+++ trunk/Source/_javascript_Core/runtime/JSStringJoiner.h 2016-03-23 21:03:02 UTC (rev 198592)
@@ -38,6 +38,7 @@
~JSStringJoiner();
void append(ExecState&, JSValue);
+ bool appendWithoutSideEffects(ExecState&, JSValue);
void appendEmptyString();
JSValue join(ExecState&);
@@ -97,7 +98,7 @@
m_strings.uncheckedAppend({ { }, { } });
}
-ALWAYS_INLINE void JSStringJoiner::append(ExecState& state, JSValue value)
+ALWAYS_INLINE bool JSStringJoiner::appendWithoutSideEffects(ExecState& state, JSValue value)
{
// The following code differs from using the result of JSValue::toString in the following ways:
// 1) It's inlined more than JSValue::toString is.
@@ -105,37 +106,46 @@
// 3) It doesn't create a JSString for numbers, true, or false.
// 4) It turns undefined and null into the empty string instead of "undefined" and "null".
// 5) It uses optimized code paths for all the cases known to be 8-bit and for the empty string.
+ // If we might make an effectful calls, return false. Otherwise return true.
if (value.isCell()) {
JSString* jsString;
- if (value.asCell()->isString())
- jsString = asString(value);
- else
- jsString = value.toString(&state);
+ if (!value.asCell()->isString())
+ return false;
+ jsString = asString(value);
append(jsString->viewWithUnderlyingString(state));
- return;
+ return true;
}
if (value.isInt32()) {
append8Bit(state.vm().numericStrings.add(value.asInt32()));
- return;
+ return true;
}
if (value.isDouble()) {
append8Bit(state.vm().numericStrings.add(value.asDouble()));
- return;
+ return true;
}
if (value.isTrue()) {
append8Bit(state.vm().propertyNames->trueKeyword.string());
- return;
+ return true;
}
if (value.isFalse()) {
append8Bit(state.vm().propertyNames->falseKeyword.string());
- return;
+ return true;
}
ASSERT(value.isUndefinedOrNull());
appendEmptyString();
+ return true;
}
+ALWAYS_INLINE void JSStringJoiner::append(ExecState& state, JSValue value)
+{
+ if (!appendWithoutSideEffects(state, value)) {
+ JSString* jsString = value.toString(&state);
+ append(jsString->viewWithUnderlyingString(state));
+ }
}
+}
+
#endif
