Fix broken String.prototype.replace() and replaceAll().
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Nov 2019 02:16:57 +0000 (02:16 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Nov 2019 02:16:57 +0000 (02:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204479
<rdar://problem/57354854>

Reviewed by Ross Kirsling and Yusuke Suzuki.

JSTests:

* ChakraCore.yaml:
* ChakraCore/test/Strings/replace.baseline-jsc: Removed.
- We no longer need this because we've fixed the spec compliance issue in replace().
* stress/string-replaceAll-2.js: Added.

Source/JavaScriptCore:

String.prototype.replace() regressed due to r252683: <https://trac.webkit.org/r252683>
for webkit.org/b/202471.  The patch failed to handle InternalFunctions.

This patch also fixed a spec compliance bug for String.prototype.replace() i.e.
the replaceValue needs to be evaluated before we check if there's a match in the
source string.
Ref: 21.1.3.16-6 at https://www.ecma-international.org/ecma-262/10.0/#sec-string.prototype.replace

For String.prototype.replaceAll(), make sure it "behaves just like
String.prototype.replace if searchValue is a global regular expression".
Ref: https://github.com/tc39/proposal-string-replaceall

r252683 also made replaceUsingStringSearch() work the same way as
replaceUsingRegExpSearch().  I think this is the wrong trade off to make.
replaceUsingRegExpSearch() expects each search leg to do a RegExp search, which
is inherently expensive.  We shouldn't make string searches slower just because
the RegExp search does it that way.

However, at https://bugs.webkit.org/show_bug.cgi?id=202471#c22, Ross pointed out
that JetStream 2 results appeared to be neutral.  I think we should double check
with a micro-benchmark as well.  I'll leave this for a later patch.  For now, the
goal of this patch is simply to achieve correctness.
Ref: https://bugs.webkit.org/show_bug.cgi?id=204481

* runtime/StringPrototype.cpp:
(JSC::replaceUsingRegExpSearch):
(JSC::replaceUsingStringSearch):

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

JSTests/ChakraCore.yaml
JSTests/ChakraCore/test/Strings/replace.baseline-jsc [deleted file]
JSTests/ChangeLog
JSTests/stress/string-replaceAll-2.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/StringPrototype.cpp

index 71a1555..afe7623 100644 (file)
 - path: ChakraCore/test/Strings/compare.js
   cmd: runChakra :baseline, "NoException", "compare.baseline", []
 - path: ChakraCore/test/Strings/replace.js
-  cmd: runChakra :baseline, "NoException", "replace.baseline-jsc", []
+  cmd: runChakra :baseline, "NoException", "replace.baseline", []
 - path: ChakraCore/test/Strings/trim.js
   cmd: runChakra :baseline, "NoException", "trim.baseline", []
 - path: ChakraCore/test/Strings/lastindexof.js
diff --git a/JSTests/ChakraCore/test/Strings/replace.baseline-jsc b/JSTests/ChakraCore/test/Strings/replace.baseline-jsc
deleted file mode 100644 (file)
index fa65492..0000000
+++ /dev/null
@@ -1,199 +0,0 @@
-s1.replace("some", "any");: result = This is a any string value. 12.34
-s2.replace("some", "any");: result = This is a any string value. 12.34
-s1.replace("some", "");: result = This is a  string value. 12.34
-s2.replace("some", "");: result = This is a  string value. 12.34
-s1.replace("some", undefined);: result = This is a undefined string value. 12.34
-s2.replace("some", undefined);: result = This is a undefined string value. 12.34
-s1.replace("some", null);: result = This is a null string value. 12.34
-s2.replace("some", null);: result = This is a null string value. 12.34
-f0
-s1.replace("some", f0);: result = This is a f0 string value. 12.34
-f0
-s2.replace("some", f0);: result = This is a f0 string value. 12.34
-f1 x: some
-s1.replace("some", f1);: result = This is a f1 string value. 12.34
-f1 x: some
-s2.replace("some", f1);: result = This is a f1 string value. 12.34
-f2 x: some y: 10
-s1.replace("some", f2);: result = This is a f2 string value. 12.34
-f2 x: some y: 10
-s2.replace("some", f2);: result = This is a f2 string value. 12.34
-f3 x: some y: 10 z: This is a some string value. 12.34
- is a 
-s1.replace("some", f3);: result = This is a f3 string value. 12.34
-f3 x: some y: 10 z: This is a some string value. 12.34
- is a 
-s2.replace("some", f3);: result = This is a f3 string value. 12.34
-s1.replace(12, "any");: result = This is a some string value. any.34
-s2.replace(12, "any");: result = This is a some string value. any.34
-s1.replace(12, "");: result = This is a some string value. .34
-s2.replace(12, "");: result = This is a some string value. .34
-s1.replace(12, undefined);: result = This is a some string value. undefined.34
-s2.replace(12, undefined);: result = This is a some string value. undefined.34
-s1.replace(12, null);: result = This is a some string value. null.34
-s2.replace(12, null);: result = This is a some string value. null.34
-f0
-s1.replace(12, f0);: result = This is a some string value. f0.34
-f0
-s2.replace(12, f0);: result = This is a some string value. f0.34
-f1 x: 12
-s1.replace(12, f1);: result = This is a some string value. f1.34
-f1 x: 12
-s2.replace(12, f1);: result = This is a some string value. f1.34
-f2 x: 12 y: 29
-s1.replace(12, f2);: result = This is a some string value. f2.34
-f2 x: 12 y: 29
-s2.replace(12, f2);: result = This is a some string value. f2.34
-f3 x: 12 y: 29 z: This is a some string value. 12.34
-is is a some string value. 
-s1.replace(12, f3);: result = This is a some string value. f3.34
-f3 x: 12 y: 29 z: This is a some string value. 12.34
-is is a some string value. 
-s2.replace(12, f3);: result = This is a some string value. f3.34
-s1.replace(34, "any");: result = This is a some string value. 12.any
-s2.replace(34, "any");: result = This is a some string value. 12.any
-s1.replace(34, "");: result = This is a some string value. 12.
-s2.replace(34, "");: result = This is a some string value. 12.
-s1.replace(34, undefined);: result = This is a some string value. 12.undefined
-s2.replace(34, undefined);: result = This is a some string value. 12.undefined
-s1.replace(34, null);: result = This is a some string value. 12.null
-s2.replace(34, null);: result = This is a some string value. 12.null
-f0
-s1.replace(34, f0);: result = This is a some string value. 12.f0
-f0
-s2.replace(34, f0);: result = This is a some string value. 12.f0
-f1 x: 34
-s1.replace(34, f1);: result = This is a some string value. 12.f1
-f1 x: 34
-s2.replace(34, f1);: result = This is a some string value. 12.f1
-f2 x: 34 y: 32
-s1.replace(34, f2);: result = This is a some string value. 12.f2
-f2 x: 34 y: 32
-s2.replace(34, f2);: result = This is a some string value. 12.f2
-f3 x: 34 y: 32 z: This is a some string value. 12.34
-is is a some string value. 12.
-s1.replace(34, f3);: result = This is a some string value. 12.f3
-f3 x: 34 y: 32 z: This is a some string value. 12.34
-is is a some string value. 12.
-s2.replace(34, f3);: result = This is a some string value. 12.f3
-s1.replace(/[0-9]/, "any");: result = This is a some string value. any2.34
-s2.replace(/[0-9]/, "any");: result = This is a some string value. any2.34
-s1.replace(/[0-9]/, "");: result = This is a some string value. 2.34
-s2.replace(/[0-9]/, "");: result = This is a some string value. 2.34
-s1.replace(/[0-9]/, undefined);: result = This is a some string value. undefined2.34
-s2.replace(/[0-9]/, undefined);: result = This is a some string value. undefined2.34
-s1.replace(/[0-9]/, null);: result = This is a some string value. null2.34
-s2.replace(/[0-9]/, null);: result = This is a some string value. null2.34
-f0
-s1.replace(/[0-9]/, f0);: result = This is a some string value. f02.34
-f0
-s2.replace(/[0-9]/, f0);: result = This is a some string value. f02.34
-f1 x: 1
-s1.replace(/[0-9]/, f1);: result = This is a some string value. f12.34
-f1 x: 1
-s2.replace(/[0-9]/, f1);: result = This is a some string value. f12.34
-f2 x: 1 y: 29
-s1.replace(/[0-9]/, f2);: result = This is a some string value. f22.34
-f2 x: 1 y: 29
-s2.replace(/[0-9]/, f2);: result = This is a some string value. f22.34
-f3 x: 1 y: 29 z: This is a some string value. 12.34
-his is a some string value. 
-s1.replace(/[0-9]/, f3);: result = This is a some string value. f32.34
-f3 x: 1 y: 29 z: This is a some string value. 12.34
-his is a some string value. 
-s2.replace(/[0-9]/, f3);: result = This is a some string value. f32.34
-s1.replace(/[0-9]+/, "any");: result = This is a some string value. any.34
-s2.replace(/[0-9]+/, "any");: result = This is a some string value. any.34
-s1.replace(/[0-9]+/, "");: result = This is a some string value. .34
-s2.replace(/[0-9]+/, "");: result = This is a some string value. .34
-s1.replace(/[0-9]+/, undefined);: result = This is a some string value. undefined.34
-s2.replace(/[0-9]+/, undefined);: result = This is a some string value. undefined.34
-s1.replace(/[0-9]+/, null);: result = This is a some string value. null.34
-s2.replace(/[0-9]+/, null);: result = This is a some string value. null.34
-f0
-s1.replace(/[0-9]+/, f0);: result = This is a some string value. f0.34
-f0
-s2.replace(/[0-9]+/, f0);: result = This is a some string value. f0.34
-f1 x: 12
-s1.replace(/[0-9]+/, f1);: result = This is a some string value. f1.34
-f1 x: 12
-s2.replace(/[0-9]+/, f1);: result = This is a some string value. f1.34
-f2 x: 12 y: 29
-s1.replace(/[0-9]+/, f2);: result = This is a some string value. f2.34
-f2 x: 12 y: 29
-s2.replace(/[0-9]+/, f2);: result = This is a some string value. f2.34
-f3 x: 12 y: 29 z: This is a some string value. 12.34
-is is a some string value. 
-s1.replace(/[0-9]+/, f3);: result = This is a some string value. f3.34
-f3 x: 12 y: 29 z: This is a some string value. 12.34
-is is a some string value. 
-s2.replace(/[0-9]+/, f3);: result = This is a some string value. f3.34
-s1.replace(/[0-9]+/g, "any");: result = This is a some string value. any.any
-s2.replace(/[0-9]+/g, "any");: result = This is a some string value. any.any
-s1.replace(/[0-9]+/g, "");: result = This is a some string value. .
-s2.replace(/[0-9]+/g, "");: result = This is a some string value. .
-s1.replace(/[0-9]+/g, undefined);: result = This is a some string value. undefined.undefined
-s2.replace(/[0-9]+/g, undefined);: result = This is a some string value. undefined.undefined
-s1.replace(/[0-9]+/g, null);: result = This is a some string value. null.null
-s2.replace(/[0-9]+/g, null);: result = This is a some string value. null.null
-f0
-f0
-s1.replace(/[0-9]+/g, f0);: result = This is a some string value. f0.f0
-f0
-f0
-s2.replace(/[0-9]+/g, f0);: result = This is a some string value. f0.f0
-f1 x: 12
-f1 x: 34
-s1.replace(/[0-9]+/g, f1);: result = This is a some string value. f1.f1
-f1 x: 12
-f1 x: 34
-s2.replace(/[0-9]+/g, f1);: result = This is a some string value. f1.f1
-f2 x: 12 y: 29
-f2 x: 34 y: 32
-s1.replace(/[0-9]+/g, f2);: result = This is a some string value. f2.f2
-f2 x: 12 y: 29
-f2 x: 34 y: 32
-s2.replace(/[0-9]+/g, f2);: result = This is a some string value. f2.f2
-f3 x: 12 y: 29 z: This is a some string value. 12.34
-is is a some string value. 
-f3 x: 34 y: 32 z: This is a some string value. 12.34
-is is a some string value. 12.
-s1.replace(/[0-9]+/g, f3);: result = This is a some string value. f3.f3
-f3 x: 12 y: 29 z: This is a some string value. 12.34
-is is a some string value. 
-f3 x: 34 y: 32 z: This is a some string value. 12.34
-is is a some string value. 12.
-s2.replace(/[0-9]+/g, f3);: result = This is a some string value. f3.f3
-s1.replace(undefined, "any");: result = This is a some string value. 12.34
-s2.replace(undefined, "any");: result = This is a some string value. 12.34
-s1.replace(undefined, "");: result = This is a some string value. 12.34
-s2.replace(undefined, "");: result = This is a some string value. 12.34
-s1.replace(undefined, undefined);: result = This is a some string value. 12.34
-s2.replace(undefined, undefined);: result = This is a some string value. 12.34
-s1.replace(undefined, null);: result = This is a some string value. 12.34
-s2.replace(undefined, null);: result = This is a some string value. 12.34
-s1.replace(undefined, f0);: result = This is a some string value. 12.34
-s2.replace(undefined, f0);: result = This is a some string value. 12.34
-s1.replace(undefined, f1);: result = This is a some string value. 12.34
-s2.replace(undefined, f1);: result = This is a some string value. 12.34
-s1.replace(undefined, f2);: result = This is a some string value. 12.34
-s2.replace(undefined, f2);: result = This is a some string value. 12.34
-s1.replace(undefined, f3);: result = This is a some string value. 12.34
-s2.replace(undefined, f3);: result = This is a some string value. 12.34
-s1.replace(null, "any");: result = This is a some string value. 12.34
-s2.replace(null, "any");: result = This is a some string value. 12.34
-s1.replace(null, "");: result = This is a some string value. 12.34
-s2.replace(null, "");: result = This is a some string value. 12.34
-s1.replace(null, undefined);: result = This is a some string value. 12.34
-s2.replace(null, undefined);: result = This is a some string value. 12.34
-s1.replace(null, null);: result = This is a some string value. 12.34
-s2.replace(null, null);: result = This is a some string value. 12.34
-s1.replace(null, f0);: result = This is a some string value. 12.34
-s2.replace(null, f0);: result = This is a some string value. 12.34
-s1.replace(null, f1);: result = This is a some string value. 12.34
-s2.replace(null, f1);: result = This is a some string value. 12.34
-s1.replace(null, f2);: result = This is a some string value. 12.34
-s2.replace(null, f2);: result = This is a some string value. 12.34
-s1.replace(null, f3);: result = This is a some string value. 12.34
-s2.replace(null, f3);: result = This is a some string value. 12.34
-false
index 26dabc9..131e015 100644 (file)
@@ -1,3 +1,16 @@
+2019-11-21  Mark Lam  <mark.lam@apple.com>
+
+        Fix broken String.prototype.replace() and replaceAll().
+        https://bugs.webkit.org/show_bug.cgi?id=204479
+        <rdar://problem/57354854>
+
+        Reviewed by Ross Kirsling and Yusuke Suzuki.
+
+        * ChakraCore.yaml:
+        * ChakraCore/test/Strings/replace.baseline-jsc: Removed.
+        - We no longer need this because we've fixed the spec compliance issue in replace().
+        * stress/string-replaceAll-2.js: Added.
+
 2019-11-21  Yusuke Suzuki  <ysuzuki@apple.com>
 
         Unreviewed, rolling in again, regression is not caused by it
diff --git a/JSTests/stress/string-replaceAll-2.js b/JSTests/stress/string-replaceAll-2.js
new file mode 100644 (file)
index 0000000..b07e7f3
--- /dev/null
@@ -0,0 +1,293 @@
+const verbose = false;
+const supportsReplaceAll = (String.prototype.replaceAll != undefined);
+
+var visits;
+var failures = 0;
+
+if (console)
+    print = console.log;
+
+function verify(desc, testName, result, visits, expected) {
+    var failed = (result != expected.result);
+    if (failed || verbose) print(desc, " ", testName, ": result expected:", expected.result, " actual:", result);
+    if (failed) failures++;
+
+    var failed = (visits != expected.visits);
+    if (failed || verbose) print(desc, " ", testName, ": visits expected:", expected.visits, " actual:", visits);
+    if (failed) failures++;
+}
+
+function handleUnexpectedException(e) {
+    print(e);
+    failures++;
+}
+
+function test(replaceValueDesc, replaceValue, expected) {
+    var testCase = 0;
+    var result;
+
+    visits = 0;
+    try {
+        result = '12324'.replace('2', replaceValue);
+        verify(replaceValueDesc, "'12324'.replace('2', ...)", result, visits, expected[testCase]);
+    } catch (e) { handleUnexpectedException(e) }
+
+    visits = 0;
+    testCase++;
+    try {
+        result = '12324'.replace(/2/g, replaceValue);
+        verify(replaceValueDesc, "'12324'.replace(/2/g, ...)", result, visits, expected[testCase]);
+    } catch (e) { handleUnexpectedException(e) }
+
+    visits = 0;
+    testCase++;
+    try {
+        if (supportsReplaceAll) {
+            result = '12324'.replaceAll('2', replaceValue);
+            verify(replaceValueDesc, "'12324'.replaceAll('2', ...)", result, visits, expected[testCase]);
+        }
+    } catch (e) { handleUnexpectedException(e) }
+
+    visits = 0;
+    testCase++;
+    try {
+        result = '12324'.replace('', replaceValue);
+        verify(replaceValueDesc, "'12324'.replace('', ...)", result, visits, expected[testCase]);
+    } catch (e) { handleUnexpectedException(e) }
+
+    visits = 0;
+    testCase++;
+    try {
+        result = '12324'.replace(/(?:)/g, replaceValue);
+        verify(replaceValueDesc, "'12324'.replace(/(?:)/g, ...)", result, visits, expected[testCase]);
+    } catch (e) { handleUnexpectedException(e) }
+
+    visits = 0;
+    testCase++;
+    try {
+        if (supportsReplaceAll) {
+            result = '12324'.replaceAll('', replaceValue);
+            verify(replaceValueDesc, "'12324'.replaceAll('', ...)", result, visits, expected[testCase]);
+        }
+    } catch (e) { handleUnexpectedException(e) }
+
+    visits = 0;
+    testCase++;
+    try {
+        result = '12324'.replace('0', replaceValue);
+        verify(replaceValueDesc, "'12324'.replace('0', ...)", result, visits, expected[testCase]);
+    } catch (e) { handleUnexpectedException(e) }
+
+    visits = 0;
+    testCase++;
+    try {
+        result = '12324'.replace(/0/g, replaceValue);
+        verify(replaceValueDesc, "'12324'.replace(/0/g, ...)", result, visits, expected[testCase]);
+    } catch (e) { handleUnexpectedException(e) }
+
+    visits = 0;
+    testCase++;
+    try {
+        if (supportsReplaceAll) {
+            result = '12324'.replaceAll('0', replaceValue);
+            verify(replaceValueDesc, "'12324'.replaceAll('0', ...)", result, visits, expected[testCase]);
+        }
+    } catch (e) { handleUnexpectedException(e) }
+}
+
+function foo() {
+    visits++;
+    return 5;
+}
+
+test("function foo", foo, [
+    { result: '15324', visits: 1 }, // replace '2'
+    { result: '15354', visits: 2 }, // replace /2/g
+    { result: '15354', visits: 2 }, // replaceAll '2'
+    { result: '512324', visits: 1 }, // replace ''
+    { result: '51525352545', visits: 6 }, // replace /(?:)/g
+    { result: '51525352545', visits: 6 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+var proxy = new Proxy(foo, {
+    apply() {
+        visits++;
+        return 9;
+    }
+});
+
+test("proxy", proxy, [
+    { result: '19324', visits: 1 }, // replace '2'
+    { result: '19394', visits: 2 }, // replace /2/g
+    { result: '19394', visits: 2 }, // replaceAll '2'
+    { result: '912324', visits: 1 }, // replace ''
+    { result: '91929392949', visits: 6 }, // replace /(?:)/g
+    { result: '91929392949', visits: 6 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("Object", Object, [
+    { result: '12324', visits: 0 }, // replace '2'
+    { result: '12324', visits: 0 }, // replace /2/g
+    { result: '12324', visits: 0 }, // replaceAll '2'
+    { result: '12324', visits: 0 }, // replace ''
+    { result: '12324', visits: 0 }, // replace /(?:)/g
+    { result: '12324', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+var o = {
+    toString() {
+        visits++;
+        return "o";
+    }
+}
+
+test("o with toString", o, [
+    { result: '1o324', visits: 1 }, // replace '2'
+    { result: '1o3o4', visits: 1 }, // replace /2/g
+    { result: '1o3o4', visits: 1 }, // replaceAll '2'
+    { result: 'o12324', visits: 1 }, // replace ''
+    { result: 'o1o2o3o2o4o', visits: 1 }, // replace /(?:)/g
+    { result: 'o1o2o3o2o4o', visits: 1 }, // replaceAll ''
+    { result: '12324', visits: 1 }, // replace '0'
+    { result: '12324', visits: 1 }, // replace /0/g
+    { result: '12324', visits: 1}, // replaceAll '0'
+]);
+
+function bar() {
+    visits++;
+    return o;
+}
+
+test("function bar", bar, [
+    { result: '1o324', visits: 2 }, // replace '2'
+    { result: '1o3o4', visits: 4 }, // replace /2/g
+    { result: '1o3o4', visits: 4 }, // replaceAll '2'
+    { result: 'o12324', visits: 2 }, // replace ''
+    { result: 'o1o2o3o2o4o', visits: 12 }, // replace /(?:)/g
+    { result: 'o1o2o3o2o4o', visits: 12 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("undefined", undefined, [
+    { result: '1undefined324', visits: 0 }, // replace '2'
+    { result: '1undefined3undefined4', visits: 0 }, // replace /2/g
+    { result: '1undefined3undefined4', visits: 0 }, // replaceAll '2'
+    { result: 'undefined12324', visits: 0 }, // replace ''
+    { result: 'undefined1undefined2undefined3undefined2undefined4undefined', visits: 0 }, // replace /(?:)/g
+    { result: 'undefined1undefined2undefined3undefined2undefined4undefined', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("null", null, [
+    { result: '1null324', visits: 0 }, // replace '2'
+    { result: '1null3null4', visits: 0 }, // replace /2/g
+    { result: '1null3null4', visits: 0 }, // replaceAll '2'
+    { result: 'null12324', visits: 0 }, // replace ''
+    { result: 'null1null2null3null2null4null', visits: 0 }, // replace /(?:)/g
+    { result: 'null1null2null3null2null4null', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("0/0", 0/0, [
+    { result: '1NaN324', visits: 0 }, // replace '2'
+    { result: '1NaN3NaN4', visits: 0 }, // replace /2/g
+    { result: '1NaN3NaN4', visits: 0 }, // replaceAll '2'
+    { result: 'NaN12324', visits: 0 }, // replace ''
+    { result: 'NaN1NaN2NaN3NaN2NaN4NaN', visits: 0 }, // replace /(?:)/g
+    { result: 'NaN1NaN2NaN3NaN2NaN4NaN', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("-0/0", -0/0, [
+    { result: '1NaN324', visits: 0 }, // replace '2'
+    { result: '1NaN3NaN4', visits: 0 }, // replace /2/g
+    { result: '1NaN3NaN4', visits: 0 }, // replaceAll '2'
+    { result: 'NaN12324', visits: 0 }, // replace ''
+    { result: 'NaN1NaN2NaN3NaN2NaN4NaN', visits: 0 }, // replace /(?:)/g
+    { result: 'NaN1NaN2NaN3NaN2NaN4NaN', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("1/0", 1/0, [
+    { result: '1Infinity324', visits: 0 }, // replace '2'
+    { result: '1Infinity3Infinity4', visits: 0 }, // replace /2/g
+    { result: '1Infinity3Infinity4', visits: 0 }, // replaceAll '2'
+    { result: 'Infinity12324', visits: 0 }, // replace ''
+    { result: 'Infinity1Infinity2Infinity3Infinity2Infinity4Infinity', visits: 0 }, // replace /(?:)/g
+    { result: 'Infinity1Infinity2Infinity3Infinity2Infinity4Infinity', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("-1/0", -1/0, [
+    { result: '1-Infinity324', visits: 0 }, // replace '2'
+    { result: '1-Infinity3-Infinity4', visits: 0 }, // replace /2/g
+    { result: '1-Infinity3-Infinity4', visits: 0 }, // replaceAll '2'
+    { result: '-Infinity12324', visits: 0 }, // replace ''
+    { result: '-Infinity1-Infinity2-Infinity3-Infinity2-Infinity4-Infinity', visits: 0 }, // replace /(?:)/g
+    { result: '-Infinity1-Infinity2-Infinity3-Infinity2-Infinity4-Infinity', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("42", 42, [
+    { result: '142324', visits: 0 }, // replace '2'
+    { result: '1423424', visits: 0 }, // replace /2/g
+    { result: '1423424', visits: 0 }, // replaceAll '2'
+    { result: '4212324', visits: 0 }, // replace ''
+    { result: '42142242342242442', visits: 0 }, // replace /(?:)/g
+    { result: '42142242342242442', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("4.2", 4.2, [
+    { result: '14.2324', visits: 0 }, // replace '2'
+    { result: '14.234.24', visits: 0 }, // replace /2/g
+    { result: '14.234.24', visits: 0 }, // replaceAll '2'
+    { result: '4.212324', visits: 0 }, // replace ''
+    { result: '4.214.224.234.224.244.2', visits: 0 }, // replace /(?:)/g
+    { result: '4.214.224.234.224.244.2', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+test("'s'", 's', [
+    { result: '1s324', visits: 0 }, // replace '2'
+    { result: '1s3s4', visits: 0 }, // replace /2/g
+    { result: '1s3s4', visits: 0 }, // replaceAll '2'
+    { result: 's12324', visits: 0 }, // replace ''
+    { result: 's1s2s3s2s4s', visits: 0 }, // replace /(?:)/g
+    { result: 's1s2s3s2s4s', visits: 0 }, // replaceAll ''
+    { result: '12324', visits: 0 }, // replace '0'
+    { result: '12324', visits: 0 }, // replace /0/g
+    { result: '12324', visits: 0 }, // replaceAll '0'
+]);
+
+if (failures) {
+    print("Found", failures, "failures");
+    throw "FAILED";
+}
index 7350b65..fae231e 100644 (file)
@@ -1,3 +1,39 @@
+2019-11-21  Mark Lam  <mark.lam@apple.com>
+
+        Fix broken String.prototype.replace() and replaceAll().
+        https://bugs.webkit.org/show_bug.cgi?id=204479
+        <rdar://problem/57354854>
+
+        Reviewed by Ross Kirsling and Yusuke Suzuki.
+
+        String.prototype.replace() regressed due to r252683: <https://trac.webkit.org/r252683>
+        for webkit.org/b/202471.  The patch failed to handle InternalFunctions.
+
+        This patch also fixed a spec compliance bug for String.prototype.replace() i.e.
+        the replaceValue needs to be evaluated before we check if there's a match in the
+        source string.
+        Ref: 21.1.3.16-6 at https://www.ecma-international.org/ecma-262/10.0/#sec-string.prototype.replace
+
+        For String.prototype.replaceAll(), make sure it "behaves just like
+        String.prototype.replace if searchValue is a global regular expression".
+        Ref: https://github.com/tc39/proposal-string-replaceall
+
+        r252683 also made replaceUsingStringSearch() work the same way as
+        replaceUsingRegExpSearch().  I think this is the wrong trade off to make.
+        replaceUsingRegExpSearch() expects each search leg to do a RegExp search, which
+        is inherently expensive.  We shouldn't make string searches slower just because
+        the RegExp search does it that way.
+
+        However, at https://bugs.webkit.org/show_bug.cgi?id=202471#c22, Ross pointed out
+        that JetStream 2 results appeared to be neutral.  I think we should double check
+        with a micro-benchmark as well.  I'll leave this for a later patch.  For now, the
+        goal of this patch is simply to achieve correctness.
+        Ref: https://bugs.webkit.org/show_bug.cgi?id=204481
+
+        * runtime/StringPrototype.cpp:
+        (JSC::replaceUsingRegExpSearch):
+        (JSC::replaceUsingStringSearch):
+
 2019-11-21  Per Arne Vollan  <pvollan@apple.com>
 
         Fix Win64 compile errors
index 5c6d6eb..c760181 100644 (file)
@@ -664,7 +664,6 @@ static ALWAYS_INLINE JSString* replaceUsingRegExpSearch(
                         if (!groupName.isEmpty())
                             groups->putDirect(vm, Identifier::fromString(vm, groupName), patternValue);
                     }
-
                 }
 
                 args.append(jsNumber(result.start));
@@ -787,10 +786,6 @@ static ALWAYS_INLINE JSString* replaceUsingStringSearch(VM& vm, JSGlobalObject*
     String searchString = searchValue.toWTFString(globalObject);
     RETURN_IF_EXCEPTION(scope, nullptr);
 
-    size_t matchStart = string.find(searchString);
-    if (matchStart == notFound)
-        return jsString;
-
     CallData callData;
     CallType callType = getCallData(vm, replaceValue, callData);
     Optional<CachedCall> cachedCall;
@@ -799,24 +794,43 @@ static ALWAYS_INLINE JSString* replaceUsingStringSearch(VM& vm, JSGlobalObject*
         replaceString = replaceValue.toWTFString(globalObject);
         RETURN_IF_EXCEPTION(scope, nullptr);
     } else {
-        cachedCall.emplace(globalObject, callFrame, jsCast<JSFunction*>(replaceValue), 3);
-        cachedCall->setThis(jsUndefined());
+        JSFunction* function = jsDynamicCast<JSFunction*>(vm, replaceValue);
+        if (function) {
+            cachedCall.emplace(globalObject, callFrame, function, 3);
+            cachedCall->setThis(jsUndefined());
+        }
     }
 
+    size_t matchStart = string.find(searchString);
+    if (matchStart == notFound)
+        return jsString;
+
     size_t endOfLastMatch = 0;
     size_t searchStringLength = searchString.length();
     Vector<StringRange, 16> sourceRanges;
     Vector<String, 16> replacements;
     do {
-        if (cachedCall) {
-            auto* substring = jsSubstring(vm, string, matchStart, searchStringLength);
-            RETURN_IF_EXCEPTION(scope, nullptr);
-            cachedCall->clearArguments();
-            cachedCall->appendArgument(substring);
-            cachedCall->appendArgument(jsNumber(matchStart));
-            cachedCall->appendArgument(jsString);
-            ASSERT(!cachedCall->hasOverflowedArguments());
-            JSValue replacement = cachedCall->call();
+        if (callType != CallType::None) {
+            JSValue replacement;
+            if (cachedCall) {
+                auto* substring = jsSubstring(vm, string, matchStart, searchStringLength);
+                RETURN_IF_EXCEPTION(scope, nullptr);
+                cachedCall->clearArguments();
+                cachedCall->appendArgument(substring);
+                cachedCall->appendArgument(jsNumber(matchStart));
+                cachedCall->appendArgument(jsString);
+                ASSERT(!cachedCall->hasOverflowedArguments());
+                replacement = cachedCall->call();
+            } else {
+                MarkedArgumentBuffer args;
+                auto* substring = jsSubstring(vm, string, matchStart, searchString.impl()->length());
+                RETURN_IF_EXCEPTION(scope, nullptr);
+                args.append(substring);
+                args.append(jsNumber(matchStart));
+                args.append(jsString);
+                ASSERT(!args.hasOverflowed());
+                replacement = call(globalObject, replaceValue, callType, callData, jsUndefined(), args);
+            }
             RETURN_IF_EXCEPTION(scope, nullptr);
             replaceString = replacement.toWTFString(globalObject);
             RETURN_IF_EXCEPTION(scope, nullptr);
@@ -826,7 +840,7 @@ static ALWAYS_INLINE JSString* replaceUsingStringSearch(VM& vm, JSGlobalObject*
             OUT_OF_MEMORY(globalObject, scope);
 
         size_t matchEnd = matchStart + searchStringLength;
-        if (cachedCall)
+        if (callType != CallType::None)
             replacements.append(replaceString);
         else {
             StringBuilder replacement(StringBuilder::OverflowHandler::RecordOverflow);