Changeset 215852 in webkit

Timestamp:

Apr 26, 2017 7:28:39 PM (7 years ago)

Author:

sbarati@apple.com

Message:

ASSERTION FAILED: inIndex != notFound in JSC::invalidParameterInSourceAppender()
​https://bugs.webkit.org/show_bug.cgi?id=170924
<rdar://problem/31721052>

Reviewed by Mark Lam.

JSTests:

• stress/error-message-for-function-base-not-found.js: Added.

(assert):
(throw.new.Error):

• stress/error-messages-for-in-operator-should-not-crash.js: Added.

(catch):

LayoutTests/imported/w3c:

• web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output-expected.txt:
• web-platform-tests/css-timing-1/frames-timing-functions-output-expected.txt:
• web-platform-tests/css-timing-1/step-timing-functions-output-expected.txt:

Source/JavaScriptCore:

The error message handler for "in" was searching for the literal
string "in". However, our parser incorrectly allows escaped characters
to be part of keywords. So this is parsed as "in" in JSC: "i\u006E".
It should not be parsed that way. I opened ​https://bugs.webkit.org/show_bug.cgi?id=171310
to address this issue.

Regardless, the error message handlers should handle unexpected text gracefully.
All functions that try to augment error messages with the goal of
providing a more textual context for the error message should use
the original error message instead of crashing when they detect
unexpected text.

This patch also changes the already buggy code that tries to find
the base of a function call. That could would fail for code like this:
"zoo.bar("/abc\)*/");". See ​https://bugs.webkit.org/show_bug.cgi?id=146304
It would think that the base is "z". However, the algorithm that tries
to find the base can often tell when it fails, and when it does, it should
happily return the approximate text error message instead of thinking
that the base is "z".

• runtime/ExceptionHelpers.cpp:

(JSC::functionCallBase):
(JSC::notAFunctionSourceAppender):
(JSC::invalidParameterInSourceAppender):

LayoutTests:

• js/let-syntax-expected.txt:

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/destructuring-assignment-accepts-iterables.js (modified) (2 diffs)
• JSTests/stress/error-message-for-function-base-not-found.js (added)
• JSTests/stress/error-messages-for-in-operator-should-not-crash.js (added)
• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output-expected.txt (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output-expected.txt (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css-timing-1/step-timing-functions-output-expected.txt (modified) (1 diff)
• LayoutTests/js/let-syntax-expected.txt (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ExceptionHelpers.cpp (modified) (4 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2017-04-26  Saam Barati  <sbarati@apple.com>
+
+        ASSERTION FAILED: inIndex != notFound in JSC::invalidParameterInSourceAppender()
+        https://bugs.webkit.org/show_bug.cgi?id=170924
+        <rdar://problem/31721052>
+
+        Reviewed by Mark Lam.
+
+        * stress/error-message-for-function-base-not-found.js: Added.
+        (assert):
+        (throw.new.Error):
+        * stress/error-messages-for-in-operator-should-not-crash.js: Added.
+        (catch):
+
 2017-04-26  Keith Miller  <keith_miller@apple.com>
 

trunk/JSTests/stress/destructuring-assignment-accepts-iterables.js

@@ -123 +123 @@
         }
     };
-}, "TypeError: [a, b, c] is not a function. (In '[a, b, c]', '[a, b, c]' is undefined)");
+}, "TypeError: undefined is not a function (near '...[a, b, c]...')");
 
 shouldThrow(function () {
@@ -131 +131 @@
         }
     };
