Bug 204479 – Fix broken String.prototype.replace() and replaceAll().

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.  See 21.1.3.14-9 at https://www.ecma-international.org/ecma-262/6.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".  See https://github.com/tc39/proposal-string-replaceall.

<rdar://problem/57354854>

[Mark Lam]

Created attachment 384104 [details]
proposed patch.

[Mark Lam]

Comment on attachment 384104 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=384104&action=review

> JSTests/stress/string-replaceAll-2.js:2
> +const supportsReplaceAll = (String.prototype.replaceAll != undefined);

FYI, I added this so that I can run this test on Chrome and Firefox as well to validate the test results.

[Ross Kirsling]

Comment on attachment 384104 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=384104&action=review

It's a bit interesting that these tests fail silently in release, but I guess that's the sort of issue it is.

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

Ugh, it seems that I underestimated the reason for the two huge branches in replaceUsingRegExpSearch. Sorry about that. :(

> Source/JavaScriptCore/ChangeLog:15
> +        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.14-9 at https://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.replace

Oh, good catch! I guess there haven't been any tests for this.

BTW, this is now 21.1.3.17-6 in the living spec: https://tc39.es/ecma262/#sec-string.prototype.replace

[Yusuke Suzuki]

Comment on attachment 384104 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=384104&action=review

r=me too

> Source/JavaScriptCore/ChangeLog:31
> +        Ref: https://bugs.webkit.org/show_bug.cgi?id=204481

Maybe, due to the reason why we have two functions, we should split it. String#replace is one of the most frequently used function.

[Mark Lam]

Comment on attachment 384104 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=384104&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +        for webkit.org/b/202471.  The patch failed to handle InternalFunctions.
> 
> Ugh, it seems that I underestimated the reason for the two huge branches in replaceUsingRegExpSearch. Sorry about that. :(

The test that I just added will catch this from now on.

>> Source/JavaScriptCore/ChangeLog:15
>> +        Ref: 21.1.3.14-9 at https://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.replace
> 
> Oh, good catch! I guess there haven't been any tests for this.
> 
> BTW, this is now 21.1.3.17-6 in the living spec: https://tc39.es/ecma262/#sec-string.prototype.replace

Is the living spec constantly changing?  I'll stay with ecma-262/6.0 so that this ref doesn't go stale when the living spec changes again.  I think it's reasonable for the reader to look up a newer version if he/she wants it.

>> Source/JavaScriptCore/ChangeLog:31
>> +        Ref: https://bugs.webkit.org/show_bug.cgi?id=204481
> 
> Maybe, due to the reason why we have two functions, we should split it. String#replace is one of the most frequently used function.

I'll see what the microbenchmark says.

[Mark Lam]

Created attachment 384116 [details]
patch for landing (includes rebased ChakraCore test results).

[Mark Lam]

Landed in r252758: <http://trac.webkit.org/r252758>.