Summary: | REGRESSION(r199075): String.prototype.replace fails after being used many times with different replace values | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Joseph Pecoraro
2016-05-20 21:30:37 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> 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! > Please remove that entry when fixing! Test marked flakey in: <http://trac.webkit.org/changeset/201240> Sounds related to Fil's constant folding patch from a month ago. (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 Created attachment 279539 [details]
patch
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. 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. (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 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! Please add a test as Joe pointed out. Comment on attachment 279539 [details]
patch
Wow!!! R=me too
> 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?
Created attachment 279551 [details]
patch for landing
Comment on attachment 279551 [details] patch for landing Clearing flags on attachment: 279551 Committed r201254: <http://trac.webkit.org/changeset/201254> All reviewed patches have been landed. Closing bug. |