-}, "TypeError: [a, b, c] is not a function. (In '[a, b, c]', '[a, b, c]' is undefined)");
+}, "TypeError: undefined is not a function (near '...[a, b, c]...')");
 
 shouldThrow(function () {

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-04-26  Saam Barati  <sbarati@apple.com>
+
+        ASSERTION FAILED: inIndex != notFound in JSC::invalidParameterInSourceAppender()
+        https://bugs.webkit.org/show_bug.cgi?id=170924
+        <rdar://problem/31721052>
+
+        Reviewed by Mark Lam.
+
+        * js/let-syntax-expected.txt:
+
 2017-04-26  Joanmarie Diggs  <jdiggs@igalia.com>
 

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2017-04-26  Saam Barati  <sbarati@apple.com>
+
+        ASSERTION FAILED: inIndex != notFound in JSC::invalidParameterInSourceAppender()
+        https://bugs.webkit.org/show_bug.cgi?id=170924
+        <rdar://problem/31721052>
+
+        Reviewed by Mark Lam.
+
+        * web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output-expected.txt:
+        * web-platform-tests/css-timing-1/frames-timing-functions-output-expected.txt:
+        * web-platform-tests/css-timing-1/step-timing-functions-output-expected.txt:
+
 2017-04-26  Ryan Haddad  <ryanhaddad@apple.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css-timing-1/cubic-bezier-timing-functions-output-expected.txt

@@ -1 +1 @@
 
-FAIL cubic-bezier easing with input progress greater than 1 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL cubic-bezier easing with input progress greater than 1 and where the tangent on the upper boundary is infinity target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL cubic-bezier easing with input progress less than 0 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL cubic-bezier easing with input progress less than 0 and where the tangent on the lower boundary is infinity target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
+FAIL cubic-bezier easing with input progress greater than 1 undefined is not a function (near '...target.animate...')
+FAIL cubic-bezier easing with input progress greater than 1 and where the tangent on the upper boundary is infinity undefined is not a function (near '...target.animate...')
+FAIL cubic-bezier easing with input progress less than 0 undefined is not a function (near '...target.animate...')
+FAIL cubic-bezier easing with input progress less than 0 and where the tangent on the lower boundary is infinity undefined is not a function (near '...target.animate...')
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css-timing-1/frames-timing-functions-output-expected.txt

@@ -5 +5 @@
 FAIL The number of frames is correctly reflected in the frames timing function output assert_equals: expected "0px" but got "auto"
 FAIL The number of frames is correctly reflected in the frames timing function output on CSS Transitions assert_equals: expected "0px" but got "auto"
-FAIL frames easing with input progress greater than 1 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL frames easing with input progress greater than 1.5 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL frames easing with input progress less than 0 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
+FAIL frames easing with input progress greater than 1 undefined is not a function (near '...target.animate...')
+FAIL frames easing with input progress greater than 1.5 undefined is not a function (near '...target.animate...')
+FAIL frames easing with input progress less than 0 undefined is not a function (near '...target.animate...')
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css-timing-1/step-timing-functions-output-expected.txt

@@ -1 +1 @@
 
-FAIL step-start easing with input progress greater than 1 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL step-end easing with input progress greater than 1 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL step-end easing with input progress greater than 2 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL step-start easing with input progress less than 0 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL step-start easing with input progress less than -1 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
-FAIL step-end easing with input progress less than 0 target.animate is not a function. (In 'target.animate', 'target.animate' is undefined)
+FAIL step-start easing with input progress greater than 1 undefined is not a function (near '...target.animate...')
+FAIL step-end easing with input progress greater than 1 undefined is not a function (near '...target.animate...')
+FAIL step-end easing with input progress greater than 2 undefined is not a function (near '...target.animate...')
+FAIL step-start easing with input progress less than 0 undefined is not a function (near '...target.animate...')
+FAIL step-start easing with input progress less than -1 undefined is not a function (near '...target.animate...')
+FAIL step-end easing with input progress less than 0 undefined is not a function (near '...target.animate...')
 

trunk/LayoutTests/js/let-syntax-expected.txt

@@ -405 +405 @@
 SyntaxError: Cannot use abbreviated destructuring syntax for keyword 'let'.
 PASS Has syntax error: ''use strict'; var {let} = 40;'
-TypeError: [let] is not a function. (In '[let]', '[let]' is undefined)
+TypeError: undefined is not a function (near '...[let]...')
 PASS Does not have syntax error: 'var [let] = 40;'
 SyntaxError: Cannot use 'let' as a variable name in strict mode.

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-04-26  Saam Barati  <sbarati@apple.com>
+
+        ASSERTION FAILED: inIndex != notFound in JSC::invalidParameterInSourceAppender()
+        https://bugs.webkit.org/show_bug.cgi?id=170924
+        <rdar://problem/31721052>
+
+        Reviewed by Mark Lam.
+
+        The error message handler for "in" was searching for the literal
+        string "in". However, our parser incorrectly allows escaped characters
+        to be part of keywords. So this is parsed as "in" in JSC: "i\u006E".
+        It should not be parsed that way. I opened https://bugs.webkit.org/show_bug.cgi?id=171310
+        to address this issue.
+        
+        Regardless, the error message handlers should handle unexpected text gracefully.
+        All functions that try to augment error messages with the goal of
+        providing a more textual context for the error message should use
+        the original error message instead of crashing when they detect
+        unexpected text.
+        
+        This patch also changes the already buggy code that tries to find
+        the base of a function call. That could would fail for code like this:
+        "zoo.bar("/abc\)*/");". See https://bugs.webkit.org/show_bug.cgi?id=146304
+        It would think that the base is "z". However, the algorithm that tries
+        to find the base can often tell when it fails, and when it does, it should
+        happily return the approximate text error message instead of thinking
+        that the base is "z".
+
+        * runtime/ExceptionHelpers.cpp:
+        (JSC::functionCallBase):
+        (JSC::notAFunctionSourceAppender):
+        (JSC::invalidParameterInSourceAppender):
+
 2017-04-26  Keith Miller  <keith_miller@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ExceptionHelpers.cpp

@@ -128 +128 @@
         // will not inlcude the text in between these parentheses, it will just be the desired
         // text that precedes the parentheses.
-        return sourceText;
+        return String();
     }
 
