Closed Bug 1343375 Opened 7 years ago Closed 7 years ago

RegExp.prototype[@@replace] needs to re-read global/sticky flags for non-global opt

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(1 file, 1 obsolete file)

Test case:
---
var r = /a/y;
r.lastIndex = {
    valueOf() {
        r.compile("a", "")
        return 0;
    }
};
r[Symbol.replace]("a");
print(r.lastIndex);
---

Expected: Prints 0
Actual: Prints 1
This issue also affects RegExp.prototype[@@match]:
---
var r = /a/y;
r.lastIndex = {
    valueOf() {
        r.compile("a", "")
        return 0;
    }
};
r[Symbol.match]("a");
print(r.lastIndex);
---

Expected: Prints 0
Actual: Prints 1
I think it makes sense this fix this bug before working on bug 1317397, therefore marking this one as blocking 1317397.
Blocks: 1317397
Attached patch bug1343375.patch (obsolete) — Splinter Review
Also fixes bug 1343369 because it's related to this issue.

Updating RegExpLocalMatchOpt to properly handle recompilations made RegExpLocalMatchOpt identical to RegExpBuiltinExec, therefore I've simply removed RegExpLocalMatchOpt and called RegExpBuiltinExec instead.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8842848 - Flags: review?(arai.unmht)
Comment on attachment 8842848 [details] [diff] [review]
bug1343375.patch

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

Thanks!

Interestingly I don't see any notable performance difference between using RegExpLocalMatchOpt and RegExpBuiltinExec now.
this is a nice code reduction :)

::: js/src/builtin/RegExp.js
@@ +307,1 @@
>          if (functionalReplace) {

please remove braces

@@ +309,2 @@
>          }
>          if (firstDollarIndex !== -1) {

here too

::: js/src/builtin/RegExpLocalReplaceOpt.h.js
@@ +21,5 @@
>  #endif
>                    )
>  {
> +    // 21.2.5.2.2 RegExpBuiltinExec, step 4.
> +    var lastIndex = ToLength(rx.lastIndex);

it would be nice to explain this operation can recompile RegExp,
and that's the reason why steps 6-7 below checks global too,
even if this function is optimized for local replace.
Attachment #8842848 - Flags: review?(arai.unmht) → review+
Attached patch bug1343375.patchSplinter Review
Updated patch per review comments, carrying r+ from arai.
Attachment #8842848 - Attachment is obsolete: true
Attachment #8842864 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c7e13a967c
Update RegExp.prototype.replace and .match to call ToLength(lastIndex) for non-global RegExp and handle recompilations. r=arai
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a9c7e13a967c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.