465199 - RegExp lastIndex property set should not coerce type to number

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

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

Comment 1

[Jim Blandy :jimb]

Rob, would you be interested in taking this, since you've got bug 430930 already?

Comment 2

[Robert Sayre]

sure, I can take this

Comment 3

[Jim Blandy :jimb]

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.

Comment 4

[Jeff Walden [:Waldo]]

Patch done, running through tests now, easy enough it seems reasonable to steal without discussion...

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Patch and test

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.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Updated for fatvals

Comment 7

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

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.

Comment 8

[Jeff Walden [:Waldo]]

(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...

Comment 9

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/498f412bfa8f

Didn't see a consistent difference either way with the likely annotation -- landed as-is.

Comment 10

[Jeff Walden [:Waldo]]

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.

Comment 11

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/498f412bfa8f
http://hg.mozilla.org/mozilla-central/rev/df76c7ddf749

Comment 12

[Eric Shepherd [:sheppy]]

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?

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Eric Shepherd [:sheppy]]

Yeah, I'm not going to document this change. You're right about the degree of nitpickery; I can't imagine anyone will notice. :)