Bug 112741 – JSC: Fix incorrect debugger column number value

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

The column number for function block entries can be erroneous.  Will fix.

ref: <rdar://problem/13277445>.

[Mark Lam]

Created attachment 193974 [details]
the patch

[WebKit Review Bot]

Attachment 193974 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h', u'Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/parser/ASTBuilder.h', u'Source/JavaScriptCore/parser/Lexer.cpp', u'Source/JavaScriptCore/parser/Lexer.h', u'Source/JavaScriptCore/parser/NodeConstructors.h', u'Source/JavaScriptCore/parser/Nodes.cpp', u'Source/JavaScriptCore/parser/Nodes.h', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/Parser.h', u'Source/JavaScriptCore/parser/ParserTokens.h', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/StringImpl.cpp', u'Source/WTF/wtf/text/StringImpl.h', u'Source/WTF/wtf/text/WTFString.h']" exit_code: 1
Source/JavaScriptCore/parser/Parser.h:975:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 193974 [details]
the patch

Attachment 193974 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17213721

[Mark Lam]

Created attachment 193980 [details]
new patch with the obsolete refs removed from the export list files and added a missing ChangeLog comment.

[WebKit Review Bot]

Attachment 193980 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.order', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCoreExports.def', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCoreExportGenerator/JavaScriptCoreExports.def.in', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h', u'Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/parser/ASTBuilder.h', u'Source/JavaScriptCore/parser/Lexer.cpp', u'Source/JavaScriptCore/parser/Lexer.h', u'Source/JavaScriptCore/parser/NodeConstructors.h', u'Source/JavaScriptCore/parser/Nodes.cpp', u'Source/JavaScriptCore/parser/Nodes.h', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/Parser.h', u'Source/JavaScriptCore/parser/ParserTokens.h', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/StringImpl.cpp', u'Source/WTF/wtf/text/StringImpl.h', u'Source/WTF/wtf/text/WTFString.h']" exit_code: 1
Source/JavaScriptCore/parser/Parser.h:975:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 193980 [details]
new patch with the obsolete refs removed from the export list files and added a missing ChangeLog comment.

View in context: https://bugs.webkit.org/attachment.cgi?id=193980&action=review

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1347
> +    size_t lineTerminatorPosition = source.reverseFindLineTerminator(actualCharPosition);

Does this vend the correct column number for script attributes and other things where the start of the source doesn't match the start of the line?   e.g.

<script>foo()
</script>

or <foo onload="blah()">
?

> Source/WTF/ChangeLog:18
> [phone]  Mark Lam  <[e-mail]>
> +
> +        Need a short description (OOPS!).
> +        Need the bug URL (OOPS!).
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * wtf/text/StringImpl.cpp:
> +        (WTF::StringImpl::reverseFindLineTerminator):
> +        (WTF):
> +        * wtf/text/StringImpl.h:
> +        (StringImpl):
> +        (WTF):
> +        (WTF::reverseFindLineTerminator):
> +        * wtf/text/WTFString.h:
> +        (WTF::String::reverseFindLineTerminator):
> +        (String):
> +

