The default bug view has changed. See this FAQ.

Wrong results with String.prototype.replace, rope

VERIFIED FIXED in Firefox 8

Status

()

Core
JavaScript Engine
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: cdleary)

Tracking

({verified-aurora, verified-beta})

Other Branch
mozilla10
x86
Mac OS X
verified-aurora, verified-beta
Points:
---

Firefox Tracking Flags

(firefox8 fixed, firefox9 fixed)

Details

(Whiteboard: [qa!])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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.
Assignee: general → cdleary
Created attachment 564285 [details] [diff] [review]
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

6 years ago
Created attachment 564289 [details] [diff] [review]
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

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

Well, one of us should r? the other :)
Attachment #564289 - Flags: review?(cdleary)
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 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.
Attachment #564289 - Flags: review?(cdleary) → review+

Comment 6

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5259affbebbc
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/5259affbebbc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 8

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

Comment 9

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

Comment 10

6 years ago
(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.)
Asa, how does one propose a bug get escalated into a sooner release channel?

/be

Comment 12

6 years ago
(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

6 years ago
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.
Attachment #564289 - Flags: approval-mozilla-beta?
Attachment #564289 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #564289 - Flags: approval-mozilla-beta?
Attachment #564289 - Flags: approval-mozilla-beta+
Attachment #564289 - Flags: approval-mozilla-aurora?
Attachment #564289 - Flags: approval-mozilla-aurora+

Comment 14

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

Updated

6 years ago
status-firefox8: --- → fixed
status-firefox9: --- → fixed
Whiteboard: [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

5 years ago
(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 ;).
Yeah, must have been something I missed. That does the trick. Thanks.

Verified based on comment 15 and 16.
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.