1509768 - RegExp behaving very strangely

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect, P1)

Description

[snek]

Attachment: eshost output

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

Running this series of regex related methods returns unexpected results:

`'aa.ui.widget.Progressbar'.replace(/\./g, '/').replace('', 'baz')`


Actual results:

Got the output "aa/ui/widget/Progressbar"


Expected results:

got the output "bazaa/ui/widget/Progressbar"

Comment 1

[Tooru Fujisawa [:arai]]

regression window
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b6683e141c47c022598c0caac3ea8ba8c6236d42&tochange=d9b1a9829c8ee2862955043f28183efa07de3d2b

apparently from bug 887016

Comment 2

[Tooru Fujisawa [:arai]]

looks like rope with length>=24 inteacts badly with it

> var x = "r"; ("aa/ui/widget/Progressba" + x).replace('', 'baz')
"aa/ui/widget/Progressbar"
> ("aa/ui/widget/Progressbar").replace('', 'baz')
"bazaa/ui/widget/Progressbar"
> ("aa/ui/widget/Progressb" + x).replace('', 'baz')
"bazaa/ui/widget/Progressbr"

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Handle the case that String#replace is called with a empty string pattern on a rope.

BuildFlatRopeReplacement haven't handled the case that the pattern is empty string.
added the code for it at the beginning of the function (because it only matches to the head of the string)

Comment 4

[Tom S]

Comment on attachment 9027436 [details] [diff] [review]
Handle the case that String#replace is called with a empty string pattern on a rope.

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

I am probably not the best person to review this, but I looked at the code and understand that "pos < matchEnd" can never be true when both are zero.

Comment 5

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9a88ca20ebd876c51948bc171269502865c3ee
Bug 1509768 - Handle the case that String#replace is called with a empty string pattern on a rope. r=evilpie

Comment 6

[Tooru Fujisawa [:arai]]

ni? me to work on uplift

Comment 7

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4f9a88ca20eb

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-esr60) Handle the case that String#replace is called with a empty string pattern on a rope. r=evilpie

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Not a crash.
Just a wrong behavior in JS execution that the resulting value is different than expected.

User impact if declined: JS code (string replacement) doesn't work as expected but no replacement happens.

in particular, when using String.prototype.replace to prepend a string, it does nothing and doesn't prepend.

this would be very rare case (at least I haven't seen any wild code which uses replace for this purpose),
so this might not match the ESR uplift bar.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This adds missing code for handling the case, which is simple enough and won't cause any issue.

String or UUID changes made by this patch: none

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Handle the case that String#replace is called with a empty string pattern on a rope. r=evilpie

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 887016

User impact if declined: JS code (string replacement) doesn't work as expected but no replacement happens.

in particular, when using String.prototype.replace to prepend a string, it does nothing and doesn't prepend.

this would be very rare case (at least I haven't seen any wild code which uses replace for this purpose),
so this might not match the ESR uplift bar.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: --

List of other uplifts needed: none

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This adds missing code for handling the case, which is simple enough and won't cause any issue.

String changes made/needed: none

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9028182 [details] [diff] [review]
(mozilla-beta) Handle the case that String#replace is called with a empty string pattern on a rope. r=evilpie

This has been around long enough that it seems it can wait another cycle.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9028181 [details] [diff] [review]
(mozilla-esr60) Handle the case that String#replace is called with a empty string pattern on a rope. r=evilpie

Doesn't sound like this is needed for ESR60 either given how rare it is.