Bug 146298 – REGRESSION (r181889): basspro.com hangs on load under JSC::ErrorInstance::finishCreation(JSC::ExecState*, JSC::VM&, WTF::String const&, bool) + 2801 (JavaScriptCore + 3560689)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

We hang in the static function named functionCallBase() in ExceptionHelpers.cpp.

rdar://problem/21487736

[Michael Saboff]

*** Bug 146303 has been marked as a duplicate of this bug. ***

[Michael Saboff]

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.

[Ryosuke Niwa]

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.

[Mark Lam]

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.

[Michael Saboff]

(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.

[Michael Saboff]

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.

[Mark Lam]

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.

[Michael Saboff]

(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.

[Michael Saboff]

> 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*.

[Mark Lam]

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.

[Ryosuke Niwa]

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]??

[Michael Saboff]

Committed r185954: <http://trac.webkit.org/changeset/185954>

[Michael Saboff]

(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?