REGRESSION(r199075): String.prototype.replace fails after being used many times with different replace values

[Joseph Pecoraro]

* 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"

[Radar WebKit Bug Importer]

<rdar://problem/26404735>

[Joseph Pecoraro]

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]

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]

> Please remove that entry when fixing!

Test marked flakey in: <http://trac.webkit.org/changeset/201240>

[Saam Barati]

Sounds related to Fil's constant folding patch from a month ago.

[Saam Barati]

(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]

Created attachment 279539 [details]
patch

[Saam Barati]

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 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]

(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 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]

Please add a test as Joe pointed out.

[Filip Pizlo]

Comment on attachment 279539 [details]
patch

Wow!!!  R=me too

[Alexey Proskuryakov]

> 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]

Created attachment 279551 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 279551 [details]
patch for landing

Clearing flags on attachment: 279551

Committed r201254: <http://trac.webkit.org/changeset/201254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.