373118 - String.prototype.substr and others are not ECMA compliant

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nickolay_Ponomarev]

STR on trunk:
js> "test".substr(0)
test
js> "test".substr(0, undefined)

js>

ECMA says in B.2.3 step 3: "If length is *undefined*, use +<inf>; otherwise call ToInteger(length)". A similar wording is used to describe the behavior of Array.prototype.join, and it now (since bug 155285 was fixed) works as I'd expect: [1,2,3].join(undefined) == "1,2,3".

I couldn't find the substr tests in the testsuite (other than a few crasher tests).

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Trivial fix

Comment 2

[Brian Crowder]

Looking at the spec, 15.5.4.15, this same repair is needed for str_substring, also.  Thoughts?

Comment 3

[Jeff Walden [:Waldo]]

Hmm, this problem seems to be a general one -- for example, slice is affected while split is not.  We should fix them all, which means doing exhaustive testing of all the methods and comparing against spec to determine which ones are broken.  Anyone have time to do that?  :-)

Comment 4

[Brian Crowder]

Ugh.  Let's just hit the two or three we're sure of in this bug and worry about auditing the rest another time.

Comment 5

[Brian Crowder]

Would be a great "first bug" candidate or candidate(s), though...  we got some platform interns coming in a couple weeks?

Comment 6

[Brian Crowder]

Comment on attachment 257876 [details] [diff] [review]
Trivial fix

Bouncing this patch until it includes a more comprehensive fix/analysis.  I'll retarget the bug, too.

Comment 7

[Brandon Ferguson]

Attachment: Substr, substring, and slice fixed to handle undefined in second param

Here's an initial fix which allows the following to handle an undefined "end" param as per the ECMA spec:

* 15.5.4.15 - String.prototype.substring
* 15.5.4.13 - String.prototype.slice
* B.2.3 - String.prototype.substr

Let me know if I missed anything!

I'll post a separate patch for the tests that test for this bug.

Comment 8

[Brandon Ferguson]

Attachment: Tests for correct handling of undefined in the second param.

