Bug 140269 – Breakpoint doesn't fire in this HTML5 game

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

* SUMMARY
No JS breakpoints hit in this HTML5 game

* STEPS TO REPRODUCE
1. Go to http://goldenminer.org/#
2. Set a breakpoint on g.js line 268, the first line after "doMine: function() {"

* RESULTS
Breakpoint was not hit.

rdar://problem/15113345

[Michael Saboff]

Created attachment 244305 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 244305 [details]
Patch

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

Test looks good to me with nits. I don't think I'm qualified to review the parser change.

> LayoutTests/inspector/debugger/breakpoint-columns.html:41
> +        // Create the breakpoint and its actions before sending anything to the backend.

Nit: Unnecessary and incorrect comment (no actions).

> LayoutTests/inspector/debugger/breakpoint-columns.html:43
> +        breakpoint.autoContinue = false;

Nit: This is the default value, you should not need to set it, this shouldn't be doing anything.

> LayoutTests/inspector/debugger/breakpoint-columns.html:53
> +    WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.CallFramesDidChange,
> +function(event) {

Style: You should put the "function(event) {" on the previous line.

> LayoutTests/inspector/debugger/breakpoint-columns.html:58
> +        if (WebInspector.debuggerManager.activeCallFrame) {
> +            var stopLocation = "line: " +
> +                WebInspector.debuggerManager.activeCallFrame.sourceCodeLocation.lineNumber +
> +                ", column: " +
> +                WebInspector.debuggerManager.activeCallFrame.sourceCodeLocation.columnNumber;

This would read much better storing activeCallFrame into a local. Also, I think this test will now work if you just early return if there is no call frame.

    var activeCallFrame = WebInspector.debuggerManager.activeCallFrame;
    if (!activeCallFrame)
        return;

    var stopLocation = "line: " + activeCallFrame.sourceCodeLocation.lineNumber + "m column: " activeCallFrame.sourceCodeLocation.columnNumber;
    ...
    WebInspector.debuggerManager.resume();

> LayoutTests/inspector/debugger/breakpoint-columns.html:61
> +            InspectorTest.evaluateInPage("console.log('Paused at " + stopLocation + "')")

Style: Since you use semicolon's elsewhere, you may want a semicolon at the end of this statement.

> LayoutTests/inspector/debugger/breakpoint-columns.html:68
> +    WebInspector.debuggerManager.addEventListener(WebInspector.DebuggerManager.Event.Resumed,
> +function(event) {

Style: You should put the "function(event) {" on the previous line.

> LayoutTests/inspector/debugger/breakpoint-columns.html:93
> +    <p>Testing that breakpoints can be set at various line / column combinations'.</p>

Unexpected single quote.

[Michael Saboff]

Created attachment 244309 [details]
Patch with reviewer's suggested changes.

[Geoffrey Garen]

Comment on attachment 244309 [details]
Patch with reviewer's suggested changes.

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

> Source/JavaScriptCore/ChangeLog:8
> +        When parsing a single line cached function, use the lineStartOffset of the

Why does this only matter in the single line function case?

[Mark Lam]

Comment on attachment 244309 [details]
Patch with reviewer's suggested changes.

r=me.

[Michael Saboff]

(In reply to comment #4)
> Comment on attachment 244309 [details]
> Patch with reviewer's suggested changes.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244309&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        When parsing a single line cached function, use the lineStartOffset of the
> 
> Why does this only matter in the single line function case?

The lineStartOffset is used to compute the column.  It appears that lineStartOffset has two different meanings.

For multiline functions, it is the offset in the source string for the current line.  

For single line functions, it is the offset of the "{" of the next outer most enclosing function.  That is the case that we are fixing here.

[Mark Lam]

(In reply to comment #6)
> (In reply to comment #4)
> > > Source/JavaScriptCore/ChangeLog:8
> > > +        When parsing a single line cached function, use the lineStartOffset of the
> > 
> > Why does this only matter in the single line function case?
> 
> The lineStartOffset is used to compute the column.  It appears that
> lineStartOffset has two different meanings.
> 
> For multiline functions, it is the offset in the source string for the
> current line.  
> 
> For single line functions, it is the offset of the "{" of the next outer
> most enclosing function.  That is the case that we are fixing here.

No, this explanation is incorrect.  There is only 1 meaning to lineStartOffset.  After thinking thru this some more, I think the solution may be wrong too.  Details below:

The Single Line case
====================
Test source code:
function columnTest1(){var x=function x(){setInterval(function y(){})};(function z(){console.log("column test 1")})()}

The parser passes (in Parser::parseFunctionInfo()) for this code goes as follows as the code gets executed:

Pass 1: Parsing the global program
  SourceCode starts at startChar 0 (base 0), line 1 (base 1), column 1 (base 1).

  1. Scan over function columnTest1:
     - Function is NOT found in parser cache. 
     - Body start OpenParen is at location (line = 1, lineStartOffset = 0, startOffset = 22, endOffset = 23).
     *** Note: the lineStartOffset is at 0 because the SourceCode startChar is 0. ***

  2. Scan over function x:
     - Function is NOT found in parser cache. 
     - Body start OpenParen is at location (line = 1, lineStartOffset = 0, startOffset = 41, endOffset = 42).
     - Function body is added to the parser cache.
     *** Note: the end of the function (not shown here) should have lineStartOffset of 0 (based on SourceCode
               startChar) because the whole function is on 1 line. ***

  3. Scan over function y: This function is not relevant to this discussion.

  4. Scan over function z:
     - Function is NOT found in parser cache. 
     - Body start OpenParen is at location (line = 1, lineStartOffset = 0, startOffset = 84, endOffset = 85).
     - Function body is added to the parser cache.

Pass 2: Parsing columnTest1.
  SourceCode starts at startChar 22 (base 0), line 1 (base 1), column 23 (base 1). 
  *** Note: the startChar is 22 because that’s where the body of columnTest1 starts (see above).

  5. Scan over function x:
     - Function is FOUND in parser cache, and will be parsed for compilation from the cached source string. 
     - Actual Body start OpenParen is at location (line = 1, lineStartOffset = 22, startOffset = 41, endOffset = 42).
     *** Note: the actual lineStartOffset of this function is 22 because it’s SourceCode startChar is 22, not 0.
               However, we’ll be parsing the cached source string which has a lineStartOffset of 0.

               Similarly, we would expect the actual lineStartOffset of the CloseParen to be 22.
               But because we’re actually parsing the cached source string, the lexer token info at the end
               will have a lineStartOffset of 0.

     After scanning over function x based on the cached string, we need to update the lexer to point to last token
     so that we can continue parsing:

         m_lexer->setOffset(m_token.m_location.endOffset, m_token.m_location.lineStartOffset);

     Unfortunately, that lineStartOffset of that last token is from the cached source string (i.e. 0) when we expect
     it to be for the current source string (i.e. 22).  This is the bug that is being fixed by setting the token
     lineStartOffset to the starting lineStartOffset before we setOffset() on the lexer.

     But why don’t we do this when the source code is multi-line?  Well, see below ...


The Multi-Line case
===================
Test source code:
// ...
// ...
function columnTest1() { // line 3
    var x=function x() { // line 4
        setInterval(function y(){}) // line 5
    }; // end of function at line 6, column 5
    (function z() { // line 7
        console.log("column test 1")
    })()
} // line 10

function columnTest2() { // line 12
    var r, s, t, u, v, w, x=function x() { // line 13
        setInterval(function y(){}) // line 14
    }; // end of function at line 15, column 5
}

The parser passes (in Parser::parseFunctionInfo()) for this code goes as follows as the code gets executed:

Pass 1: Parsing the global program
  SourceCode starts at startChar 0 (base 0), line 1 (base 1), column 1 (base 1).

  1. Scan over function columnTest1:
     - Body start OpenParen is at location (line = 3, lineStartOffset = 123, startOffset = 146, endOffset = 147).
     *** Note: the lineStartOffset doesn’t start at 0 because it is on a different line from the start of the source (line 3).

  2. Scan over function x:
     - Body start OpenParen is at location (line = 4, lineStartOffset = 148, startOffset = 171, endOffset = 1712).
     *** Note: the lineStartOffset also doesn’t start at 0 because it is on a different line from the start of the source (line 4).
               Similarly, we would expect the lineStartOffset of the end of the function (line 6) to not be 0 as well.

  3. Scan over function y: This function is not relevant to this discussion.

  4. Scan over function z:
     - Body start OpenParen is at location (line = 7, lineStartOffset = 216, startOffset = 234, endOffset = 235).

Pass 2: Parsing columnTest1.
  SourceCode starts at startChar 146 (base 0), line 3 (base 1), column 24 (base 1). 
  *** Note: the startChar is 146 because that’s where the body of columnTest1 starts on line 3.

  5. Scan over function x:
     - Function is FOUND in parser cache, and will be parsed for compilation from the cached source string. 
     - Actual Body start OpenParen is at location (line = 4, lineStartOffset = 148, startOffset = 171, endOffset = 172).

     *** Note: the lineStartOffset of the end of the function does not depend on the first line of the function
               because, by definition, they are on different lines.

               But does this mean we don’t have to adjust the lineStartOffset here?  No, see below ...

Consider columnTest2 which also has a function x which has a body that is identical to the one in columnTest1.  However, because this function x body is located at a different place in the source string, it follows that the body end lineStartOffset must necessarily be different than that of the one in the parser cache (which is based on the columnTest1 function x).  It should be fixed up based on where its body start lineStartOffset is.

Hence, the proper fix should instead be:

        unsigned currentLineStartOffset = m_token.m_location.lineStartOffset;

        ...

        if (endColumnIsOnStartLine)
            m_token.m_location.lineStartOffset = currentLineStartOffset;
        else
            m_token.m_location.lineStartOffset += (currentLineStartOffset - cachedInfo->openBraceLineStartOffset);

Note: cachedInfo is of type SourceProviderCacheItem* here, and SourceProviderCacheItem::openBraceLineStartOffset does not currently exist.  We’ll need to add it where the cachedInfo is initialized.

[Mark Lam]

Comment on attachment 244309 [details]
Patch with reviewer's suggested changes.

In light of the data I’ve just provided, we know that this patch is incorrect.  Hence, I’m reversing my r+.

[Mark Lam]

(In reply to comment #7)
...
> The Multi-Line case
> ===================
> Test source code:
> // ...
> // ...
> function columnTest1() { // line 3
>     var x=function x() { // line 4
>         setInterval(function y(){}) // line 5
>     }; // end of function at line 6, column 5
>     (function z() { // line 7
>         console.log("column test 1")
>     })()
> } // line 10
> 
> function columnTest2() { // line 12
>     var r, s, t, u, v, w, x=function x() { // line 13
>         setInterval(function y(){}) // line 14
>     }; // end of function at line 15, column 5
> }
...
> Consider columnTest2 which also has a function x which has a body that is
> identical to the one in columnTest1.

Some corrections:
1. I added the line number comments in order to help us understand the differences.  In order for the test to work, we cannot have the line number comments there because they will make the 2 x functions different and not use the same cached info.

2. I deliberately have more characters before the start of the columnTest2 x’s “{".  This difference is ok.  It is the content from the “{" onwards that must be the same up till the “}”. 

3. In comment #7, I also made the error of using the OpenParen location as the start of the function body.  That is not correct.  It is the OpenBrace location that starts the function body.  However, since the 2 always have the same lineStartOffset in these examples, the examples are still correct in terms of the body start lineStartOffset values that we would expect to see.

[Michael Saboff]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > > Source/JavaScriptCore/ChangeLog:8

> Hence, the proper fix should instead be:
> 
>         unsigned currentLineStartOffset = m_token.m_location.lineStartOffset;
> 
>         ...
> 
>         if (endColumnIsOnStartLine)
>             m_token.m_location.lineStartOffset = currentLineStartOffset;
>         else
>             m_token.m_location.lineStartOffset += (currentLineStartOffset -
> cachedInfo->openBraceLineStartOffset);
> 
> Note: cachedInfo is of type SourceProviderCacheItem* here, and
> SourceProviderCacheItem::openBraceLineStartOffset does not currently exist. 
> We’ll need to add it where the cachedInfo is initialized.

I implemented the suggested changes.  I got the open paren's lineStartOffset from the startLocation.  Where the SourceProviderCacheItemCreationParameters struct is created I have, using the newly added field:

      parameters.openBraceLineStartOffset = startLocation.lineStartOffset;

These suggested changes caused a crash in a debug build in the following layout tests
    webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html  
    js/regress/nested-function-parsing.html    
    js/slow-stress/nested-function-parsing-random.html 
    compositing/culling/unscrolled-within-boxshadow.html
    compositing/culling/scrolled-within-boxshadow.html

All similar to:
Process:               DumpRenderTree [74439]
Path:                  /Volumes/VOLUME/*/DumpRenderTree
Identifier:            DumpRenderTree
Version:               0
Code Type:             X86-64 (Native)
Parent Process:        Python [52192]
Responsible:           Terminal [379]
User ID:               501

Date/Time:             2015-01-09 10:35:27.575 -0800
OS Version:            Mac OS X 10.11 (15A64)
Report Version:        11
Anonymous UUID:        D41B71B4-1D4F-DCC9-DE79-773C9742C2B3

Sleep/Wake UUID:       A4F62134-B26B-4BC0-85CA-EA39A6BC4AEB

Time Awake Since Boot: 120000 seconds
Time Since Wake:       3800 seconds

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x00000000bbadbeef

VM Regions Near 0xbbadbeef:
--> 
    __TEXT                 0000000104634000-00000001046c9000 [  596K] r-x/rwx SM=COW  /Volumes/VOLUME/*

Application Specific Information:
CRASHING TEST: webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000001052c64e7 WTFCrash + 39
1   com.apple.JavaScriptCore      	0x00000001050552dc JSC::Lexer<unsigned char>::setOffset(int, int) + 188
2   com.apple.JavaScriptCore      	0x000000010516086b bool JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionInfo<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::FunctionRequirements, JSC::FunctionParseMode, bool, JSC::Identifier const*&, JSC::ASTBuilder::FormalParameterList&, JSC::ASTBuilder::FunctionBody&, unsigned int&, unsigned int&, int&, unsigned int&) + 4795
3   com.apple.JavaScriptCore      	0x000000010515e466 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 486
4   com.apple.JavaScriptCore      	0x000000010515d35b JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 1051
5   com.apple.JavaScriptCore      	0x000000010515c62a JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 186
6   com.apple.JavaScriptCore      	0x000000010515bc5c JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 60
7   com.apple.JavaScriptCore      	0x000000010515b17e JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 590
8   com.apple.JavaScriptCore      	0x0000000105162ae4 JSC::ASTBuilder::Arguments JSC::Parser<JSC::Lexer<unsigned char> >::parseArguments<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Parser<JSC::Lexer<unsigned char> >::SpreadMode) + 1300
9   com.apple.JavaScriptCore      	0x000000010515ec09 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 2441
10  com.apple.JavaScriptCore      	0x000000010515d35b JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 1051
11  com.apple.JavaScriptCore      	0x000000010515c62a JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 186
12  com.apple.JavaScriptCore      	0x000000010515bc5c JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 60
13  com.apple.JavaScriptCore      	0x000000010515b17e JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 590
14  com.apple.JavaScriptCore      	0x000000010515a921 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) + 177
15  com.apple.JavaScriptCore      	0x0000000105159d0a JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseExpressionOrLabelStatement<JSC::ASTBuilder>(JSC::ASTBuilder&) + 138
16  com.apple.JavaScriptCore      	0x0000000105151916 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) + 998
17  com.apple.JavaScriptCore      	0x00000001050f9bac JSC::ASTBuilder::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::SourceElementsMode) + 108
18  com.apple.JavaScriptCore      	0x0000000105152297 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseBlockStatement<JSC::ASTBuilder>(JSC::ASTBuilder&) + 327
19  com.apple.JavaScriptCore      	0x00000001051516c5 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) + 405
20  com.apple.JavaScriptCore      	0x00000001050f9bac JSC::ASTBuilder::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::SourceElementsMode) + 108
21  com.apple.JavaScriptCore      	0x00000001050f9588 JSC::Parser<JSC::Lexer<unsigned char> >::parseInner() + 232
22  com.apple.JavaScriptCore      	0x000000010525a7e5 std::__1::unique_ptr<JSC::FunctionNode, std::__1::default_delete<JSC::FunctionNode> > JSC::Parser<JSC::Lexer<unsigned char> >::parse<JSC::FunctionNode>(JSC::ParserError&, bool) + 405
23  com.apple.JavaScriptCore      	0x0000000105259a83 std::__1::unique_ptr<JSC::FunctionNode, std::__1::default_delete<JSC::FunctionNode> > JSC::parse<JSC::FunctionNode>(JSC::VM*, JSC::SourceCode const&, JSC::FunctionParameters*, JSC::Identifier const&, JSC::JSParserStrictness, JSC::JSParserMode, JSC::ParserError&, JSC::JSTextPosition*, bool) + 403
24  com.apple.JavaScriptCore      	0x0000000105253ed5 JSC::generateFunctionCodeBlock(JSC::VM&, JSC::UnlinkedFunctionExecutable*, JSC::SourceCode const&, JSC::CodeSpecializationKind, JSC::DebuggerMode, JSC::ProfilerMode, JSC::UnlinkedFunctionKind, bool, JSC::ParserError&) + 325
25  com.apple.JavaScriptCore      	0x0000000105253c16 JSC::UnlinkedFunctionExecutable::codeBlockFor(JSC::VM&, JSC::SourceCode const&, JSC::CodeSpecializationKind, JSC::DebuggerMode, JSC::ProfilerMode, bool, JSC::ParserError&) + 374
26  com.apple.JavaScriptCore      	0x0000000104d71093 JSC::ScriptExecutable::newCodeBlockFor(JSC::CodeSpecializationKind, JSC::JSFunction*, JSC::JSScope**, JSC::JSObject*&) + 1731
27  com.apple.JavaScriptCore      	0x0000000104d717de JSC::ScriptExecutable::prepareForExecutionImpl(JSC::ExecState*, JSC::JSFunction*, JSC::JSScope**, JSC::CodeSpecializationKind) + 110
28  com.apple.JavaScriptCore      	0x00000001049a24f5 JSC::ScriptExecutable::prepareForExecution(JSC::ExecState*, JSC::JSFunction*, JSC::JSScope**, JSC::CodeSpecializationKind) + 101
29  com.apple.JavaScriptCore      	0x0000000105083951 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 305
30  com.apple.JavaScriptCore      	0x0000000105083806 JSC::LLInt::genericCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind) + 230
31  com.apple.JavaScriptCore      	0x000000010508008c llint_slow_path_call + 60
32  com.apple.JavaScriptCore      	0x000000010508cffb llint_entry + 25615
33  ???                           	0x00002f1545e01a3a 0 + 51768413133370
34  com.apple.JavaScriptCore      	0x000000010508d1f7 llint_entry + 26123
35  com.apple.JavaScriptCore      	0x000000010508d00a llint_entry + 25630
36  com.apple.JavaScriptCore      	0x000000010508d00a llint_entry + 25630
37  com.apple.JavaScriptCore      	0x00000001050869a9 vmEntryToJavaScript + 361
38  com.apple.JavaScriptCore      	0x0000000104f13ddc JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 252
39  com.apple.JavaScriptCore      	0x0000000104ef87ff JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1487
40  com.apple.JavaScriptCore      	0x0000000104a0a45e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 190
41  com.apple.JavaScriptCore      	0x0000000104a0a4c3 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, JSC::JSValue*) + 83
42  com.apple.WebCore             	0x0000000109978e8b WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, JSC::JSValue*) + 107 (JSMainThreadExecState.h:56)
43  com.apple.WebCore             	0x0000000109af4bae WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 1278 (JSEventListener.cpp:127)
44  com.apple.WebCore             	0x00000001092f3c74 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&) + 1444 (EventTarget.cpp:255)
45  com.apple.WebCore             	0x00000001092f34ae WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 334 (EventTarget.cpp:207)
46  com.apple.WebCore             	0x000000010921bf4b WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>) + 539 (DOMWindow.cpp:1897)
47  com.apple.WebCore             	0x00000001092238a5 WebCore::DOMWindow::dispatchLoadEvent() + 293 (DOMWindow.cpp:1855)
48  com.apple.WebCore             	0x00000001090feb8d WebCore::Document::dispatchWindowLoadEvent() + 141 (Document.cpp:3789)
49  com.apple.WebCore             	0x00000001090fc0cd WebCore::Document::implicitClose() + 557 (Document.cpp:2432)
50  com.apple.WebCore             	0x000000010945283b WebCore::FrameLoader::checkCallImplicitClose() + 155 (FrameLoader.cpp:910)
51  com.apple.WebCore             	0x000000010945250e WebCore::FrameLoader::checkCompleted() + 270 (FrameLoader.cpp:857)
52  com.apple.WebCore             	0x0000000109451012 WebCore::FrameLoader::finishedParsing() + 178 (FrameLoader.cpp:777)
53  com.apple.WebCore             	0x00000001091087e3 WebCore::Document::finishedParsing() + 483 (Document.cpp:4605)
54  com.apple.WebCore             	0x00000001095a29f8 WebCore::HTMLConstructionSite::finishedParsing() + 24 (HTMLConstructionSite.cpp:405)
55  com.apple.WebCore             	0x00000001096d7f3a WebCore::HTMLTreeBuilder::finished() + 186 (HTMLTreeBuilder.cpp:2943)
56  com.apple.WebCore             	0x00000001095d0b9e WebCore::HTMLDocumentParser::end() + 190 (HTMLDocumentParser.cpp:426)
57  com.apple.WebCore             	0x00000001095cef5e WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd() + 254 (HTMLDocumentParser.cpp:435)
58  com.apple.WebCore             	0x00000001095cecfe WebCore::HTMLDocumentParser::prepareToStopParsing() + 270 (HTMLDocumentParser.cpp:154)
59  com.apple.WebCore             	0x00000001095d0bef WebCore::HTMLDocumentParser::attemptToEnd() + 63 (HTMLDocumentParser.cpp:447)
60  com.apple.WebCore             	0x00000001095d0c48 WebCore::HTMLDocumentParser::finish() + 72 (HTMLDocumentParser.cpp:475)
61  com.apple.WebCore             	0x0000000109189b3c WebCore::DocumentWriter::end() + 332 (DocumentWriter.cpp:247)
62  com.apple.WebCore             	0x0000000109152036 WebCore::DocumentLoader::finishedLoading(double) + 1574 (DocumentLoader.cpp:441)
63  com.apple.WebCore             	0x000000010915197e WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource*) + 270 (DocumentLoader.cpp:375)
64  com.apple.WebCore             	0x0000000108db2952 WebCore::CachedResource::checkNotify() + 130 (CachedResource.cpp:293)
65  com.apple.WebCore             	0x0000000108db2a61 WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*) + 49 (CachedResource.cpp:311)
66  com.apple.WebCore             	0x0000000108dae53a WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 218 (CachedRawResource.cpp:105)
67  com.apple.WebCore             	0x000000010a85b0d5 WebCore::SubresourceLoader::didFinishLoading(double) + 581 (SubresourceLoader.cpp:357)
68  com.apple.WebCore             	0x000000010a56fcc5 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 53 (ResourceLoader.cpp:507)
69  com.apple.WebCore             	0x000000010aafabea -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 186 (WebCoreResourceHandleAsDelegate.mm:261)
70  com.apple.CFNetwork           	0x00007fff90ba059d __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke + 69
71  com.apple.CFNetwork           	0x00007fff90ba0401 -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] + 232
72  com.apple.CFNetwork           	0x00007fff90ba0307 -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] + 48
73  com.apple.CFNetwork           	0x00007fff90ba1314 ___ZN27URLConnectionClient_Classic26_delegate_didFinishLoadingEU13block_pointerFvvE_block_invoke + 104
74  com.apple.CFNetwork           	0x00007fff90c6442d ___ZN27URLConnectionClient_Classic18_withDelegateAsyncEPKcU13block_pointerFvP16_CFURLConnectionPK33CFURLConnectionClientCurrent_VMaxE_block_invoke_2 + 94
75  libdispatch.dylib             	0x00007fff93700958 _dispatch_block_invoke + 457
76  com.apple.CFNetwork           	0x00007fff90afd620 RunloopBlockContext::_invoke_block(void const*, void*) + 24
77  com.apple.CoreFoundation      	0x00007fff8a462734 CFArrayApplyFunction + 68
78  com.apple.CFNetwork           	0x00007fff90afd513 RunloopBlockContext::perform() + 133
79  com.apple.CFNetwork           	0x00007fff90afd2f8 MultiplexerSource::perform() + 282
80  com.apple.CFNetwork           	0x00007fff90afd11a MultiplexerSource::_perform(void*) + 72
81  com.apple.CoreFoundation      	0x00007fff8a496371 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
82  com.apple.CoreFoundation      	0x00007fff8a4888fd __CFRunLoopDoSources0 + 269
83  com.apple.CoreFoundation      	0x00007fff8a487f1f __CFRunLoopRun + 927
84  com.apple.CoreFoundation      	0x00007fff8a487918 CFRunLoopRunSpecific + 296
85  DumpRenderTree                	0x0000000104651124 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 5396
86  DumpRenderTree                	0x000000010464fbaa runTestingServerLoop() + 330
87  DumpRenderTree                	0x000000010464f360 dumpRenderTree(int, char const**) + 448
88  DumpRenderTree                	0x00000001046519e6 DumpRenderTreeMain(int, char const**) + 102
89  DumpRenderTree                	0x00000001046a17a2 main + 34
90  libdyld.dylib                 	0x00007fff908d95c9 start + 1

[Mark Lam]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > > Source/JavaScriptCore/ChangeLog:8
> > > > +        When parsing a single line cached function, use the lineStartOffset of the
> > > 
> > > Why does this only matter in the single line function case?
> > 
> > The lineStartOffset is used to compute the column.  It appears that
> > lineStartOffset has two different meanings.
> > 
> > For multiline functions, it is the offset in the source string for the
> > current line.  
> > 
> > For single line functions, it is the offset of the "{" of the next outer
> > most enclosing function.  That is the case that we are fixing here.
> 
> No, this explanation is incorrect.  There is only 1 meaning to
> lineStartOffset.  

Sorry, my tone came off harsher than I intended there.  But it is not true that there are 2 different meanings to the lineStartOffset.  The definition of the lineStartOffset is the offset / character position in the SourceCode string for the start of the current line a.k.a column 0 of the current line.  Michael's "multiline function" definition is correct.  For the single line function case which you observed, the only reason that it is the offset of the "{" is because that "{" is at start (column 0) of the current one and only line in the source code was being inspected.

> After thinking thru this some more, I think the solution
> may be wrong too.

I was wrong on this part.  Michael's fix is correct.  I had confused about the parser's functionCache with the CodeCache's CodeCacheMap used for reusing UnlinkedCodeBlocks.  With the parser's functionCache, the scenario of multiple functions reusing the same SourceProviderCacheItem does not exist.  Sorry for the noise.

For the ChangeLog comment, given Geoff's inquiry, I think perhaps it needs more details on why there is a difference between the single line and multi line scenarios.  How about something like this?

"Consider the following single line program:
function outer(){ function inner1() { doStuff(); }; (function inner2() { doMoreStuff()l })()}

The first time we parse the program, the SourceCode start character is at the start of the program.  On this parser pass, we will cache functions inner1() and inner2() with a lineStartOffset that is at the start of the program.

Subsequently, when we parse function outer(), the SourceCode start character is at the start of outer()'s open brace.  While parsing the body of outer(), we encounter inner1() and will attempt to skip over it by getting its closeBrace position from the parser's function cache.  Unfortunately, the cached closeBrace position will have a lineStartOffset at the start of the program instead of the start of SourceCode start character where the open brace is.  Hence, if the cached function is a single line function, we need to set its close brace lineStartOffset to that of its open brace.

We don't need to do this for multi line cached functions because the close brace is guaranteed to be on a different line than the open brace.  Hence, its lineStartOffset will not change with the change of the SourceCode start character."

Would that work?

[Mark Lam]

Comment on attachment 244309 [details]
Patch with reviewer's suggested changes.

r=me with ChangeLog revised.

[Michael Saboff]

(In reply to comment #11)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #4)
> > > > > Source/JavaScriptCore/ChangeLog:8
> > > > > +        When parsing a single line cached function, use the lineStartOffset of the
> > > > 
> > > > Why does this only matter in the single line function case?
> > > 
> > > The lineStartOffset is used to compute the column.  It appears that
> > > lineStartOffset has two different meanings.
> > > 
> > > For multiline functions, it is the offset in the source string for the
> > > current line.  
> > > 
> > > For single line functions, it is the offset of the "{" of the next outer
> > > most enclosing function.  That is the case that we are fixing here.
> > 
> > No, this explanation is incorrect.  There is only 1 meaning to
> > lineStartOffset.  
> 
> Sorry, my tone came off harsher than I intended there.  But it is not true
> that there are 2 different meanings to the lineStartOffset.  The definition
> of the lineStartOffset is the offset / character position in the SourceCode
> string for the start of the current line a.k.a column 0 of the current line.
> Michael's "multiline function" definition is correct.  For the single line
> function case which you observed, the only reason that it is the offset of
> the "{" is because that "{" is at start (column 0) of the current one and
> only line in the source code was being inspected.
> 
> > After thinking thru this some more, I think the solution
> > may be wrong too.
> 
> I was wrong on this part.  Michael's fix is correct.  I had confused about
> the parser's functionCache with the CodeCache's CodeCacheMap used for
> reusing UnlinkedCodeBlocks.  With the parser's functionCache, the scenario
> of multiple functions reusing the same SourceProviderCacheItem does not
> exist.  Sorry for the noise.
> 
> For the ChangeLog comment, given Geoff's inquiry, I think perhaps it needs
> more details on why there is a difference between the single line and multi
> line scenarios.  How about something like this?
> 
> "Consider the following single line program:
> function outer(){ function inner1() { doStuff(); }; (function inner2() {
> doMoreStuff()l })()}
> 
> The first time we parse the program, the SourceCode start character is at
> the start of the program.  On this parser pass, we will cache functions
> inner1() and inner2() with a lineStartOffset that is at the start of the
> program.
> 
> Subsequently, when we parse function outer(), the SourceCode start character
> is at the start of outer()'s open brace.  While parsing the body of outer(),
> we encounter inner1() and will attempt to skip over it by getting its
> closeBrace position from the parser's function cache.  Unfortunately, the
> cached closeBrace position will have a lineStartOffset at the start of the
> program instead of the start of SourceCode start character where the open
> brace is.  Hence, if the cached function is a single line function, we need
> to set its close brace lineStartOffset to that of its open brace.
> 
> We don't need to do this for multi line cached functions because the close
> brace is guaranteed to be on a different line than the open brace.  Hence,
> its lineStartOffset will not change with the change of the SourceCode start
> character."
> 
> Would that work?

I'll add this or something similar to the change log.

[Michael Saboff]

Committed r178232: <http://trac.webkit.org/changeset/178232>

[David Kilzer (:ddkilzer)]

> For a multi-line function, the close brace is guaranteed to be on a different line
> than the open brace. Hence, its lineStartOffset will not change with the change of
> the SourceCode start character

So this isn't valid JavaScript?

function f()
{ return 1; }

[Michael Saboff]

(In reply to comment #15)
> > For a multi-line function, the close brace is guaranteed to be on a different line
> > than the open brace. Hence, its lineStartOffset will not change with the change of
> > the SourceCode start character
> 
> So this isn't valid JavaScript?
> 
> function f()
> { return 1; }

Of course it is.  The case being handled here is a function declared inside another function.  The relevant braces are those of the outer function.

Example

function foo()
{ // <-- This opening brace
    function f()
    { return 1; }

    return f;
} // <-- This closing brace