778531 - Array.prototype.join must perform ToUint32(length) before ToString(separator)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

Per spec, Array.prototype.join [15.4.4.5] performs ToUint32(length) before ToString(separator), cf. step 2/3 and step 5.


var object = {length: {valueOf: function(){ throw "length" }}};
var sep = {toString: function(){ throw "toString" }};
try {
  Array.prototype.join.call(object, sep);
  print("ERROR - expected exception");
} catch (e) {
  if (e !== "length") {
    print("ERROR - expected 'length' but was: " + e);
  } else {
    print("SUCCESS");
  }
}


Actual results:

Output is: `ERROR - expected 'length' but was: toString`


Expected results:

Output should be `SUCCESS`

Comment 1

[:Benjamin Peterson]

Attachment: update join and toLocaleString to follow the spec

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 646989 [details] [diff] [review]
update join and toLocaleString to follow the spec

Review of attachment 646989 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with or without changes.

::: js/src/tests/ecma_5/Array/join-01.js
@@ +59,5 @@
> +var gotLength = false, gotString = false;
> +arr = {length: {valueOf: function () { assertEq(gotString, false); gotLength = true; return 2; }},
> +      0: "x", 1: "z"};
> +var sep = {toString: function () { assertEq(gotLength, true); gotString = true; return "y"; }};
> +assertEq(Array.prototype.join.call(arr, sep), "xyz");

A nice way to test this kind of thing is to keep a log, like this:

var log = '';
arr = {length: {valueOf: function () { log += 'L'; return 2; }}, 0: "x", 1: "z"};
var sep = {toString: function () { log += 'S'; return "y"; }};
assertEq(Array.prototype.join.call(arr, sep), "xyz");
assertEq(log, 'LS');

I think that's a little easier to follow, and it's a stricter test that way. YMMV, just a thought. Same comment in toLocaleString-01.js.

Comment 3

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a28c47bf240f

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a28c47bf240f