Bug 53003

Related to the following: <http://webkit.org/b/52615> Web Inspector: set breakpoints by line:column

Created attachment 143987 [details] Patch

This bug causes Browser with JSC to work wrong in Pretty Print debug mode if the Javascript files put several statements in one line.

Comment on attachment 143987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143987&action=review Looks good, can be improved some more. > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=53003 Better add an empty line here. > Source/WebCore/ChangeLog:5 > + The RIM PR https://bugzilla.qnx.com/bugzilla/show_bug.cgi?id=154675 is depend on this bug. This needs to go below Reviewed by. Also please add an empty line after Reviewed by. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:75 > + return true; I think you can combine below so you only have two returns. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:525 > + m_lastHitScriptBreakpoints.clear(); You are doing same lines of code as in returnEvent. So I think there can be a helper function to avoid duplicating code.

Created attachment 144450 [details] Patch

Attachment 144450 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 144450 [details] Patch Attachment 144450 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12847365

Comment on attachment 144450 [details] Patch Attachment 144450 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12845413

Created attachment 144453 [details] Patch

Comment on attachment 144453 [details] Patch Attachment 144453 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12845416

Comment on attachment 144453 [details] Patch Attachment 144453 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12838508

Comment on attachment 144453 [details] Patch Attachment 144453 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12845418

Comment on attachment 144453 [details] Patch Attachment 144453 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12846382

Comment on attachment 144453 [details] Patch Attachment 144453 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12853146

Created attachment 144524 [details] Patch

