JavaScriptCore ArrayPrototype::join shouldn't cache butterfly when it makes effectful...
authormsaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 Mar 2016 21:03:02 +0000 (21:03 +0000)
committermsaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 Mar 2016 21:03:02 +0000 (21:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=155776

Reviewed by Saam Barati.

Source/JavaScriptCore:

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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@198592 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/js/regress-155776-expected.txt [new file with mode: 0644]
LayoutTests/js/regress-155776.html [new file with mode: 0644]
LayoutTests/js/script-tests/regress-155776.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
Source/JavaScriptCore/runtime/JSStringJoiner.h

index 312c9cf..7406b5e 100644 (file)
@@ -1,3 +1,18 @@
+2016-03-23  Michael Saboff  <msaboff@apple.com>
+
+        JavaScriptCore 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  <dabates@apple.com>
 
         CSP: Make violation console messages concise and consistent
diff --git a/LayoutTests/js/regress-155776-expected.txt b/LayoutTests/js/regress-155776-expected.txt
new file mode 100644 (file)
index 0000000..a5c59e7
--- /dev/null
@@ -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
+
diff --git a/LayoutTests/js/regress-155776.html b/LayoutTests/js/regress-155776.html
new file mode 100644 (file)
index 0000000..d585d89
--- /dev/null
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="script-tests/regress-155776.js"></script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/js/script-tests/regress-155776.js b/LayoutTests/js/script-tests/regress-155776.js
new file mode 100644 (file)
index 0000000..a323d95
--- /dev/null
@@ -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');
index 69fcda0..fadea30 100644 (file)
@@ -1,3 +1,22 @@
+2016-03-23  Michael Saboff  <msaboff@apple.com>
+
+        JavaScriptCore 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  <keith_miller@apple.com>
 
         Array.prototype native functions' species constructors should work with proxies
index 6113247..cfdd7fc 100644 (file)
@@ -495,9 +495,8 @@ static inline JSValue join(ExecState& state, JSObject* thisObject, StringView se
         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 @@ static inline JSValue join(ExecState& state, JSObject* thisObject, StringView se
         auto data = storage.vector().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();
         }
index 6561418..f9e8bbd 100644 (file)
@@ -38,6 +38,7 @@ public:
     ~JSStringJoiner();
 
     void append(ExecState&, JSValue);
+    bool appendWithoutSideEffects(ExecState&, JSValue);
     void appendEmptyString();
 
     JSValue join(ExecState&);
@@ -97,7 +98,7 @@ ALWAYS_INLINE void JSStringJoiner::appendEmptyString()
     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,35 +106,44 @@ ALWAYS_INLINE void JSStringJoiner::append(ExecState& state, JSValue value)
     // 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));
+    }
 }
 
 }