Bug 53003 - Web Inspector: [JSC] implement setting breakpoints by line:column
Summary: Web Inspector: [JSC] implement setting breakpoints by line:column
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 92951 93228
Blocks: 93387
  Show dependency treegraph
 
Reported: 2011-01-24 05:19 PST by Pavel Podivilov
Modified: 2012-08-07 13:18 PDT (History)
25 users (show)

See Also:


Attachments
Patch (11.62 KB, patch)
2012-05-24 23:30 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (11.69 KB, patch)
2012-05-28 23:06 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (11.70 KB, patch)
2012-05-28 23:19 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (11.62 KB, patch)
2012-05-29 04:53 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (12.48 KB, patch)
2012-06-05 01:55 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (10.00 KB, patch)
2012-06-26 01:31 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (10.01 KB, patch)
2012-06-26 02:09 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (298.67 KB, patch)
2012-07-01 23:04 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (298.69 KB, patch)
2012-07-01 23:16 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (298.83 KB, patch)
2012-07-02 00:23 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (299.58 KB, patch)
2012-07-02 01:01 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (299.58 KB, patch)
2012-07-02 02:12 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (299.34 KB, patch)
2012-07-02 02:39 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (299.62 KB, patch)
2012-07-06 00:56 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (308.16 KB, patch)
2012-07-20 00:28 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (307.43 KB, patch)
2012-08-01 03:45 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (307.44 KB, patch)
2012-08-01 20:55 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (314.13 KB, patch)
2012-08-02 05:24 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (316.96 KB, patch)
2012-08-03 04:58 PDT, Peter Wang
no flags Details | Formatted Diff | Diff
Patch (316.97 KB, patch)
2012-08-05 17:42 PDT, Peter Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Podivilov 2011-01-24 05:19:39 PST
Web Inspector: [JSC] implement setting breakpoints by line:column
Comment 1 Joseph Pecoraro 2011-01-24 08:24:59 PST
Related to the following:
<http://webkit.org/b/52615> Web Inspector: set breakpoints by line:column
Comment 2 Peter Wang 2012-05-24 23:30:21 PDT
Created attachment 143987 [details]
Patch
Comment 3 Peter Wang 2012-05-25 00:01:35 PDT
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 4 Rob Buis 2012-05-28 12:49:59 PDT
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.
Comment 5 Peter Wang 2012-05-28 23:06:30 PDT
Created attachment 144450 [details]
Patch
Comment 6 WebKit Review Bot 2012-05-28 23:08:26 PDT
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 7 Build Bot 2012-05-28 23:18:32 PDT
Comment on attachment 144450 [details]
Patch

Attachment 144450 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12847365
Comment 8 Early Warning System Bot 2012-05-28 23:18:37 PDT
Comment on attachment 144450 [details]
Patch

Attachment 144450 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12845413
Comment 9 Peter Wang 2012-05-28 23:19:36 PDT
Created attachment 144453 [details]
Patch
Comment 10 Build Bot 2012-05-28 23:24:32 PDT
Comment on attachment 144453 [details]
Patch

Attachment 144453 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12845416
Comment 11 Early Warning System Bot 2012-05-28 23:28:34 PDT
Comment on attachment 144453 [details]
Patch

Attachment 144453 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12838508
Comment 12 Build Bot 2012-05-28 23:31:10 PDT
Comment on attachment 144453 [details]
Patch

Attachment 144453 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12845418
Comment 13 Gyuyoung Kim 2012-05-28 23:36:29 PDT
Comment on attachment 144453 [details]
Patch

Attachment 144453 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12846382
Comment 14 Early Warning System Bot 2012-05-28 23:39:05 PDT
Comment on attachment 144453 [details]
Patch

