1163757 - String.prototype.replace doesn't handle lastIndex correctly if sticky flag is set.

Status: RESOLVED DUPLICATE of bug 887016

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

(I bumped into this while implementing RegExp.prototype[@@replace] in bug 887016, so it will be fixed anyway there, but it's blocked by bug 1108382, and I'm not sure when we can fix it, for now)

In RegExpBuiltinExec, lastIndex should not be set to 0 if sticky flag is set
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexpbuiltinexec
> 10. If global is false and sticky is false, let lastIndex be 0.
Also, it should be updated both failure and success cases.
>     c. If r is failure, then
>         i. If sticky is true, then
>             1. Let setStatus be Set(R, "lastIndex", 0, true).
> 18. If global is true or sticky is true,
>     a. Let setStatus be Set(R, "lastIndex", e, true).

But currently DoMatchForReplaceLocal passes 0 as start argument to RegExpShared::execute, and it doesn't update lastIndex.

Here is test case.
  var input = "abcdeabcdeabcdefghij";
  var re = new RegExp("abcde", "y");
  re.test(input);
  console.log(re.lastIndex);
  var ret = input.replace(re, "ABCDE");
  console.log(ret);
  console.log(re.lastIndex);
  re.lastIndex = 10;
  ret = input.replace(re, "ABCDE");
  console.log(ret);
  console.log(re.lastIndex);
  re.lastIndex = 15;
  ret = input.replace(re, "ABCDE");
  console.log(ret);
  console.log(re.lastIndex);

Expected result:
  5
  abcdeABCDEabcdefghij
  10
  abcdeabcdeABCDEfghij
  15
  abcdeabcdeabcdefghij
  0

Actual result (apparently 2nd and 3rd results don't make sense):
  5
  ABCDEabcdeabcdefghij
  5
  ABCDEabcdeabcdefghij
  10
  ABCDEabcdeabcdefghij
  15

Comment 1

[Tooru Fujisawa [:arai]]

Draft patch is ready.
I'll post it after it passes try run.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Use and update lastIndex in String.prototype.replace when sticky flag is set.

testcase in dce-with-rinstructions.js (added by bug 1050649) fails with this fix, do you think this can be a compatibility issue?

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31e46efcf063

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8604553 [details] [diff] [review]
Use and update lastIndex in String.prototype.replace when sticky flag is set.

Review of attachment 8604553 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this :)
I am afraid this is never that simple :/

::: js/src/jsstr.cpp
@@ +2589,5 @@
> +    size_t lastIndex = 0;
> +    if (sticky) {
> +        RootedValue lastIndexVal(cx);
> +        if (!rdata.g.getLastIndex(cx, &lastIndexVal))
> +            return false;

21.2.5.2.2 Runtime Semantics: RegExpBuiltinExec ( R, S )
Step 4. Let lastIndex be ToLength(Get(R,"lastIndex")).
Step 5. ReturnIfAbrupt(lastIndex).

These steps are unconditional, but here it is conditionally executed after the check of the sticky flag.
I guess these should be moved to the StrReplaceRegExp function.

Also, as this is being spec-ed now, I suggest you add "// Step X" annotation comments in these functions.  Note that the order of the steps can be observed by using a getter on the lastIndex field.

@@ +3350,5 @@
>      if (re.global()) {
>          if (!DoMatchForReplaceGlobal(cx, res, linearStr, re, rdata))
>              return false;
>      } else {
> +        if (!DoMatchForReplaceLocal(cx, res, linearStr, re, rdata, re.sticky()))

We already give RegExpShared as argument, no need to add a sticky argument.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8604553 [details] [diff] [review]
Use and update lastIndex in String.prototype.replace when sticky flag is set.

Review of attachment 8604553 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +2620,5 @@
>          return true;
> +    }
> +
> +    if (sticky)
> +        rdata.g.setLastIndex(cx, matches[0].limit);

This corresponds to 21.2.5.2.2 R Step 18., but we are in DoMatchForReplaceLocal, which implies global == false.  Thus this if branch should not exists here.

Comment 5

[Nicolas B. Pierron [:nbp]]

> ret = input.replace(re, "ABCDE");

To be more precise, this path correspond to str_replace, with the following trace:

[1,1'] String.prototype.replace
// jsstr.cpp:3525-2531
1. Let O be RequireObjectCoercible(this value).
2. ReturnIfAbrupt(O).
// jsstr.cpp mistakenly do step 8., and step 9.
// Step 8. IsCallable can be observed with a proxy [5]
3. If searchValue is not undefined, then
  // Currently we assume that this is a regexp, and don't check for @@replace.
  a. Let replacer be GetMethod(searchValue, @@replace).
  b. ReturnIfAbrupt(replacer).
  c. If replacer is not undefined, then
      i. Return Call(replacer, searchValue, «O, replaceValue»).

[2,2'] RegExp.prototype [ @@replace ] ( string, replaceValue )
// jsstr.cpp ignores Step 6. and 7. are ignored, but observable with a proxy.
8. Let global be ToBoolean(Get(rx, "global")).
// jsstr.cpp:3291: is unfallible, and do not accept this flag to be changed.
10.c. Let setStatus be Set(rx, "lastIndex", 0, true).
** fixed by the attached patch **
13.a. Let result be RegExpExec(rx, S).

[3] Runtime Semantics: RegExpExec ( R, S )
// No code associated
[4,4'] Runtime Semantics: RegExpBuiltinExec ( R, S )
4. Let lastIndex be ToLength(Get(R,"lastIndex")).
6. Let global be ToBoolean(Get(R, "global")).
8. Let sticky be ToBoolean(Get(R, "sticky")).
(see review comments)


So, my conclusion is that we probably want to rewrite all these pieces of code to follow the spec, instead of being undocumented code, with no standard annotation.

Also, we probably want to add tests cases to verify the order of observable calls to IsCallable / Get.


[1] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.replace
[1'] https://dxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp#3520-3575
[2] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexp.prototype-@@replace
[2'] https://dxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp?from=StrReplaceRegExp&case=true#3273-3329
[3] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexpexec
[4] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexpbuiltinexec
[4'] https://dxr.mozilla.org/mozilla-central/source/js/src/jsstr.cpp#2568-2584

[5] https://dxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp#2593-2599,2611-2625

Comment 6

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

as discussed in IRC, and suggested in the review comments, I'll fix all of them in bug 887016.
I'm going to move all RegExp part to builtin/RegExp.js (or maybe builtin/RegExp.cpp, depending on the result of performance comparizon)
Also, I'll add more testcases for observation pointed out in review comments :)