Fix order of conversions in String.prototype.substr.

The start argument must be converted to an integer before the length argument is
converted.  (Consequently, the start argument is converted even when the length
is 0.)  This matters because conversion is observable.

Also rewrite the function in a way that closely resembles the spec text.

R=littledan@chromium.org
BUG=v8:5140

Review-Url: https://codereview.chromium.org/2109583002
Cr-Commit-Position: refs/heads/master@{#37378}

src/js/string.js

@@ -447,43 +447,19 @@ function StringSubstring(start, end) {
 }
 
 
-// ES6 draft, revision 26 (2014-07-18), section B.2.3.1
-function StringSubstr(start, n) {
+// ecma262/#sec-string.prototype.substr
+function StringSubstr(start, length) {
   CHECK_OBJECT_COERCIBLE(this, "String.prototype.substr");
-
   var s = TO_STRING(this);
-  var len;
+  var size = s.length;
+  start = TO_INTEGER(start);
+  length = IS_UNDEFINED(length) ? size : TO_INTEGER(length);
 
-  // Correct n: If not given, set to string length; if explicitly
-  // set to undefined, zero, or negative, returns empty string.
-  if (IS_UNDEFINED(n)) {
-    len = s.length;
-  } else {
-    len = TO_INTEGER(n);
-    if (len <= 0) return '';
-  }
+  if (start < 0) start = MaxSimple(size + start, 0);
+  length = MinSimple(MaxSimple(length, 0), size - start);
 
-  // Correct start: If not given (or undefined), set to zero; otherwise
-  // convert to integer and handle negative case.
-  if (IS_UNDEFINED(start)) {
-    start = 0;
-  } else {
-    start = TO_INTEGER(start);
-    // If positive, and greater than or equal to the string length,
-    // return empty string.
-    if (start >= s.length) return '';
-    // If negative and absolute value is larger than the string length,
-    // use zero.
-    if (start < 0) {
-      start += s.length;
-      if (start < 0) start = 0;
-    }
-  }
-
-  var end = start + len;
-  if (end > s.length) end = s.length;
-
-  return %_SubString(s, start, end);
+  if (length <= 0) return '';
+  return %_SubString(s, start, start + length);
 }
 
 

test/mjsunit/substr.js

@@ -152,3 +152,22 @@ for (var i = 63; i >= 0; i--) {
   assertEquals(xl - offset, z.length);
   offset -= i;
 }
+
+
+// Order of conversions.
+{
+  let log = [];
+  let string = {[Symbol.toPrimitive]() { log.push("this"); return "abc" }};
+  let start = {[Symbol.toPrimitive]() { log.push("start"); return 0 }};
+  let length = {[Symbol.toPrimitive]() { log.push("length"); return 1 }};
+  assertEquals("a", String.prototype.substr.call(string, start, length));
+  assertEquals(["this", "start", "length"], log);
+}
+{
+  let log = [];
+  let string = {[Symbol.toPrimitive]() { log.push("this"); return "abc" }};
+  let start = {[Symbol.toPrimitive]() { log.push("start"); return 0 }};
+  let length = {[Symbol.toPrimitive]() { log.push("length"); return 0 }};
+  assertEquals("", String.prototype.substr.call(string, start, length));
+  assertEquals(["this", "start", "length"], log);
+}

test/test262/test262.status

@@ -469,9 +469,6 @@
   'annexB/built-ins/Date/prototype/setYear/time-clip': [FAIL],
   'annexB/built-ins/Date/prototype/setYear/year-number-relative': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=5140
-  'annexB/built-ins/String/prototype/substr/start-to-int-err': [FAIL],
-
   ######################## NEEDS INVESTIGATION ###########################
 
   # These test failures are specific to the intl402 suite and need investigation