Attachment 144453 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12853146
Comment 15 Peter Wang 2012-05-29 04:53:18 PDT
Created attachment 144524 [details]
Patch
Comment 16 Peter Wang 2012-05-29 04:54:50 PDT
(In reply to comment #15)
> Created an attachment (id=144524) [details]
> Patch

To resolve build problem with other platform
Comment 17 Rob Buis 2012-05-30 18:51:45 PDT
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 18 Yury Semikhatsky 2012-06-01 03:11:04 PDT
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?
Comment 19 Peter Wang 2012-06-04 03:32:43 PDT
(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.
Comment 20 Peter Wang 2012-06-05 01:55:12 PDT
Created attachment 145731 [details]
Patch
Comment 21 Vsevolod Vlasov 2012-06-06 13:04:42 PDT
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?
Comment 22 Vsevolod Vlasov 2012-06-06 13:06:06 PDT
> > > 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 :)
Comment 23 Peter Wang 2012-06-08 04:39:05 PDT
(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 24 Pavel Feldman 2012-06-09 07:17:37 PDT
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.
Comment 25 Peter Wang 2012-06-10 21:17:53 PDT
(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 26 Pavel Feldman 2012-06-11 01:03:14 PDT
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?
Comment 27 Peter Wang 2012-06-11 18:22:24 PDT
(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.
Comment 28 Peter Wang 2012-06-26 01:31:13 PDT
Created attachment 149483 [details]
Patch
Comment 29 Build Bot 2012-06-26 01:40:01 PDT
Comment on attachment 149483 [details]
Patch

Attachment 149483 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13098366
Comment 30 Peter Wang 2012-06-26 02:09:30 PDT
Created attachment 149490 [details]
Patch
Comment 31 Yury Semikhatsky 2012-06-26 05:02:08 PDT
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?
Comment 32 Peter Wang 2012-06-26 05:49:26 PDT
(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.
Comment 33 Peter Wang 2012-06-26 05:52:37 PDT
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.
Comment 34 Yury Semikhatsky 2012-06-26 06:21:35 PDT
(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?
Comment 35 Peter Wang 2012-06-26 21:05:31 PDT
(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?
Comment 36 Andrey Adaikin 2012-06-26 21:12:11 PDT
> > > 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";
Comment 37 Peter Wang 2012-06-26 23:11:29 PDT
(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...
Comment 38 Peter Wang 2012-07-01 23:04:52 PDT
Created attachment 150358 [details]
Patch
Comment 39 WebKit Review Bot 2012-07-01 23:06:47 PDT
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.
Comment 40 Peter Wang 2012-07-01 23:16:46 PDT
Created attachment 150359 [details]
Patch
Comment 41 Build Bot 2012-07-01 23:45:33 PDT
Comment on attachment 150359 [details]
Patch

Attachment 150359 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13129063
Comment 42 Peter Wang 2012-07-02 00:23:39 PDT
Created attachment 150369 [details]
Patch
Comment 43 Build Bot 2012-07-02 00:41:00 PDT
Comment on attachment 150369 [details]
Patch

Attachment 150369 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13129078
Comment 44 Peter Wang 2012-07-02 01:01:58 PDT
Created attachment 150376 [details]
Patch
Comment 45 Build Bot 2012-07-02 01:47:02 PDT
Comment on attachment 150376 [details]
Patch

Attachment 150376 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13118995
Comment 46 Peter Wang 2012-07-02 02:12:05 PDT
Created attachment 150382 [details]
Patch
Comment 47 Build Bot 2012-07-02 02:28:14 PDT
Comment on attachment 150382 [details]
Patch

Attachment 150382 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13136006
Comment 48 Peter Wang 2012-07-02 02:39:41 PDT
Created attachment 150385 [details]
Patch
Comment 49 Peter Wang 2012-07-02 03:37:01 PDT
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).
Comment 50 Michael Saboff 2012-07-05 18:13:33 PDT
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) { };
Comment 51 Peter Wang 2012-07-06 00:56:51 PDT
Created attachment 151028 [details]
Patch
Comment 52 Peter Wang 2012-07-06 01:18:55 PDT
(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.
Comment 53 Peter Wang 2012-07-15 00:45:42 PDT
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 54 Geoffrey Garen 2012-07-16 12:47:25 PDT
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.
Comment 55 Peter Wang 2012-07-20 00:28:41 PDT
Created attachment 153432 [details]
Patch
Comment 56 Peter Wang 2012-07-20 02:43:52 PDT
(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.
Comment 57 Peter Wang 2012-08-01 03:45:08 PDT
Created attachment 155774 [details]
Patch
Comment 58 Peter Wang 2012-08-01 04:51:16 PDT
(In reply to comment #57)
> Created an attachment (id=155774) [details]
> Patch

update patch to catch up the newest code.
Comment 59 Geoffrey Garen 2012-08-01 11:58:31 PDT
Comment on attachment 155774 [details]
Patch

r=me
Comment 60 WebKit Review Bot 2012-08-01 19:31:56 PDT
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
Comment 61 Peter Wang 2012-08-01 20:55:37 PDT
Created attachment 155964 [details]
Patch
Comment 62 Charles Wei 2012-08-01 21:05:25 PDT
Comment on attachment 155964 [details]
Patch

commit.
Comment 63 WebKit Review Bot 2012-08-01 21:49:43 PDT
Comment on attachment 155964 [details]
Patch

Clearing flags on attachment: 155964

Committed r124406: <http://trac.webkit.org/changeset/124406>
Comment 64 WebKit Review Bot 2012-08-01 21:49:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 65 WebKit Review Bot 2012-08-01 23:39:56 PDT
Re-opened since this is blocked by 92951
Comment 67 Patrick R. Gansterer 2012-08-02 00:01:11 PDT
(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.
Comment 68 Peter Wang 2012-08-02 05:24:22 PDT
Created attachment 156045 [details]
Patch
Comment 69 Filip Pizlo 2012-08-02 10:59:44 PDT
(In reply to comment #68)
> Created an attachment (id=156045) [details]
> Patch

What does this do to fix the crashes in the previous patch?
Comment 70 Peter Wang 2012-08-02 20:40:38 PDT
(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.
Comment 71 Peter Wang 2012-08-03 04:58:16 PDT
Created attachment 156329 [details]
Patch
Comment 72 Peter Wang 2012-08-03 05:09:22 PDT
(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 73 Charles Wei 2012-08-03 06:22:34 PDT
Comment on attachment 156329 [details]
Patch

Commit it after the author had run the test against Mac build without regression.
Comment 74 WebKit Review Bot 2012-08-03 06:26:09 PDT
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
Comment 75 Peter Wang 2012-08-05 17:42:57 PDT
Created attachment 156577 [details]
Patch
Comment 76 WebKit Review Bot 2012-08-05 20:17:03 PDT
Comment on attachment 156577 [details]
Patch

Clearing flags on attachment: 156577

Committed r124729: <http://trac.webkit.org/changeset/124729>
Comment 77 WebKit Review Bot 2012-08-05 20:17:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 78 Dean Jackson 2012-08-07 13:12:44 PDT
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