(In reply to comment #15) > Created an attachment (id=144524) [details] > Patch To resolve build problem with other platform

Comment on attachment 144524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144524&action=review This looks good to me, but I'd rather have a Inspector person look at this. pfeldman maybe? > Source/WebCore/ChangeLog:6 > + The RIM PR https://bugzilla.qnx.com/bugzilla/show_bug.cgi?id=154675 is depend on this bug. s/is depend/depends > Source/WebCore/ChangeLog:8 > + Reviewed by Rob Buis. You don't want to fill this in until you have r+.

Comment on attachment 144524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144524&action=review > Source/WebCore/bindings/js/ScriptDebugServer.cpp:95 > + unsigned i; Please move it to the for() initialization section: for (size_t i = 0; i < breaksCount; i++) { > Source/WebCore/bindings/js/ScriptDebugServer.cpp:135 > + if (breaksVector.at(i).columnNumber == (int)columnNumber) { We should change types of ScriptBreakpoint.{lolumn,line}Number from int to unsigned if they are always >= 0. Also please use static_cast<int> instead of c-syle cast. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:163 > + unsigned j; Please declare the variables in the corresponding for() sections where they are initialized. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:438 > + const TextPosition position = m_currentCallFrame->position(); What's the point in extracting this into a local variable given that it is used only once in the method? > Source/WebCore/bindings/js/ScriptDebugServer.cpp:446 > + m_currentCallFrame->updatePosition(hitPosition); You shouldn't need to update call frame position from the ScriptDebugServer, VM should provide correct location. > Source/WebCore/bindings/js/ScriptDebugServer.h:137 > + typedef HashMap<Page*, ListenerSet*> PageListenersMap; Revert this line. > Source/WebCore/bindings/js/ScriptDebugServer.h:153 > + mutable BreakpointsInLine m_lastHitScriptBreakpoints; Why do you need these fields to be mutable?

(In reply to comment #18) > (From update of attachment 144524 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144524&action=review > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:95 > > + unsigned i; > > Please move it to the for() initialization section: > for (size_t i = 0; i < breaksCount; i++) { > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:135 > > + if (breaksVector.at(i).columnNumber == (int)columnNumber) { > > We should change types of ScriptBreakpoint.{lolumn,line}Number from int to unsigned if they are always >= 0. Also please use static_cast<int> instead of c-syle cast. > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:163 > > + unsigned j; > > Please declare the variables in the corresponding for() sections where they are initialized. > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:438 > > + const TextPosition position = m_currentCallFrame->position(); > > What's the point in extracting this into a local variable given that it is used only once in the method? sorry, it's a legacy of my old patch, I just neglected it. it will be modified. > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:446 > > + m_currentCallFrame->updatePosition(hitPosition); > > You shouldn't need to update call frame position from the ScriptDebugServer, VM should provide correct location. > Actually, the VM of JSC can not provide correct enough location. It can only provide the line number. If there are several statements in one line, ScriptDebugServer has to "guest" the column and set it in currentCallFrame to make sure send a right position to frontend. > > Source/WebCore/bindings/js/ScriptDebugServer.h:137 > > + typedef HashMap<Page*, ListenerSet*> PageListenersMap; > > Revert this line. Sorry, ok. > > Source/WebCore/bindings/js/ScriptDebugServer.h:153 > > + mutable BreakpointsInLine m_lastHitScriptBreakpoints; > > Why do you need these fields to be mutable? Because I think it's most appropriate to change it in "hasBreakpoint", but "hasBreakpoint" is a const function. I just don't want to break the original design.

Created attachment 145731 [details] Patch

Comment on attachment 145731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145731&action=review > Source/WebCore/bindings/js/ScriptDebugServer.cpp:160 > + unsigned hitBreaksCount = m_lastHitScriptBreakpoints.size(); I don't understand the purpose of m_lastHitScriptBreakpoints - isn't it always empty at this point? > Source/WebCore/bindings/js/ScriptDebugServer.cpp:179 > + m_hitScriptBreakpoint = breaksVector.at(i); Why don't you return m_hitScriptBreakpoint in an output parameter instead of adding a mutable field?

> > > Source/WebCore/bindings/js/ScriptDebugServer.h:153 > > > + mutable BreakpointsInLine m_lastHitScriptBreakpoints; > > > > Why do you need these fields to be mutable? > Because I think it's most appropriate to change it in "hasBreakpoint", but "hasBreakpoint" is a const function. I just don't want to break the original design. hasBreakpoint is used in one place only as far as I can see, doesn't look like a valuable design to keep to me :)

(In reply to comment #22) > > Source/WebCore/bindings/js/ScriptDebugServer.h:153 > >+ mutable BreakpointsInLine m_lastHitScriptBreakpoints; > > Why do you need these fields to be mutable? The function "hasBreakpoint" is a const function, I just don't want to break the original design. Maybe I should just change it as a normal function and avoid "mutable". > hasBreakpoint is used in one place only as far as I can see, doesn't look like a valuable design to keep to me :) Yes, it looks like redundant :). But actually it's the heart of JSC debugger. Since the breakpoint in JSC is "fake" breakpoint. It's not inserted in JS code. If debugger is attached, interpreter just calling ScriptController to check if the breakpoint is reached at current line using "hasBreakpoint". So "hasBreakpoint" is almost invoked by every JS statement interpreting.

Comment on attachment 145731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145731&action=review > Source/WebCore/ChangeLog:6 > + RIM PR https://bugzilla.qnx.com/bugzilla/show_bug.cgi?id=152507 depends on this bug. What is RIM PR? > Source/WebCore/bindings/js/ScriptDebugServer.cpp:99 > + std::sort(breaksVector.begin(), breaksVector.end(), breakpointOrderSortFunction); If breaksVector are breakpoints in the line, why comparing the line numbers in the comparator? >> Source/WebCore/bindings/js/ScriptDebugServer.cpp:160 >> + unsigned hitBreaksCount = m_lastHitScriptBreakpoints.size(); > > I don't understand the purpose of m_lastHitScriptBreakpoints - isn't it always empty at this point? I also can't say I'm following the purpose of the m_lastHitScriptBreakpoints. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:171 > + m_lastHitScriptBreakpoints.append(breaksVector.at(i)); So at first, you will go here and consider that you should stop on breakpoint no matter whether its position matches the breakpoint column? This looks wrong. If you set a breakpoint in the formatted script (which originally was a single javascript line), it will map to say line 1, column 42. Then every statement regardless of the offset will be finding this breakpoint and stopping on it. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:445 > + m_currentCallFrame->updatePosition(hitPosition); This sounds like a hack. You should get someone from the JSC team to bless it.

(In reply to comment #24) > (From update of attachment 145731 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145731&action=review > > > Source/WebCore/ChangeLog:6 > > + RIM PR https://bugzilla.qnx.com/bugzilla/show_bug.cgi?id=152507 depends on this bug. > > What is RIM PR? It's Problem Report of the BlackBerry Browser. Our Browser cannot support "Pretty print" mode of JS debugging because of this bug of webkit. > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:99 > > + std::sort(breaksVector.begin(), breaksVector.end(), breakpointOrderSortFunction); > > If breaksVector are breakpoints in the line, why comparing the line numbers in the comparator? Because the breakpoints in m_lastHitScriptBreakpoints are not always in same line. > >> Source/WebCore/bindings/js/ScriptDebugServer.cpp:160 > >> + unsigned hitBreaksCount = m_lastHitScriptBreakpoints.size(); > > > > I don't understand the purpose of m_lastHitScriptBreakpoints - isn't it always empty at this point? > > I also can't say I'm following the purpose of the m_lastHitScriptBreakpoints. The m_lastHitScriptBreakpoints is used to record recent hit points. By its help we can figure out what breakpoint should be toggled. For example, if there 3 breakpoints in a same line, and the first and second are already hit, then now in this line the third should hit, even the JSC VM only provide the line number, we can figure out accurately which breakpoint should be toggled. > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:171 > > + m_lastHitScriptBreakpoints.append(breaksVector.at(i)); > > So at first, you will go here and consider that you should stop on breakpoint no matter whether its position matches the breakpoint column? This looks wrong. > If you set a breakpoint in the formatted script (which originally was a single javascript line), it will map to say line 1, column 42. Then every statement regardless of the offset will be finding this breakpoint and stopping on it. The information in breaksVector is accurate since it come from Frontend. In the webpage of Inspector the script calculates the breakpoint's position and sends message, finally the setBreakpoints is invoked and save it in breaksVector. The problem is that in hasBreakpoints we only know which line we are. So we using m_lastHitScriptBreakpoints to calculate which breakpoint in this line should be toggled. And this breakpoint also should be recorded as recent toggled breakpoints. > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:445 > > + m_currentCallFrame->updatePosition(hitPosition); > > This sounds like a hack. You should get someone from the JSC team to bless it. You can take m_curerntCallFrame as a copy of callFrame of JSC VM. The Inspector Frontend send the brekpoint information of it to outside. Since JSC VM only provide the line number, the column number is always 0. Here just to set a accurate column number.

Comment on attachment 145731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145731&action=review >>> Source/WebCore/ChangeLog:6 >>> + RIM PR https://bugzilla.qnx.com/bugzilla/show_bug.cgi?id=152507 depends on this bug. >> >> What is RIM PR? > > It's Problem Report of the BlackBerry Browser. Our Browser cannot support "Pretty print" mode of JS debugging because of this bug of webkit. Ok, it sounds like we should disable setting breakpoints on formatted scripts in JSC for now. >>> Source/WebCore/bindings/js/ScriptDebugServer.cpp:171 >>> + m_lastHitScriptBreakpoints.append(breaksVector.at(i)); >> >> So at first, you will go here and consider that you should stop on breakpoint no matter whether its position matches the breakpoint column? This looks wrong. >> If you set a breakpoint in the formatted script (which originally was a single javascript line), it will map to say line 1, column 42. Then every statement regardless of the offset will be finding this breakpoint and stopping on it. > > The information in breaksVector is accurate since it come from Frontend. In the webpage of Inspector the script calculates the breakpoint's position and sends message, finally the setBreakpoints is invoked and save it in breaksVector. > The problem is that in hasBreakpoints we only know which line we are. So we using m_lastHitScriptBreakpoints to calculate which breakpoint in this line should be toggled. And this breakpoint also should be recorded as recent toggled breakpoints. I don't understand how it works though. Consider following JavaScript snippet: var a = 0; var b = 0; var c = a + b; in formatted mode, it is: var a = 0; var b = 0; var c = a + b; <-- now set breakpoint in this line it ends up line = 0; column = 22; JSC starts interpretation of "var a = 0;" at [0, 0] and hasBreakpoint request is made with line 0. You fetch all breakpoints for this line including the one we set above. Then you get to this place of code (since j == hitBreaksCount == 0) and stop on that breakpoint. As a result, instead of stopping on "var c = a + b" you stop on "var a = 0". What do I miss here?

(In reply to comment #26) > (From update of attachment 145731 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145731&action=review > > >>> Source/WebCore/ChangeLog:6 > >>> + RIM PR https://bugzilla.qnx.com/bugzilla/show_bug.cgi?id=152507 depends on this bug. > >> > >> What is RIM PR? > > > > It's Problem Report of the BlackBerry Browser. Our Browser cannot support "Pretty print" mode of JS debugging because of this bug of webkit. > > Ok, it sounds like we should disable setting breakpoints on formatted scripts in JSC for now. > > >>> Source/WebCore/bindings/js/ScriptDebugServer.cpp:171 > >>> + m_lastHitScriptBreakpoints.append(breaksVector.at(i)); > >> > >> So at first, you will go here and consider that you should stop on breakpoint no matter whether its position matches the breakpoint column? This looks wrong. > >> If you set a breakpoint in the formatted script (which originally was a single javascript line), it will map to say line 1, column 42. Then every statement regardless of the offset will be finding this breakpoint and stopping on it. > > > > The information in breaksVector is accurate since it come from Frontend. In the webpage of Inspector the script calculates the breakpoint's position and sends message, finally the setBreakpoints is invoked and save it in breaksVector. > > The problem is that in hasBreakpoints we only know which line we are. So we using m_lastHitScriptBreakpoints to calculate which breakpoint in this line should be toggled. And this breakpoint also should be recorded as recent toggled breakpoints. > > I don't understand how it works though. Consider following JavaScript snippet: > var a = 0; var b = 0; var c = a + b; > > in formatted mode, it is: > var a = 0; > var b = 0; > var c = a + b; <-- now set breakpoint in this line > > it ends up line = 0; column = 22; > > JSC starts interpretation of "var a = 0;" at [0, 0] and hasBreakpoint request is made with line 0. You fetch all breakpoints for this line including the one we set above. Then you get to this place of code (since j == hitBreaksCount == 0) and stop on that breakpoint. As a result, instead of stopping on "var c = a + b" you stop on "var a = 0". > > What do I miss here? Oops, I think I need to correct something. Thx.

Created attachment 149483 [details] Patch

Comment on attachment 149483 [details] Patch Attachment 149483 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13098366

Created attachment 149490 [details] Patch

Comment on attachment 149490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149490&action=review > Source/WebCore/bindings/js/ScriptDebugServer.cpp:136 > + if (m_currentSourceID != sourceID) { Can you explain how that is expected to work in case when you first stop in a script 1, then in a script 2 and again in the same line in script 1? It seems that in that case you will return the first column as the current one or am I missing something? > Source/WebCore/bindings/js/ScriptDebugServer.cpp:157 > + int nextColumn = codeInLine.find(";", m_currentStatementPosition.columnNumber); You shouldn't be trying to parse the code here, instead JSC should be extended to report you current statement's column. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:160 > + if (c == ' ' || c == '\t') What is the purpose of skipping first whitespace?

(In reply to comment #31) > (From update of attachment 149490 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149490&action=review > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:136 > > + if (m_currentSourceID != sourceID) { > > Can you explain how that is expected to work in case when you first stop in a script 1, then in a script 2 and again in the same line in script 1? It seems that in that case you will return the first column as the current one or am I missing something? > The sourceID represents the code block of current callframe, if code block is changed, then it must starts from the first line. You can take this call stack as example: Interpreter::debug(...) debugger->callEvent(..., callFrame->codeBlock()->ownerExecutable()->sourceID(),...); ScriptDebugServer::createCallFrameAndPauseIfNeeded(... ,sourceID, ...); updateCurrentStatementPosition(sourceID); > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:157 > > + int nextColumn = codeInLine.find(";", m_currentStatementPosition.columnNumber); > > You shouldn't be trying to parse the code here, instead JSC should be extended to report you current statement's column. > It's a very big change to make JSC to provide current statement's column. Since JSC doesn't really insert break point, it just call interface of Debugger at entrance of Function or code-block. And actually, the Debugger has enough info to calculate it. > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:160 > > + if (c == ' ' || c == '\t') > > What is the purpose of skipping first whitespace? For example, the column of second statement in "var a = 1;[white apace][white apace]...[white apace]var b=1;", the formatter of forntend assign it as 11.

sorry. For example, the column of second statement in "var a = 1;[white Space][white space]...[white space]var b=1;", the formatter of forntend assign it as 11.

(In reply to comment #32) > (In reply to comment #31) > > (From update of attachment 149490 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=149490&action=review > > > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:136 > > > + if (m_currentSourceID != sourceID) { > > > > Can you explain how that is expected to work in case when you first stop in a script 1, then in a script 2 and again in the same line in script 1? It seems that in that case you will return the first column as the current one or am I missing something? > > > The sourceID represents the code block of current callframe, if code block is changed, then it must starts from the first line. sourceID is a pointer to the corresponding SourceProvider which is shared between several functions/code blocks. > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:157 > > > + int nextColumn = codeInLine.find(";", m_currentStatementPosition.columnNumber); > > > > You shouldn't be trying to parse the code here, instead JSC should be extended to report you current statement's column. > > > It's a very big change to make JSC to provide current statement's column. Since JSC doesn't really insert break point, it just call interface of Debugger at entrance of Function or code-block. > And actually, the Debugger has enough info to calculate it. I doubt it. E.g. how can you tell in ScriptDebugServer which column is going to be next in the following statement: "if (foo()) { bar(); } else { baz(); }" ? > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:160 > > > + if (c == ' ' || c == '\t') > > > > What is the purpose of skipping first whitespace? > For example, the column of second statement in "var a = 1;[white apace][white apace]...[white apace]var b=1;", the formatter of forntend assign it as 11. I still don't understand that, we skip first whitespace and the statement now starts with 10 spaces?

(In reply to comment #34) > (In reply to comment #32) > > (In reply to comment #31) > > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:157 > > > > + int nextColumn = codeInLine.find(";", m_currentStatementPosition.columnNumber); > > > > > > You shouldn't be trying to parse the code here, instead JSC should be extended to report you current statement's column. > > > > > It's a very big change to make JSC to provide current statement's column. Since JSC doesn't really insert break point, it just call interface of Debugger at entrance of Function or code-block. > > And actually, the Debugger has enough info to calculate it. > I doubt it. E.g. how can you tell in ScriptDebugServer which column is going to be next in the following statement: "if (foo()) { bar(); } else { baz(); }" ? Yes, my code is failed with that case. I thought JSC will take code inside curved parentheses as a code-block. I'm wrong :(. Maybe, it's impossible to solve it keeping JSC unchanged?

> > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:157 > > > + int nextColumn = codeInLine.find(";", m_currentStatementPosition.columnNumber);(In reply to comment #35) > > I doubt it. E.g. how can you tell in ScriptDebugServer which column is going to be next in the following statement: "if (foo()) { bar(); } else { baz(); }" ? > Yes, my code is failed with that case. I thought JSC will take code inside curved parentheses as a code-block. I'm wrong :(. Maybe, it's impossible to solve it keeping JSC unchanged? How about this code: var a = "a string; <- with a semicolon";

(In reply to comment #36) > > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:157 > > > > + int nextColumn = codeInLine.find(";", m_currentStatementPosition.columnNumber);(In reply to comment #35) > > > > I doubt it. E.g. how can you tell in ScriptDebugServer which column is going to be next in the following statement: "if (foo()) { bar(); } else { baz(); }" ? > > Yes, my code is failed with that case. I thought JSC will take code inside curved parentheses as a code-block. I'm wrong :(. Maybe, it's impossible to solve it keeping JSC unchanged? > > How about this code: > var a = "a string; <- with a semicolon"; Yes, I see. Thx. Maybe, let JSC rather than Debugger to provide "column" is the only right way. I have to investigate JSC...

Created attachment 150358 [details] Patch

Attachment 150358 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 150359 [details] Patch

Comment on attachment 150359 [details] Patch Attachment 150359 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13129063

Created attachment 150369 [details] Patch

Comment on attachment 150369 [details] Patch Attachment 150369 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13129078

Created attachment 150376 [details] Patch

Comment on attachment 150376 [details] Patch Attachment 150376 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13118995

Created attachment 150382 [details] Patch

Comment on attachment 150382 [details] Patch Attachment 150382 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13136006

Created attachment 150385 [details] Patch

The main modifications of this patch are: (1) Add "m_columnNumber" to JSC::Lexer (Lexer.cpp) as a counter of column. (2) Add "m_columnNumber" to JSC::Node (Nodes.cpp) to record the column of this Node. (3) Add "column" for instruction "op_debug" (BytecodeGenerator.cpp).

I tried compiling the proposed patch on ToT (r121930) and died with the compilation error below. Looks like some other code needs to have a column parameter added. I was building to check performance implications of the patch. In file included from /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebFrame.mm:51: /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebScriptDebugger.h:57:18: error: 'WebScriptDebugger::callEvent' hides overloaded virtual function [-Werror,-Woverloaded-virtual] virtual void callEvent(const JSC::DebuggerCallFrame&, intptr_t sourceID, int lineNumber); ^ /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Debugger.h:47:22: note: hidden overloaded virtual function 'JSC::Debugger::callEvent' declared here virtual void callEvent(const DebuggerCallFrame&, intptr_t, int, int) { }; ^ In file included from /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebFrame.mm:51: /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebScriptDebugger.h:58:18: error: 'WebScriptDebugger::atStatement' hides overloaded virtual function [-Werror,-Woverloaded-virtual] virtual void atStatement(const JSC::DebuggerCallFrame&, intptr_t sourceID, int lineNumber); ^ /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Debugger.h:46:22: note: hidden overloaded virtual function 'JSC::Debugger::atStatement' declared here virtual void atStatement(const DebuggerCallFrame&, intptr_t, int, int) { }; ^ In file included from /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebFrame.mm:51: /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebScriptDebugger.h:59:18: error: 'WebScriptDebugger::returnEvent' hides overloaded virtual function [-Werror,-Woverloaded-virtual] virtual void returnEvent(const JSC::DebuggerCallFrame&, intptr_t sourceID, int lineNumber); ^ /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Debugger.h:48:22: note: hidden overloaded virtual function 'JSC::Debugger::returnEvent' declared here virtual void returnEvent(const DebuggerCallFrame&, intptr_t, int, int) { }; ^ In file included from /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebFrame.mm:51: /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebScriptDebugger.h:60:18: error: 'WebScriptDebugger::exception' hides overloaded virtual function [-Werror,-Woverloaded-virtual] virtual void exception(const JSC::DebuggerCallFrame&, intptr_t sourceID, int lineNumber, bool hasHandler); ^ /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Debugger.h:45:22: note: hidden overloaded virtual function 'JSC::Debugger::exception' declared here virtual void exception(const DebuggerCallFrame&, intptr_t, int, int, bool) { }; ^ In file included from /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebFrame.mm:51: /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebScriptDebugger.h:61:18: error: 'WebScriptDebugger::willExecuteProgram' hides overloaded virtual function [-Werror,-Woverloaded-virtual] virtual void willExecuteProgram(const JSC::DebuggerCallFrame&, intptr_t sourceID, int lineno); ^ /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Debugger.h:50:22: note: hidden overloaded virtual function 'JSC::Debugger::willExecuteProgram' declared here virtual void willExecuteProgram(const DebuggerCallFrame&, intptr_t, int, int) { }; ^ In file included from /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebFrame.mm:51: /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebScriptDebugger.h:62:18: error: 'WebScriptDebugger::didExecuteProgram' hides overloaded virtual function [-Werror,-Woverloaded-virtual] virtual void didExecuteProgram(const JSC::DebuggerCallFrame&, intptr_t sourceID, int lineno); ^ /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Debugger.h:51:22: note: hidden overloaded virtual function 'JSC::Debugger::didExecuteProgram' declared here virtual void didExecuteProgram(const DebuggerCallFrame&, intptr_t, int, int) { }; ^ In file included from /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebFrame.mm:51: /Volumes/Data/src/webkit.work/Source/WebKit/mac/WebView/WebScriptDebugger.h:63:18: error: 'WebScriptDebugger::didReachBreakpoint' hides overloaded virtual function [-Werror,-Woverloaded-virtual] virtual void didReachBreakpoint(const JSC::DebuggerCallFrame&, intptr_t sourceID, int lineno); ^ /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/Debugger.h:52:22: note: hidden overloaded virtual function 'JSC::Debugger::didReachBreakpoint' declared here virtual void didReachBreakpoint(const DebuggerCallFrame&, intptr_t, int, int) { };

Created attachment 151028 [details] Patch

(In reply to comment #51) > Created an attachment (id=151028) [details] > Patch Hi Michael, this patch can help your porting to avoid to be broken. I've just added a series of old interface for Debugger to make it compatible with the code that use it.

In theory, my patch brings extra calculation only in "parsing" phase, since it adds a counter to record the column of "token", and this extra calculation is a very small part of the calculation of "parsing". For the generated bytecode, only instruction "debug" has one extra parameter. With my local compiled Qt Browser, I did some times of SunSpider test. The results are fluctuating, below data are the ones with narrowest fluctuating range: with my patch 208.8ms ±0.8% without my patch 209.6ms ±0.6% The feature of debugging with uglified JS code is very important now, since most JS code in live sites are uglified. I tried to implement this feature avoiding modification of JSC, but failed. It seems let JSC provide the "column" info is appropriate, since in "parsing" phase JSC "knows" the "column" info of every token, it just needs to be recorded. I'd like to discuss the potential impact of my patch, since we must be careful when changing JSC. Welcome comments.

Comment on attachment 151028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151028&action=review Overall approach in JSC looks good. One comment below. > Source/JavaScriptCore/parser/Parser.cpp:1639 > + expr = context.makePostfixNode(m_lexer->lastLineNumber(), tokenColumn(), expr, OpPlusPlus, subExprStart, lastTokenEnd(), tokenEnd()); It's awkward to get the line number from the lexer, but get the column number from the current token. Let's get all the data from the current token instead. The smallest change you can make to fix this is to pass a reference to the current token instead of two separate numbers. It would be even better to rename JSTokenInfo to TokenLocation, put line, column, start, end, and divot all in TokenLocation, and pass just that one object to all clients.

Created attachment 153432 [details] Patch

(In reply to comment #54) > (From update of attachment 151028 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151028&action=review > > Overall approach in JSC looks good. One comment below. > > > Source/JavaScriptCore/parser/Parser.cpp:1639 > > + expr = context.makePostfixNode(m_lexer->lastLineNumber(), tokenColumn(), expr, OpPlusPlus, subExprStart, lastTokenEnd(), tokenEnd()); > > It's awkward to get the line number from the lexer, but get the column number from the current token. Let's get all the data from the current token instead. > > The smallest change you can make to fix this is to pass a reference to the current token instead of two separate numbers. > > It would be even better to rename JSTokenInfo to TokenLocation, put line, column, start, end, and divot all in TokenLocation, and pass just that one object to all clients. Thank you for your comments. I've updated a new patch according to your comments. There is only one exception that I didn't include divot in TokenLocation, since it will bring a lot of more modifications. I really want to keep the modification and risk of side-effect minimum, if the modification is acceptable.

Created attachment 155774 [details] Patch

(In reply to comment #57) > Created an attachment (id=155774) [details] > Patch update patch to catch up the newest code.

Comment on attachment 155774 [details] Patch r=me

Comment on attachment 155774 [details] Patch Rejecting attachment 155774 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 48>At revision 134927. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13411701

Created attachment 155964 [details] Patch

Comment on attachment 155964 [details] Patch commit.

Comment on attachment 155964 [details] Patch Clearing flags on attachment: 155964 Committed r124406: <http://trac.webkit.org/changeset/124406>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 92951

This has set the bots on fire: http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r124406%20(1500)/results.html

(In reply to comment #63) > (From update of attachment 155964 [details]) > Clearing flags on attachment: 155964 > > Committed r124406: <http://trac.webkit.org/changeset/124406> Please also change http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp?rev=124406#L5257 when landing again.

Created attachment 156045 [details] Patch

(In reply to comment #68) > Created an attachment (id=156045) [details] > Patch What does this do to fix the crashes in the previous patch?

(In reply to comment #57) > Created an attachment (id=155774) [details] > Patch update patch to catch up the newest code. (In reply to comment #69) > (In reply to comment #68) > > Created an attachment (id=156045) [details] [details] > > Patch > > What does this do to fix the crashes in the previous patch? I need to verify on my local Mac build again. So far, I think the reason is "JSC::Debugger", in my patch the interfaces like "callEvent" of "JSC::Debugger" are added one parameter. The class of Mac porting WebScriptDebugger derived from "JSC::Debugger" and override these interfaces. I missed the Mac porting, I should at least changed the interfaces of WebScriptDebugger and test.

Created attachment 156329 [details] Patch

(In reply to comment #71) > Created an attachment (id=156329) [details] > Patch Comparing to the patch which caused problem for Mac porting, there are two modifications: (1) Add a new parameter to interfaces "callEvent", "atStatement" ... of WebScriptDebugger, since WebScriptDebugger of Mac porting is derived from "JSC::Debugger" and these interfaces of "JSC::Debugger" were changed. (2) Change the argument number of "_llint_op_debug" form 4 to 5, since debug byte-code command was added a new argument. I'm so sorry, my local porting doesn't support LLINT so I missed the "_llint_op_debug".

Comment on attachment 156329 [details] Patch Commit it after the author had run the test against Mac build without regression.

Comment on attachment 156329 [details] Patch Rejecting attachment 156329 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/13434011

Created attachment 156577 [details] Patch

Comment on attachment 156577 [details] Patch Clearing flags on attachment: 156577 Committed r124729: <http://trac.webkit.org/changeset/124729>

All reviewed patches have been landed. Closing bug.

Appears that this may have broken the Mac Lion Debug bots: inspector/debugger/script-formatter-breakpoints.html inspector/debugger/pause-in-internal-script.html inspector/debugger/watch-expressions-panel-switch.html inspector/debugger/debugger-activation-crash2.html inspector/debugger/debugger-set-breakpoint-regex.html