@@ -136 +136 @@
     // Note that we're scanning text right to left instead of the more common left to right, 
     // so syntax detection is backwards.
-    while (parenStack > 0) {
+    while (parenStack && idx) {
         UChar curChar = sourceText[idx];
-        if (isInMultiLineComment)  {
-            if (idx > 0 && curChar == '*' && sourceText[idx - 1] == '/') {
+        if (isInMultiLineComment) {
+            if (curChar == '*' && sourceText[idx - 1] == '/') {
                 isInMultiLineComment = false;
-                idx -= 1;
+                --idx;
             }
         } else if (curChar == '(')
-            parenStack -= 1;
+            --parenStack;
         else if (curChar == ')')
-            parenStack += 1;
-        else if (idx > 0 && curChar == '/' && sourceText[idx - 1] == '*') {
+            ++parenStack;
+        else if (curChar == '/' && sourceText[idx - 1] == '*') {
             isInMultiLineComment = true;
-            idx -= 1;
+            --idx;
         }
 
-        if (!idx)
-            break;
-
-        idx -= 1;
+        if (idx)
+            --idx;
+    }
+
+    if (parenStack) {
+        // As noted in the FIXME at the top of this function, there are bugs
+        // in the above string processing. This algorithm is mostly best effort
+        // and it works for most JS text in practice. However, if we determine
+        // that the algorithm failed, we should just return the empty value.
+        return String();
     }
 
@@ -178 +184 @@
 
     String base = functionCallBase(sourceText);
+    if (!base)
+        return defaultApproximateSourceError(originalMessage, sourceText);
     StringBuilder builder;
     builder.append(base);
@@ -206 +214 @@
     ASSERT(occurrence == ErrorInstance::FoundExactSource);
     auto inIndex = sourceText.reverseFind("in");
-    RELEASE_ASSERT(inIndex != notFound);
+    if (inIndex == notFound) {
+        // This should basically never happen, since JS code must use the literal
+        // text "in" for the `in` operation. However, if we fail to find "in"
+        // for any reason, just fail gracefully.
+        return originalMessage;
+    }
     if (sourceText.find("in") != inIndex)
         return makeString(originalMessage, " (evaluating '", sourceText, "')");