690959 - Wrong results with String.prototype.replace, rope

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

var s = 'abcdFF0123456789012345fail';
s = s.replace("abcd", "0123456789012345678901234567890123456789012FF");
//({})[s];
s = s.replace("FF0123456789012345fail", "ok");
assertEq(s, '0123456789012345678901234567890123456789012FFok');

The first replace() call works as expected, but the second one fails to match anything. Uncommenting the third line causes the test to pass. My theory is the relevant side effect here is that ({})[s] flattens s; if so, we have a bug with replace() on non-flattened strings.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP: fix rope string flat match.

When testing matches across rope nodes we failed to reset the "current leaf node" pointer. WIP because I'm writing some more tests, this code is a little bit scary and deserves some more coverage.

Comment 2

[Luke Wagner [:luke]]

Attachment: same fix, different comments

Hah!  I just finished jit-testing my patch which is equivalent to yours, but I also tweaked the comments.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 564289 [details] [diff] [review]
same fix, different comments

Well, one of us should r? the other :)

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Ah, I tried flagging the assignee, but I guess that's not too effective these days. :-(

Writing tests for this path is harder than one would expect because we also test the textstr against |textstrlen >> sRopeMatchThresholdLog2|. I was mentioning to jorendorff that path coverage would have probably pointed this untested path out to us.

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 564289 [details] [diff] [review]
same fix, different comments

If you think this doesn't need more tests it's a clear r+. :-) You know this area better than I do.

Comment 6

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5259affbebbc

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/5259affbebbc

Comment 8

[Andrew Paprocki]

Is there any chance this can get applied to aurora (ff9)?

Comment 9

[Luke Wagner [:luke]]

(In reply to Andrew Paprocki from comment #8)
The bug goes back to 4.0 (0777c65f92c9); is it biting you now?

Comment 10

[Andrew Paprocki]

(In reply to Luke Wagner [:luke] from comment #9)
Yes, finished rolling out 1.8.5 (ff4) 3 weeks ago and hit this during the process. (Jason put in the ticket after I gave him test case on IRC.)

Comment 11

[Brendan Eich [:brendan]]

Asa, how does one propose a bug get escalated into a sooner release channel?

/be

Comment 12

[Asa Dotzler [:asa]]

(In reply to Brendan Eich [:brendan] from comment #11)
> Asa, how does one propose a bug get escalated into a sooner release channel?
> 
> /be

By making a request on the patch for Aurora and or Beta. Also, if that request comes with a risk vs reward analysis and the motivation for bypassing the normal uplifts, it'll be a lot easier for the release team to make the call and sooner. The next triage for Beta is on Monday and the next triage for Aurora is on Tuesday. Both days at 2PM Pacific time.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 564289 [details] [diff] [review]
same fix, different comments

The fix is low risk; independently derived twice by Chris and I.  The reward is fixing a pretty fundamental correctness issue in string operations.

Comment 14

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c50a1970615
https://hg.mozilla.org/releases/mozilla-beta/rev/8e4532857b15

Comment 15

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0) Gecko/20100101 Firefox/9.0 beta 3
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111130 Firefox/10.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a1) Gecko/20111130 Firefox/11.0a1

Used the code from comment 0 to check this issue with Web Console and Error Console (only changed the assert part with 's'):

var s = 'abcdFF0123456789012345fail';
s = s.replace("abcd", "0123456789012345678901234567890123456789012FF");
//({})[s];
s = s.replace("FF0123456789012345fail", "ok");
s;

This works fine in Webconsole, but Error console displays the reported behavior when evaluation the code. Checked on F9Beta3, F10Aurora, F11Nightly-all the same.

Is this known or expected?

Comment 16

[Luke Wagner [:luke]]

(In reply to Virgil Dicu [QA] from comment #15)
Phew, you gave me a scare.  If you change the line comment to /*({})[s];*/ then you get the expected result ;).

Comment 17

[Virgil Dicu [:virgil] [QA]]

Yeah, must have been something I missed. That does the trick. Thanks.

Verified based on comment 15 and 16.