Bug 112741 - JSC: Fix incorrect debugger column number value
Summary: JSC: Fix incorrect debugger column number value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-19 13:52 PDT by Mark Lam
Modified: 2013-03-20 02:12 PDT (History)
11 users (show)

See Also:


Attachments
the patch (deleted)
2013-03-19 20:29 PDT, Mark Lam
buildbot: commit-queue-
Details | Formatted Diff | Diff
new patch with the obsolete refs removed from the export list files and added a missing ChangeLog comment. (76.51 KB, patch)
2013-03-19 21:31 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch : addressed Oliver’s feedback. (76.01 KB, patch)
2013-03-20 01:44 PDT, Mark Lam
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-03-19 13:52:59 PDT
The column number for function block entries can be erroneous.  Will fix.

ref: <rdar://problem/13277445>.
Comment 1 Mark Lam 2013-03-19 20:29:00 PDT
Created attachment 193974 [details]
the patch
Comment 2 WebKit Review Bot 2013-03-19 20:33:05 PDT
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.
Comment 3 Build Bot 2013-03-19 21:16:29 PDT
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
Comment 4 Mark Lam 2013-03-19 21:31:23 PDT
Created attachment 193980 [details]
new patch with the obsolete refs removed from the export list files and added a missing ChangeLog comment.
Comment 5 WebKit Review Bot 2013-03-19 21:33:23 PDT
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.
Comment 6 Oliver Hunt 2013-03-20 00:56:51 PDT
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
> +2013-03-19  Mark Lam  <mark.lam@apple.com>
> +
> +        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 :(
Comment 7 Mark Lam 2013-03-20 01:04:26 PDT
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?
Comment 8 Oliver Hunt 2013-03-20 01:14:14 PDT
(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?
Comment 9 Mark Lam 2013-03-20 01:23:04 PDT
(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.
Comment 10 Mark Lam 2013-03-20 01:27:05 PDT
> (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().
Comment 11 Oliver Hunt 2013-03-20 01:37:04 PDT
(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
Comment 12 Mark Lam 2013-03-20 01:44:34 PDT
Created attachment 194000 [details]
patch : addressed Oliver’s feedback.
Comment 13 WebKit Review Bot 2013-03-20 01:49:04 PDT
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.
Comment 14 Mark Lam 2013-03-20 02:12:33 PDT
Thanks for the review.  Landed in r146318: <http://trac.webkit.org/changeset/146318>.