Bug 157968

Summary: REGRESSION(r199075): String.prototype.replace fails after being used many times with different replace values
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, commit-queue, fpizlo, ggaren, joepeck, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
rniwa: review+
patch for landing none

Joseph Pecoraro
Reported 2016-05-20 21:30:37 PDT
* SUMMARY String.prototype.replace fails used many times with different replace values. * TEST <script> (function() { var result = "foo".replace(/f/g, ""); if (result != "oo") throw "Error: bad result: "+ result; for (var i = 0; i < 100000; ++i) result = "foo".replace(/f/g, 42); if (result != "42oo") throw "Error: bad result: "+ result; })(); </script> * STEPS TO REPRODUCE 1. Run test => Unexpected Exception, test should not have an exception => The Error is: "Error: bad result: oo"
Attachments
patch (2.48 KB, patch)
2016-05-21 00:33 PDT, Saam Barati 		rniwa: review+
DetailsFormatted DiffDiff
patch for landing (3.15 KB, patch)
2016-05-22 11:42 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-05-20 21:31:04 PDT
<rdar://problem/26404735>
Joseph Pecoraro
Comment 2 2016-05-20 21:42:20 PDT
Bisection results: <http://trac.webkit.org/log/trunk/?rev=199084&stop_rev=199073> Most likely culprit is: DFG and FTL should constant-fold RegExpExec, RegExpTest, and StringReplace <​https://bugs.webkit.org/show_bug.cgi?id=155270> <http://trac.webkit.org/changeset/199075>
Joseph Pecoraro
Comment 3 2016-05-20 22:20:05 PDT
Based on the order of tests run on the bots "js/regress/string-replace-generic.html" can fail if "js/regress/string-replace-empty.html" runs before it. So, in the mean time I am marking "js/regress/string-replace-generic.html" as [ Pass Fail ] in LayoutTests/TestExpectations. Please remove that entry when fixing!
Joseph Pecoraro
Comment 4 2016-05-20 22:22:08 PDT
> Please remove that entry when fixing! Test marked flakey in: <http://trac.webkit.org/changeset/201240>
Saam Barati
Comment 5 2016-05-20 22:25:40 PDT
Sounds related to Fil's constant folding patch from a month ago.
Saam Barati
Comment 6 2016-05-20 23:57:42 PDT
(In reply to comment #5) > Sounds related to Fil's constant folding patch from a month ago. Oops. Looks like you already bisected to that patch
Saam Barati
Comment 7 2016-05-21 00:33:37 PDT
Created attachment 279539 [details] patch
Saam Barati
Comment 8 2016-05-21 00:34:54 PDT
Fil, we currently won't constant fold the test case now. It's because 42 is a number and not a string. Should we extend LazyJSValue::tryGetStringImpl to handle numbers too? This seems like something we really should constant fold.
Joseph Pecoraro
Comment 9 2016-05-21 02:22:39 PDT
Comment on attachment 279539 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279539&action=review > LayoutTests/TestExpectations:-984 > -webkit.org/b/157968 js/regress/string-replace-generic.html [ Pass Failure ] We should add a new test for this. We got lucky that a bot ran 2 tests in sequence and triggered this. It seems worthwhile to have a single test that covers this on its own.
Saam Barati
Comment 10 2016-05-21 10:42:14 PDT
(In reply to comment #9) > Comment on attachment 279539 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279539&action=review > > > LayoutTests/TestExpectations:-984 > > -webkit.org/b/157968 js/regress/string-replace-generic.html [ Pass Failure ] > > We should add a new test for this. We got lucky that a bot ran 2 tests in > sequence and triggered this. It seems worthwhile to have a single test that > covers this on its own. I'll add this to JSC's tests. It reproduces every time when run in JSC without concurrent JIT
Ryosuke Niwa
Comment 11 2016-05-21 11:38:26 PDT
Comment on attachment 279539 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279539&action=review > Source/JavaScriptCore/ChangeLog:10 > + There was a bug in the DFG where we were checking a condition > + on the wrong variable. LOL, that won't do any good!
Ryosuke Niwa
Comment 12 2016-05-21 11:39:03 PDT
Please add a test as Joe pointed out.
Filip Pizlo
Comment 13 2016-05-21 11:40:26 PDT
Comment on attachment 279539 [details] patch Wow!!! R=me too
Alexey Proskuryakov
Comment 14 2016-05-21 13:26:40 PDT
> Based on the order of tests run on the bots "js/regress/string-replace-generic.html" can fail if "js/regress/string-replace-empty.html" runs before it. This should never happen. Tests in one directory are supposed to run sequentially, and alphabetically. Do you have a link to a test run where this happened?
Saam Barati
Comment 15 2016-05-22 11:42:29 PDT
Created attachment 279551 [details] patch for landing
WebKit Commit Bot
Comment 16 2016-05-22 12:12:18 PDT
Comment on attachment 279551 [details] patch for landing Clearing flags on attachment: 279551 Committed r201254: <http://trac.webkit.org/changeset/201254>
WebKit Commit Bot
Comment 17 2016-05-22 12:12:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.