Last Comment Bug 690959 - Wrong results with String.prototype.replace, rope
: Wrong results with String.prototype.replace, rope
Status: VERIFIED FIXED
[qa!]
: verified-aurora, verified-beta
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-30 16:11 PDT by Jason Orendorff [:jorendorff]
Modified: 2013-12-27 14:33 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
WIP: fix rope string flat match. (2.33 KB, patch)
2011-10-03 11:59 PDT, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Splinter Review
same fix, different comments (2.47 KB, patch)
2011-10-03 12:14 PDT, Luke Wagner [:luke]
cdleary: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-09-30 16:11:48 PDT
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) 2011-10-03 11:59:12 PDT
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 Luke Wagner [:luke] 2011-10-03 12:14:28 PDT
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 Luke Wagner [:luke] 2011-10-03 12:15:55 PDT
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) 2011-10-03 12:29:00 PDT
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) 2011-10-03 12:29:59 PDT
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 7 Marco Bonardo [::mak] 2011-10-05 05:07:18 PDT
https://hg.mozilla.org/mozilla-central/rev/5259affbebbc
Comment 8 Andrew Paprocki 2011-10-08 09:23:16 PDT
Is there any chance this can get applied to aurora (ff9)?
Comment 9 Luke Wagner [:luke] 2011-10-08 23:34:47 PDT
(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 2011-10-09 07:43:29 PDT
(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] 2011-10-09 13:40:23 PDT
Asa, how does one propose a bug get escalated into a sooner release channel?

/be
Comment 12 Asa Dotzler [:asa] 2011-10-09 13:45:25 PDT
(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] 2011-10-09 14:48:42 PDT
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 15 Virgil Dicu [:virgil] [QA] 2011-11-30 08:34:23 PST
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] 2011-11-30 10:07:02 PST
(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] 2011-12-02 04:37:47 PST
Yeah, must have been something I missed. That does the trick. Thanks.

Verified based on comment 15 and 16.

Note You need to log in before you can comment on or make changes to this bug.