Double changelog :(

[Mark Lam]

Comment on attachment 193980 [details]
new patch with the obsolete refs removed from the export list files and added a missing ChangeLog comment.

View in context: https://bugs.webkit.org/attachment.cgi?id=193980&action=review

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:1347
>> +    size_t lineTerminatorPosition = source.reverseFindLineTerminator(actualCharPosition);
> 
> Does this vend the correct column number for script attributes and other things where the start of the source doesn't match the start of the line?   e.g.
> 
> <script>foo()
> </script>
> 
> or <foo onload="blah()">
> ?

It checks for a ‘\n’ or ‘\t’ or ‘\0’.  So, it doesn’t care if you have html tags preceding it.  It truly measures the displacement from the start of the line.  That’s what we want, right?

>> Source/WTF/ChangeLog:18
>> +
> 
> Double changelog :(

Duh!!! Will fix.  Can I get a r+ to land assuming I fix this before landing?

[Oliver Hunt]

(In reply to comment #7)
> (From update of attachment 193980 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193980&action=review
> 
> >> Source/JavaScriptCore/interpreter/Interpreter.cpp:1347
> >> +    size_t lineTerminatorPosition = source.reverseFindLineTerminator(actualCharPosition);
> > 
> > Does this vend the correct column number for script attributes and other things where the start of the source doesn't match the start of the line?   e.g.
> > 
> > <script>foo()
> > </script>
> > 
> > or <foo onload="blah()">
> > ?
> 
> It checks for a ‘\n’ or ‘\t’ or ‘\0’.  So, it doesn’t care if you have html tags preceding it.  It truly measures the displacement from the start of the line.  That’s what we want, right?

'\0' can't appear in the string - how do you ensure that the offset you are recording as the start of a line is the actual start of the line?  Alternatively how do you avoid walking off the start of the source?

[Mark Lam]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 193980 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=193980&action=review
> > 
> > >> Source/JavaScriptCore/interpreter/Interpreter.cpp:1347
> > >> +    size_t lineTerminatorPosition = source.reverseFindLineTerminator(actualCharPosition);
> > > 
> > > Does this vend the correct column number for script attributes and other things where the start of the source doesn't match the start of the line?   e.g.
> > > 
> > > <script>foo()
> > > </script>
> > > 
> > > or <foo onload="blah()">
> > > ?
> > 
> > It checks for a ‘\n’ or ‘\t’ or ‘\0’.  So, it doesn’t care if you have html tags preceding it.  It truly measures the displacement from the start of the line.  That’s what we want, right?
> 
> '\0' can't appear in the string - how do you ensure that the offset you are recording as the start of a line is the actual start of the line?  Alternatively how do you avoid walking off the start of the source?

See reverseFindLineTerminator() in StringImpl.h (also in this patch).  “source” is a WTF::String.  The reverseFind is bounded by the length of the string.  If it runs out of characters, it will return notFound.

[Mark Lam]

> (In reply to comment #8)
> > '\0' can't appear in the string - how do you ensure that the offset you are recording as the start of a line is the actual start of the line?  Alternatively how do you avoid walking off the start of the source?

Regarding ‘\0’, I included in the StringImpl::reverseFindLineTerminator() because it uses an explicitly specified length unlike C strings that requires a ‘\0’ terminator.  I wasn’t sure if we ever allow WTF::String instances that include ‘\0’ in them (though we don’t allow that in our parsed source strings.  Are you saying that we (the webkit project) does not allow that?  If so, I can remove the ‘\0’ check in StringImpl::reverseFindLineTerminator().

[Oliver Hunt]

(In reply to comment #10)
> > (In reply to comment #8)
> > > '\0' can't appear in the string - how do you ensure that the offset you are recording as the start of a line is the actual start of the line?  Alternatively how do you avoid walking off the start of the source?
> 
> Regarding ‘\0’, I included in the StringImpl::reverseFindLineTerminator() because it uses an explicitly specified length unlike C strings that requires a ‘\0’ terminator.  I wasn’t sure if we ever allow WTF::String instances that include ‘\0’ in them (though we don’t allow that in our parsed source strings.  Are you saying that we (the webkit project) does not allow that?  If so, I can remove the ‘\0’ check in StringImpl::reverseFindLineTerminator().

It's probably possible to induce a null in a html string or some such, but in such a case i don't believe the null would be a line delimiter

[Mark Lam]

Created attachment 194000 [details]
patch : addressed Oliver’s feedback.

[WebKit Review Bot]

Attachment 194000 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.order', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCoreExports.def', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCoreExportGenerator/JavaScriptCoreExports.def.in', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h', u'Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/parser/ASTBuilder.h', u'Source/JavaScriptCore/parser/Lexer.cpp', u'Source/JavaScriptCore/parser/Lexer.h', u'Source/JavaScriptCore/parser/NodeConstructors.h', u'Source/JavaScriptCore/parser/Nodes.cpp', u'Source/JavaScriptCore/parser/Nodes.h', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/Parser.h', u'Source/JavaScriptCore/parser/ParserTokens.h', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/text/StringImpl.cpp', u'Source/WTF/wtf/text/StringImpl.h', u'Source/WTF/wtf/text/WTFString.h']" exit_code: 1
Source/JavaScriptCore/parser/Parser.h:975:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Thanks for the review.  Landed in r146318: <http://trac.webkit.org/changeset/146318>.