| Summary: | Breakpoint doesn't fire in this HTML5 game | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||
| Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ddkilzer, joepeck, mark.lam, timothy | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Michael Saboff
2015-01-08 15:07:16 PST
Created attachment 244305 [details]
Patch
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. Created attachment 244309 [details]
Patch with reviewer's suggested changes.
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? Comment on attachment 244309 [details]
Patch with reviewer's suggested changes.
r=me.
(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. (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. 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+.
(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. (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 (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? Comment on attachment 244309 [details]
Patch with reviewer's suggested changes.
r=me with ChangeLog revised.
(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. Committed r178232: <http://trac.webkit.org/changeset/178232> > 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; }
(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 |