Changeset 252758 in webkit


Ignore:
Timestamp:
Nov 21, 2019 6:16:57 PM (4 years ago)
Author:
mark.lam@apple.com
Message:

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.

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

Location:
trunk
Files:
1 added
1 deleted
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/JSTests/ChakraCore.yaml

    r251819 r252758  
    13031303  cmd: runChakra :baseline, "NoException", "compare.baseline", []
    13041304- path: ChakraCore/test/Strings/replace.js
    1305   cmd: runChakra :baseline, "NoException", "replace.baseline-jsc", []
     1305  cmd: runChakra :baseline, "NoException", "replace.baseline", []
    13061306- path: ChakraCore/test/Strings/trim.js
    13071307  cmd: runChakra :baseline, "NoException", "trim.baseline", []
  • trunk/JSTests/ChangeLog

    r252754 r252758  
     12019-11-21  Mark Lam  <mark.lam@apple.com>
     2
     3        Fix broken String.prototype.replace() and replaceAll().
     4        https://bugs.webkit.org/show_bug.cgi?id=204479
     5        <rdar://problem/57354854>
     6
     7        Reviewed by Ross Kirsling and Yusuke Suzuki.
     8
     9        * ChakraCore.yaml:
     10        * ChakraCore/test/Strings/replace.baseline-jsc: Removed.
     11        - We no longer need this because we've fixed the spec compliance issue in replace().
     12        * stress/string-replaceAll-2.js: Added.
     13
    1142019-11-21  Yusuke Suzuki  <ysuzuki@apple.com>
    215
  • trunk/Source/JavaScriptCore/ChangeLog

    r252756 r252758  
     12019-11-21  Mark Lam  <mark.lam@apple.com>
     2
     3        Fix broken String.prototype.replace() and replaceAll().
     4        https://bugs.webkit.org/show_bug.cgi?id=204479
     5        <rdar://problem/57354854>
     6
     7        Reviewed by Ross Kirsling and Yusuke Suzuki.
     8
     9        String.prototype.replace() regressed due to r252683: <https://trac.webkit.org/r252683>
     10        for webkit.org/b/202471.  The patch failed to handle InternalFunctions.
     11
     12        This patch also fixed a spec compliance bug for String.prototype.replace() i.e.
     13        the replaceValue needs to be evaluated before we check if there's a match in the
     14        source string.
     15        Ref: 21.1.3.16-6 at https://www.ecma-international.org/ecma-262/10.0/#sec-string.prototype.replace
     16
     17        For String.prototype.replaceAll(), make sure it "behaves just like
     18        String.prototype.replace if searchValue is a global regular expression".
     19        Ref: https://github.com/tc39/proposal-string-replaceall
     20
     21        r252683 also made replaceUsingStringSearch() work the same way as
     22        replaceUsingRegExpSearch().  I think this is the wrong trade off to make.
     23        replaceUsingRegExpSearch() expects each search leg to do a RegExp search, which
     24        is inherently expensive.  We shouldn't make string searches slower just because
     25        the RegExp search does it that way.
     26
     27        However, at https://bugs.webkit.org/show_bug.cgi?id=202471#c22, Ross pointed out
     28        that JetStream 2 results appeared to be neutral.  I think we should double check
     29        with a micro-benchmark as well.  I'll leave this for a later patch.  For now, the
     30        goal of this patch is simply to achieve correctness.
     31        Ref: https://bugs.webkit.org/show_bug.cgi?id=204481
     32
     33        * runtime/StringPrototype.cpp:
     34        (JSC::replaceUsingRegExpSearch):
     35        (JSC::replaceUsingStringSearch):
     36
    1372019-11-21  Per Arne Vollan  <pvollan@apple.com>
    238
  • trunk/Source/JavaScriptCore/runtime/StringPrototype.cpp

    r252754 r252758  
    665665                            groups->putDirect(vm, Identifier::fromString(vm, groupName), patternValue);
    666666                    }
    667 
    668667                }
    669668
     
    788787    RETURN_IF_EXCEPTION(scope, nullptr);
    789788
    790     size_t matchStart = string.find(searchString);
    791     if (matchStart == notFound)
    792         return jsString;
    793 
    794789    CallData callData;
    795790    CallType callType = getCallData(vm, replaceValue, callData);
     
    800795        RETURN_IF_EXCEPTION(scope, nullptr);
    801796    } else {
    802         cachedCall.emplace(globalObject, callFrame, jsCast<JSFunction*>(replaceValue), 3);
    803         cachedCall->setThis(jsUndefined());
    804     }
     797        JSFunction* function = jsDynamicCast<JSFunction*>(vm, replaceValue);
     798        if (function) {
     799            cachedCall.emplace(globalObject, callFrame, function, 3);
     800            cachedCall->setThis(jsUndefined());
     801        }
     802    }
     803
     804    size_t matchStart = string.find(searchString);
     805    if (matchStart == notFound)
     806        return jsString;
    805807
    806808    size_t endOfLastMatch = 0;
     
    809811    Vector<String, 16> replacements;
    810812    do {
    811         if (cachedCall) {
    812             auto* substring = jsSubstring(vm, string, matchStart, searchStringLength);
    813             RETURN_IF_EXCEPTION(scope, nullptr);
    814             cachedCall->clearArguments();
    815             cachedCall->appendArgument(substring);
    816             cachedCall->appendArgument(jsNumber(matchStart));
    817             cachedCall->appendArgument(jsString);
    818             ASSERT(!cachedCall->hasOverflowedArguments());
    819             JSValue replacement = cachedCall->call();
     813        if (callType != CallType::None) {
     814            JSValue replacement;
     815            if (cachedCall) {
     816                auto* substring = jsSubstring(vm, string, matchStart, searchStringLength);
     817                RETURN_IF_EXCEPTION(scope, nullptr);
     818                cachedCall->clearArguments();
     819                cachedCall->appendArgument(substring);
     820                cachedCall->appendArgument(jsNumber(matchStart));
     821                cachedCall->appendArgument(jsString);
     822                ASSERT(!cachedCall->hasOverflowedArguments());
     823                replacement = cachedCall->call();
     824            } else {
     825                MarkedArgumentBuffer args;
     826                auto* substring = jsSubstring(vm, string, matchStart, searchString.impl()->length());
     827                RETURN_IF_EXCEPTION(scope, nullptr);
     828                args.append(substring);
     829                args.append(jsNumber(matchStart));
     830                args.append(jsString);
     831                ASSERT(!args.hasOverflowed());
     832                replacement = call(globalObject, replaceValue, callType, callData, jsUndefined(), args);
     833            }
    820834            RETURN_IF_EXCEPTION(scope, nullptr);
    821835            replaceString = replacement.toWTFString(globalObject);
     
    827841
    828842        size_t matchEnd = matchStart + searchStringLength;
    829         if (cachedCall)
     843        if (callType != CallType::None)
    830844            replacements.append(replaceString);
    831845        else {
Note: See TracChangeset for help on using the changeset viewer.