Second patch (since the JS tests aren't part of the default checkout) which test to make sure slice, substr, and substring are responding correctly to undefined as their second var (end).

Not sure if I added these to the correct directory (currently ecma) or if it's appropriate to put them here, but they're related to this bug so here they are.

Comment 9

[Brian Crowder]

Comment on attachment 262319 [details] [diff] [review]
Substr, substring, and slice fixed to handle undefined in second param

Great, thanks for the patch.  What other routines did you inspect for similar issues?  I'll let bc bless the test patch (thanks for that, too, though), and I will land this on Monday.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 262319 [details] [diff] [review]
Substr, substring, and slice fixed to handle undefined in second param

>Index: js/src/jsstr.c

>@@ -724,17 +724,17 @@ str_substring(JSContext *cx, JSObject *o
>@@ -1918,17 +1918,17 @@ str_substr(JSContext *cx, JSObject *obj,

>-        if (argc == 1) {
>+        if (JSVAL_IS_VOID(argv[1])) {

It took me the longest time to notice that these are actually correct (because nargs == 2 in the JSFunctionSpec); maybe it's just me, but I'd prefer a comment to that effect.

Also, nits on your tests: the last sentence of each description is grammatically questionable, in particular "That is that" and the entirety of the last sentence for substr.  :-)

Comment 11

[Brian Crowder]

> >-        if (argc == 1) {
> >+        if (JSVAL_IS_VOID(argv[1])) {
> 
> It took me the longest time to notice that these are actually correct (because
> nargs == 2 in the JSFunctionSpec); maybe it's just me, but I'd prefer a comment
> to that effect.

I puzzled over this too, briefly.  I'll add a comment when I check-in, if a new patch isn't up by then.

Comment 12

[Brandon Ferguson]

Attachment: Commented on lack of argc checks.

Thanks for the feedback everyone. Here's another version of the same patch as above just with commenting (if I'm too verbose, do feel free to edit it). 

Also btw, I hope it's not a faux pas that I took on an assigned bug - my apologies if so.

Comment 13

[Brandon Ferguson]

Attachment: More thorough ECMA compliance tests, corrected poor grammar

Updated the tests as well. There was some particularly bad grammar in there, thank you for that. =) 

Also I referred back to the ECMA spec (E262 Ed. 3 I think) and added checks for the each of the steps possible quarks (Such as substring swapping start and end, and slice and substring searching backwards with negative numbers). So there should now be (more) complete tests for:

15.5.4.13 - slice
15.5.4.15 - substring
B.2.3 - substr

However, this may not be needed at all; if not I can revert back to the old version no problem.

Comment 14

[Brandon Ferguson]

(In reply to comment #9)
> Great, thanks for the patch.  What other routines did you inspect for similar
> issues?  

I've covered all of the ones in the ECMA String spec that have rules involving handling undefined params. Besides the ones in this patched the others were:

String.prototype.split
String.prototype.indexOf
String.prototype.lastIndexOf

All of these handled undefined passed to them the very same as the arg being left out. 

I may follow up with the rest of the functions in the spec (there's not an uncheckable amount), but I wanted to confirm that the String class is clean before doing so. 

Comment 15

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)
> Created an attachment (id=262320) [details]
> Tests for correct handling of undefined in the second param. 
> 

Brandon, I definitely appreciate you working on adding tests for these features. I do have a couple of comments but please realize that any issues with the tests are my fault for not communicating better with the community. 

The way the test library was originally organized the ecma/, ecma_2/, ecma_3/ directories refer to the different editions of ECMA 262 and the corresponding section-based file names refer to the specific specification. These features were added in ECMA 262 3rd edition, so they should probably live in the ecma_3/ directory. The ecma_3 suite also has the "advantage" of being a slightly improved framework but that is not a big deal.

As the tests currently run, they output results using only the section and do not explicitly identify the sub test being run, e.g.

15.5.4.13 =  FAILED! expected: Testing
15.5.4.13 =  FAILED! expected: ting

I think it helps when the sub test description contains a small label pointing to the specific test which fails.

In the test 'Testing Step 7: "Compute max(Result(6)âResult(5),0)"', there is some cruft from copying and pasting the text from the spec's pdf.

Rather than push back and ask for new versions of the tests, if it is ok with you, I will work up a set for inclusion in ecma_3/String/ as an illustration.

Comment 16

[Brandon Ferguson]

Bob, thanks for the reply. This is fantastic information. And my pleasure writing the tests, I certainly need the practice. =)

Funny enough I was just looking at the test dir structure and noticed that I had done quite a bit incorrectly. The worst of this is that a lot of my tests (in the second update) are actually not needed because they are covered by *much* more thorough tests written back in 97. I completely missed these since the sections numbers apparently change from spec to spec (ie ecma_3's 15.5.4.15 is ecma_1's 15.5.4.10). Due to this I've obsoleted my "updated" patch.

With that, an illustration would be fantastic. I appreciate the thought, and look forward to seeing it.  

Thanks again for the reply the patient feedback - it was most helpful. 

Comment 17

[Brandon Ferguson]

> I may follow up with the rest of the functions in the spec (there's not an
> uncheckable amount), but I wanted to confirm that the String class is clean
> before doing so. 

Ack, apologies for the update volume today. I did a quick check through the rest of the classes and it seems that Array.prototype.slice, Array.prototype.sort, and Number.toString all have similar problems to this one; passing "undefined" as an argument does not have the same result as not specifying the argument. 


More specifically:

* Array.slice returns an empty array instead of using the array length as the end param
* Array.sort errors as "invalid Array.prototype.sort argument" instead of doing the default sort and returning a sorted array. 
* Number.toString errors as "Illegal Radix 0" instead of printing the number with base 10


Should I go ahead and take care of these in this bug (since they're so similar), or is it more appropriate to open a new one?

Comment 18

[Jeff Walden [:Waldo]]

(In reply to comment #12)
> Also btw, I hope it's not a faux pas that I took on an assigned bug - my
> apologies if so. 

If it's reasonably obvious no work has recently occurred, as it is here, then this is fine, particularly for small bugs.  If it's a larger bug, you may wish to mention there that you're working on it in case there's a collision.

Comment 19

[Brian Crowder]

(In reply to comment #17)
> Should I go ahead and take care of these in this bug (since they're so
> similar), or is it more appropriate to open a new one?

I'd like to see all of these get handled in this same bug, ideally.

Thanks again, Brandon

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

One comment before the next patch: please use C-style comments (/* */) instead of C++ line comments.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 262353 [details] [diff] [review]
Commented on lack of argc checks. 

>Index: js/src/jsstr.c

>+        // Using just JSVAL_IS_VOID() checks for both unspecified
>+        // and undefined args here since unspecified args with 
>+        // positions < function.length have value JSVAL_VOID
>+        // the same as undefined ones
>+        if (JSVAL_IS_VOID(argv[1])) {

I think the second half of this is reasonably clear to anyone who's worked on any of the SpiderMonkey code very much (and this is how it works in JS itself, too), so I'd try and cut this down a little.  Perhaps:

>+        /* NB: nargs == 2, so argv[1] is always in-bounds. */

Comment 22

[Brandon Ferguson]

(In reply to comment #21)
> >+        /* NB: nargs == 2, so argv[1] is always in-bounds. */

Excellent, I wasn't sure how much detail was appropriate. I'll go through and replace the comments with this more concise version. I'll also be sure to to use C style from here on out. 

I should have another patch that fixes the remaining issues of this nature up in a day or so.

Thanks again everyone for the feedback. 

Comment 23

[Brandon Ferguson]

Attachment: Fixes similar problems in Arrary, and Number. Concise c-style comments.

Apologies for the silence on this one, finals hit. =)

Here's a new version of the patch which addresses the same problem in Number and Array and gets rid of the C++ style comments.

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 264005 [details] [diff] [review]
Fixes similar problems in Arrary, and Number. Concise c-style comments.

(In reply to comment #23)
> Apologies for the silence on this one, finals hit. =)

Finals are fun like that, or something.  :-)  Do you plan to update the tests for the other cases you addressed in this patch?

Also, when you post a patch you should flag it for review from someone (see mozilla.org/owners.html to figure out a suitable person, in general).  This should have been a dropdown on the attachment submission page (or on the Details page once submitted), which you should twiddle to ? and an email address.  If you don't see it, grab an IRC client (ChatZilla's a Firefox extension, which might be easy) and ask on irc.mozilla.org/developers for someone to give you editbugs privileges.

Comment 25

[Brian Crowder]

Comment on attachment 264005 [details] [diff] [review]
Fixes similar problems in Arrary, and Number. Concise c-style comments.

+    /* NB: nargs == 2, so argv[1] is always in-bounds. */
+    if (argc != 0 && !JSVAL_IS_VOID(argv[0])) {

I think this comment is incorrect in num_toString and doesn't agree with your new code anyway.

I will r+ and land the next rev with that comment removed.

Comment 26

[Nickolay_Ponomarev]

crowder: did you forget to land this or are you waiting for an updated patch from Brandon?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

crowder, did you ever land this? 

Comment 28

[Brian Crowder]

This patch never did land, but it was also written before fast-natives, and also before the recent changes to minargs.

Igor:  Has the minargs stuff landed?  Any chance you can incorporate whatever is still-valid here into that patch?  If so, I'll link this bug to depend on that one; if not, I can write a new patch here which depends on that one.  Either way, I think that code will affect this enough to pin this to it.

Comment 29

[Brendan Eich [:brendan]]

Brian, you know how to check whether the minargs removal landed -- look for one fewer args to JS_FN than to JS_FS (4 instead of 5). It's all over the sources.

/be

Comment 30

[Brian Crowder]

Comment on attachment 264005 [details] [diff] [review]
Fixes similar problems in Arrary, and Number. Concise c-style comments.

(In reply to comment #29)
> Brian, you know how to check whether the minargs removal landed -- look for one
> fewer args to JS_FN than to JS_FS (4 instead of 5). It's all over the sources.

Yeah, just didn't have time to do this last night.  minargs removal for fast-natives has landed, so this patch will need to be reworked.  I've obsoleted the attached patch.  If Brandon or Waldo have time to update the patch to the new code (sorry for not landing sooner) that'd be a great help; if not, I'll hit it as soon as I can.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Waldo's somewhere in the New Jersey wilderness, so I wouldn't wait for him.

Comment 32

[Brandon Ferguson]

I *should* be able to update it this week, or weekend. Though if someone could point me to the minargs changes (haven't been keeping up with things) so I'd know about where to start here on adjustments, that'd be a fantastic help.

Comment 33

[Igor Bukanov]

(In reply to comment #32)
> Though if someone could
> point me to the minargs changes (haven't been keeping up with things) so I'd
> know about where to start here on adjustments, that'd be a fantastic help.

The minargs changes are simple: the fast native method must always check for argc before accessing any arg even if the arg number is less than the number of arguments in the method spec. 

Comment 34

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Comment 35

[Tom S]

Attachment: Fix array_sort, array_slice, str_substr(ing), str_slice

Comment 36

[Tom S]

Attachment: Testcase

Comment 37

[Tom S]

So lets this get done, reworked "Attachment 264005 [details] [diff]", to reflect what i think is the current style.

Comment 39

[Jeff Walden [:Waldo]]

Comment on attachment 483725 [details] [diff] [review]
Fix array_sort, array_slice, str_substr(ing), str_slice

For the future, it works best if you keep both patch and tests in the same attachment -- it's pretty easy to generate patches that add files using hg (I prefer the queues extension for doing this, but that particular extension's not necessary).  The location of the test in js/src/tests isn't super-important.  These days I usually pick a relevant-looking location in ecma_5/, for a patch that touches a bunch of things as here probably ecma_5/misc/, and give it some sort of vaguely descriptive name (explicit-undefined-arguments.js, perhaps).

Two other notes about tests.  First, don't forget to add the test to the relevant jstests.list file.  Second, JS tests are considered to fail if they don't include at least one call to the reportCompare function (to guard against completely-premature script completion being deemed a pass), so it's necessary to end tests with a |reportCompare(true, true)|.

But this all looks good to me, with those tweaks, so r=me.  I'll make the noted changes and land this and the tests within the next hour or so.

(Irrelevant aside: I wish some of these methods, most particularly str_substr here, were organized more like the algorithms in the spec, to make comparison of the two easier.  :-\ )

Comment 40

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/c3b2efe4da3c

Thanks for taking this and driving it the rest of the way!

Comment 41

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c3b2efe4da3c