Summary: | Fix broken String.prototype.replace() and replaceAll(). | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | fpizlo, keith_miller, msaboff, rmorisset, ross.kirsling, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 204481 | ||||||||
Attachments: |
|
Description
Mark Lam
2019-11-21 16:29:39 PST
Created attachment 384104 [details]
proposed patch.
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. 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 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. 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. Created attachment 384116 [details]
patch for landing (includes rebased ChakraCore test results).
Landed in r252758: <http://trac.webkit.org/r252758>. |