We hang in the static function named functionCallBase() in ExceptionHelpers.cpp. rdar://problem/21487736
*** Bug 146303 has been marked as a duplicate of this bug. ***
Created attachment 255537 [details] Patch Without the fix the test will fail. Filed <https://bugs.webkit.org/show_bug.cgi?id=146305> - "Add command line option and private function to set timeout in jsc command" to add a timeout to jsc. I'll add a timeout to this test when that is done.
Comment on attachment 255537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255537&action=review r=me assuming at least my comment about sourceText[rightToLeftIndex - 1] is addressed. > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:121 > + unsigned index = 0; > + unsigned rightToLeftIndex = rightToLeftIndexFromIndex(length, index); I got confused what "rightToLeftIndex" meant. I think it's better to call the former traversalCount or traversalIndex and the latter indexInSourceText or sourceIndex. But I really don't think having an index that traverses the string backwards is bad. > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:143 > - if (curChar == '*' && sourceText[idx - 1] == '/') { > + if (curChar == '*' && sourceText[rightToLeftIndex - 1] == '/') { rightToLeftIndex is not guaranteed to be greater than 0 is it? Specifically when index = length - 1, rightToLeftIndex = length - 1 - index = 0 so this may end up accessing a random byte. I think we should explicitly check for rightToLeftIndex being greater than 0. > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:151 > - else if (curChar == '/' && sourceText[idx - 1] == '*') { > + else if (curChar == '/' && sourceText[rightToLeftIndex - 1] == '*') { Ditto. > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:-144 > - > - idx -= 1; If we kept the original sematnics of idx, we can check whether idx is 0 or not before the decrement and exit early when it is.
Comment on attachment 255537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255537&action=review > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:145 > + if (curChar == '*' && sourceText[rightToLeftIndex - 1] == '/') { > isInMultiLineComment = false; > - idx -= 1; > + index++; I know this is pre-existing code, but this backwards scan method of detecting the start of a multiline comment seems inadequate to me. Consider the case where we have a comment like this: /* A /* B */. Scanning backwards, we’d think the /* before B is the start of the comment, whereas a source parser will say that the /* before A is the start of the comment. You should test this case and see if we need to file a bug for it.
(In reply to comment #4) > Comment on attachment 255537 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255537&action=review > > > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:145 > > + if (curChar == '*' && sourceText[rightToLeftIndex - 1] == '/') { > > isInMultiLineComment = false; > > - idx -= 1; > > + index++; > > I know this is pre-existing code, but this backwards scan method of > detecting the start of a multiline comment seems inadequate to me. Consider > the case where we have a comment like this: /* A /* B */. Scanning > backwards, we’d think the /* before B is the start of the comment, whereas a > source parser will say that the /* before A is the start of the comment. > You should test this case and see if we need to file a bug for it. I filed <https://bugs.webkit.org/show_bug.cgi?id=146304> "ExceptionHelpers.cpp::functionCallBase doesn't properly handle embedded comments and string literals", but forgot to mention it in a FIXME comment. I will shortly upload a new patch that addresses Ryosuke's concerns and includes a FIXME comment.
Created attachment 255558 [details] Patch Reverted back to the original method. Added checks that idx is inbounds as well as an exit condition when idx goes to 0.
Comment on attachment 255558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255558&action=review > Source/JavaScriptCore/ChangeLog:16 > + We were underflowing in ExceptionHelpers.cpp::functionCallBase() with a right to left > + string index. Restructured the code to derive the right to left index from a normal > + index. Also added a check for the index being in range as a loop exit condition. > + > + * runtime/ExceptionHelpers.cpp: > + (JSC::rightToLeftIndexFromIndex): New helper function to map a forward index to a > + reverse index. > + (JSC::functionCallBase): > + (JSC::notAFunctionSourceAppender): Your comment is stale. Definitely no more rightToLeftIndexFromIndex(). > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:117 > + // FIXME: This function has simple processing of /* */ style comments. > + // It doesn't properly handle embedded comments of string literals that contain > + // parenthesis or comment constructs, e.g. foo.bar("/abc\)*/"). > + // https://bugs.webkit.org/show_bug.cgi?id=146304 I suggest moving this comment down to just above the while loop below where it is more relevant. > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:123 > + if (sourceLength < 2 || sourceText[idx] != ')') { > // For function calls that have many new lines in between their open parenthesis > // and their closing parenthesis, the text range passed into the message appender > // will not inlcude the text in between these parentheses, it will just be the desired Is this (sourceLength < 2) correct? I don’t entirely understand what the original code is checking for here, but the comment below it seems to suggest that it is intended to allow for sourceText that is more than 1 in length.
(In reply to comment #7) > Comment on attachment 255558 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255558&action=review > > > Source/JavaScriptCore/ChangeLog:16 > > + We were underflowing in ExceptionHelpers.cpp::functionCallBase() with a right to left > > + string index. Restructured the code to derive the right to left index from a normal > > + index. Also added a check for the index being in range as a loop exit condition. > > + > > + * runtime/ExceptionHelpers.cpp: > > + (JSC::rightToLeftIndexFromIndex): New helper function to map a forward index to a > > + reverse index. > > + (JSC::functionCallBase): > > + (JSC::notAFunctionSourceAppender): > > Your comment is stale. Definitely no more rightToLeftIndexFromIndex(). I updated it to say that we now check idx for underflow and terminate when it gets to 0. > > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:117 > > + // FIXME: This function has simple processing of /* */ style comments. > > + // It doesn't properly handle embedded comments of string literals that contain > > + // parenthesis or comment constructs, e.g. foo.bar("/abc\)*/"). > > + // https://bugs.webkit.org/show_bug.cgi?id=146304 > > I suggest moving this comment down to just above the while loop below where > it is more relevant. I put this comment at the top of the loop since it describes short comings of the function and not just the reverse index loop. > > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:123 > > + if (sourceLength < 2 || sourceText[idx] != ')') { > > // For function calls that have many new lines in between their open parenthesis > > // and their closing parenthesis, the text range passed into the message appender > > // will not inlcude the text in between these parentheses, it will just be the desired > > Is this (sourceLength < 2) correct? I don’t entirely understand what the > original code is checking for here, but the comment below it seems to > suggest that it is intended to allow for sourceText that is more than 1 in > length. This is a little subtle. The original code basically says that if we don't have a trailing close parenthesis, there is no need for further processing. My added check says that if the length is 0 or 1, there is no need to process as well because there can't be a match set of parenthesis. Without checking for length of 0, the existing check underflows in the string. The check for length of 1 protects the idx -= 1 on line 130. The comment below basically says that for multiline function declarations, this function will never see the opening and closing parentheses.
> I put this comment at the top of the loop since it describes short comings > of the function and not just the reverse index loop. I mean *top to the function*.
Comment on attachment 255558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255558&action=review r=me >>> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:117 >>> + // https://bugs.webkit.org/show_bug.cgi?id=146304 >> >> I suggest moving this comment down to just above the while loop below where it is more relevant. > > I put this comment at the top of the loop since it describes short comings of the function and not just the reverse index loop. ok. nit: adding a line break between the comment and the next line would help. As is, I instinctively associate it with the if (sourceLength < 2 || sourceText[idx] != ')’) check. But I won’t be heart broken if you don’t. >>> Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:123 >>> // will not inlcude the text in between these parentheses, it will just be the desired >> >> Is this (sourceLength < 2) correct? I don’t entirely understand what the original code is checking for here, but the comment below it seems to suggest that it is intended to allow for sourceText that is more than 1 in length. > > This is a little subtle. The original code basically says that if we don't have a trailing close parenthesis, there is no need for further processing. My added check says that if the length is 0 or 1, there is no need to process as well because there can't be a match set of parenthesis. Without checking for length of 0, the existing check underflows in the string. The check for length of 1 protects the idx -= 1 on line 130. > > The comment below basically says that for multiline function declarations, this function will never see the opening and closing parentheses. Sounds good.
Comment on attachment 255558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255558&action=review > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:136 > - if (curChar == '*' && sourceText[idx - 1] == '/') { > + if (idx > 1 && curChar == '*' && sourceText[idx - 1] == '/') { So '/' can never appear at sourceText[0]??
Committed r185954: <http://trac.webkit.org/changeset/185954>
(In reply to comment #11) > Comment on attachment 255558 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255558&action=review > > > Source/JavaScriptCore/runtime/ExceptionHelpers.cpp:136 > > - if (curChar == '*' && sourceText[idx - 1] == '/') { > > + if (idx > 1 && curChar == '*' && sourceText[idx - 1] == '/') { > > So '/' can never appear at sourceText[0]?? I'll change the check to "idx > 0" here and the other location as well. rs=you?