Closed Bug 465199 Opened 12 years ago Closed 10 years ago

RegExp lastIndex property set should not coerce type to number

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b3

People

(Reporter: brendan, Assigned: Waldo)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Our regexp implementation pre-dates ES3, which does specifies [[Put]] for all natives and does not customize [[Put]] for regexps to coerce lastIndex value to be an integer, in spite of the "is an integer" wording here:

15.10.7.5 lastIndex
The value of the lastIndex property is an integer that specifies the string position at which to start the next match.

We should fix this in a release that can get plenty of alpha and beta release soak time. It probably won't break anyone, but who knows?

Thanks to David-Sarah Hopwood for pointing this out.

/be
Blocks: es5
Rob, would you be interested in taking this, since you've got bug 430930 already?
sure, I can take this
Assignee: general → sayrer
D'oh!  I should have invited Waldo, who has bug 98409, which is related.  Ahem.
I'll just let you guys work out any shuffling about that would be helpful.
Patch done, running through tests now, easy enough it seems reasonable to steal without discussion...
Assignee: sayrer → jwalden+bmo
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9.3b2
Attached patch Patch and test (obsolete) — Splinter Review
This conflicts with njn's object reorg plans, but I guess those are on hold for awhile anyway, and this needs to be changed.  His previous reorg work to centralize slot access through one function is most definitely appreciated, to be sure.
Attachment #456990 - Flags: review?(cdleary)
Attachment #456990 - Attachment is obsolete: true
Attachment #459841 - Flags: review?(cdleary)
Attachment #456990 - Flags: review?(cdleary)
Comment on attachment 459841 [details] [diff] [review]
Updated for fatvals

Looks good.

Can you see if it makes any perf difference on the regexp benchmarks to throw a JS_LIKELY on the isNumber case? (regexp_exec_sub:5646)

Also, are you sure those JSObject methods belong in the regexp header? I would think that all the fslot-accessing doodads would go in the jsobj set of files.
Attachment #459841 - Flags: review?(cdleary) → review+
(In reply to comment #7)
> Also, are you sure those JSObject methods belong in the regexp header? I would
> think that all the fslot-accessing doodads would go in the jsobj set of files.

Class-specific methods are often defined in class-specific files, and some of these are fslots-accessing as well -- nothing unusual about this:

http://mxr.mozilla.org/mozilla-central/search?string=jsobject::
http://mxr.mozilla.org/mozilla-central/search?string=fslots

Will look at the regex perf consideration now...
http://hg.mozilla.org/tracemonkey/rev/498f412bfa8f

Didn't see a consistent difference either way with the likely annotation -- landed as-is.
Whiteboard: fixed-in-tracemonkey
Target Milestone: mozilla2.0b2 → mozilla2.0b3
http://hg.mozilla.org/tracemonkey/rev/df76c7ddf749

I could have sworn I had this bit in the patch (it's explicitly exercised in the test), wondering if maybe I had it in an early version, ran tests, then made some slight tweak or mis-rebase that killed it later.
http://hg.mozilla.org/mozilla-central/rev/498f412bfa8f
http://hg.mozilla.org/mozilla-central/rev/df76c7ddf749
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Looking at this code and the comments, I'm not clear on what changed here. How would this value not be a number in the first place? And if it's not, is this change throwing an exception or just assuming 0?
The difference is that previously, you couldn't *set* lastIndex to a non-integral value.  Whatever value you attempted to set would be converted to an integer (I think a 32-bit signed integer, going solely from memory), and the property would be set to that value.  Now, if you attempt to set lastIndex to a value, it has that value.  Matching with that regular expression in a way that would use lastIndex will only then convert it to an integer (and if it updates lastIndex later, it'll update it to an integral value).

Is that any clearer?  Honestly, I think this change is so much spec-nitpickery that it quite possibly isn't even worth pointing out in documentation.  We don't point out every bug we fix, after all.
Yeah, I'm not going to document this change. You're right about the degree of nitpickery; I can't imagine anyone will notice. :)
Keywords: dev-doc-needed
Depends on: 646490
You need to log in before you can comment on or make changes to this bug.