From f96be554b92a1e72eb524a43ad488d354ac0b603 Mon Sep 17 00:00:00 2001
From: neis <neis@chromium.org>
Date: Wed, 29 Jun 2016 02:51:37 -0700
Subject: [PATCH] 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            | 42 ++++++++-----------------------------
 test/mjsunit/substr.js      | 19 +++++++++++++++++
 test/test262/test262.status |  3 ---
 3 files changed, 28 insertions(+), 36 deletions(-)

diff --git a/src/js/string.js b/src/js/string.js
index d2eaa32809..b3bc3933cd 100644
--- a/src/js/string.js
+++ b/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);
 }
 
 
diff --git a/test/mjsunit/substr.js b/test/mjsunit/substr.js
index cab8b1bf6d..83929362a0 100644
--- a/test/mjsunit/substr.js
+++ b/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);
+}
diff --git a/test/test262/test262.status b/test/test262/test262.status
index 5ac93d9af9..b17e6532dc 100644
--- a/test/test262/test262.status
+++ b/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