Bug 93473 – Web Inspector: The front-end should provide the position in original source file when set a breakpoint

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Peter Wang]

I'd like to discuss here, should we provide the the position in original source file when set a breakpoint. For example, a JS statement like this:
[space][space][space]var[space]a=1;
The position in original source file is (0,3), but now front-end send (0,0) when set a breakpoint, the heading spaces are removed.
To accommodate it, JSC has to add code to meet the same rules of removing leading space.

If we just provide the original position of a breakpoint, we can make JS engine and front-end independent, so that when front-end change the Formatter or support feature like "Pretty Print", the JS engine is kept unchanged.

It's my personal suggestion. I'd like to discuss.

[Timothy Hatcher]

Are you just asking about column support for breakpoints? If so we have a bug about that already.

[Peter Wang]

(In reply to comment #1)
> Are you just asking about column support for breakpoints? If so we have a bug about that already.

If you mean  "https://bugs.webkit.org/show_bug.cgi?id=53003", I think I know it. Actually I made a patch for it. JSC can support column for  breakpoints now, but there is a problem left.

Now, when the inspector user set a breakpoint, the front-end of inspector send the position of breakpoint to ScriptDebugServer. Most time the front-end sends the original position of statements in JS file, except for the statements that have leading blank space. In that case the position sent by front-end is taking no account of leading blank space.

I file this bug to discuss can we let front-end to send the original position of breakpoint, so that we don't have to modify the JS engine code to accommodate the front-end formatting style.

[Yury Semikhatsky]

(In reply to comment #0)
> I'd like to discuss here, should we provide the the position in original source file when set a breakpoint. For example, a JS statement like this:
> [space][space][space]var[space]a=1;
> The position in original source file is (0,3), but now front-end send (0,0) when set a breakpoint, the heading spaces are removed.
> To accommodate it, JSC has to add code to meet the same rules of removing leading space.
> 
I'm not sure about JSC but in case of V8 it was that we couldn't break at some statements, e.g. if you set a breakpoint at line "var c = a + b;" it could move to line "f();". The functionality was added for cases like this but it handles setting breakpoint at a space equally well by finding nearest statement.

function f() {}
var a = 10;
var b = Date.now();
var c = a + b;
f();

So basically we could only break at call stubs, branching operators and loops. That was improved some time ago but you should keep in mind that the breakpoint can move not only because of spaces.

[Peter Wang]

(In reply to comment #3)
> (In reply to comment #0)
> > I'd like to discuss here, should we provide the the position in original source file when set a breakpoint. For example, a JS statement like this:
> > [space][space][space]var[space]a=1;
> > The position in original source file is (0,3), but now front-end send (0,0) when set a breakpoint, the heading spaces are removed.
> > To accommodate it, JSC has to add code to meet the same rules of removing leading space.
> > 
> I'm not sure about JSC but in case of V8 it was that we couldn't break at some statements, e.g. if you set a breakpoint at line "var c = a + b;" it could move to line "f();". The functionality was added for cases like this but it handles setting breakpoint at a space equally well by finding nearest statement.
> 
> function f() {}
> var a = 10;
> var b = Date.now();
> var c = a + b;
> f();
> 
> So basically we could only break at call stubs, branching operators and loops. That was improved some time ago but you should keep in mind that the breakpoint can move not only because of spaces.

Thank you for your comments. 
Things of JSC are different. Roughly speaking, JSC checks if there is a breakpoint before executing each statement. Taking this as example:
var a = 10;
[whitespace][whitespace]var b = Date.now();
[whitespace]var c = a + b;
f();
JSC knows the starting positions of every statements. Showing as (line,column), they are (0,0), (1,2), (2,1), and(3,0). JSC use them to match the breakpoint record to make sure if it should stop here. In old time JSC just check the "line" so can not support "Pretty Print" debug. Now JSC also records "column" during parsing. But there is still a problem, for above piece of JS code, if we set breakpoint for every statements, the positions set by front end are (0,0), (1,0), (2,0), and(3,0), since front-end ignores the leading whitespace. So I have to change the mechanism of JSC to follow the way of front-end. I'm just worry that every time the formatter of front-end is changed, JSC must have to follow.
I'm not sure if there is a standard of position of breakpoint. If not, could we just take the original position of statement as the position of breakpoint? So that JS engine and front-end can change their way of dealing with code independently.

[Pavel Feldman]

JSC should be able to "resolve" the breakpoint in order to handle this case. In fact, this will also resolve many other cases such as setting the breakpoint on the line with comments. That's what all debuggers do. It should return the particular statement position it did set the breakpoint on. I do realize that this is an extra work for the JSC debugger, but there is no other way scenarios such as pretty printing, setting breakpoints on comments, etc. would work.

[Peter Wang]

(In reply to comment #5)
> JSC should be able to "resolve" the breakpoint in order to handle this case. In fact, this will also resolve many other cases such as setting the breakpoint on the line with comments. That's what all debuggers do. It should return the particular statement position it did set the breakpoint on. I do realize that this is an extra work for the JSC debugger, but there is no other way scenarios such as pretty printing, setting breakpoints on comments, etc. would work.

Ok. I'll do the work for JSC.
The last question is can we let front-end to send the position of breakpoint as (line, -1) when it's not in "Pretty Print" mode? Since JSC just knows the "accurate" position of current statement, it's a pain for JSC if front-end changes some rules a little, it may cause debugger disable to work. If JSC knows it's not in "Pretty Print" mode, it can just match line number, so if there is a little rule mismatch, at least the basic debugger can work.

[Pavel Feldman]

> Ok. I'll do the work for JSC.
> The last question is can we let front-end to send the position of breakpoint as (line, -1) when it's not in "Pretty Print" mode? Since JSC just knows the "accurate" position of current statement, it's a pain for JSC if front-end changes some rules a little, it may cause debugger disable to work. If JSC knows it's not in "Pretty Print" mode, it can just match line number, so if there is a little rule mismatch, at least the basic debugger can work.

Why would 0 be a problem? You attempt to set a breakpoint at char 0 and JSC returns the actual statement offset it could set the breakpoint on.

[Peter Wang]

(In reply to comment #7)
> > Ok. I'll do the work for JSC.
> > The last question is can we let front-end to send the position of breakpoint as (line, -1) when it's not in "Pretty Print" mode? Since JSC just knows the "accurate" position of current statement, it's a pain for JSC if front-end changes some rules a little, it may cause debugger disable to work. If JSC knows it's not in "Pretty Print" mode, it can just match line number, so if there is a little rule mismatch, at least the basic debugger can work.
> 
> Why would 0 be a problem? You attempt to set a breakpoint at char 0 and JSC returns the actual statement offset it could set the breakpoint on.

No, I don't mean 0 is wrong. I mean if there is a way to let JSC to know the front-end is in "Normal" or "Pretty Print" mode, that will be great. Since in normal mode we don't even to match the "column", because it's always 0.

[Pavel Feldman]

> No, I don't mean 0 is wrong. I mean if there is a way to let JSC to know the front-end is in "Normal" or "Pretty Print" mode, that will be great. Since in normal mode we don't even to match the "column", because it's always 0.

Well, front-end does not really distinguish between modes. It attempts to set breakpoint on 0, backend resolves it and sends the actual location to the front-end.

[Peter Wang]

(In reply to comment #9)
> > No, I don't mean 0 is wrong. I mean if there is a way to let JSC to know the front-end is in "Normal" or "Pretty Print" mode, that will be great. Since in normal mode we don't even to match the "column", because it's always 0.
> 
> Well, front-end does not really distinguish between modes. It attempts to set breakpoint on 0, backend resolves it and sends the actual location to the front-end.

I see. Thx.

[Peter Wang]

Created attachment 161905 [details]
Patch

[Peter Wang]

Created attachment 161964 [details]
Patch

[Peter Wang]

(In reply to comment #12)
> Created an attachment (id=161964) [details]
> Patch

This patch was verified with Qt port, it can pass all the opened DRT test cases of Inspector.

[Yury Semikhatsky]

Comment on attachment 161964 [details]
Patch

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

Can you provide a test for this?

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:144
> +        m_lastExecutedLine = lineNumber;

has* methods should not modify the object, this is why in particular it was marked const.

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:162
> +        // Since frontend truncates the indent, so the first statement in a line must matches the breakpoint (line, 0).

typo: Since frontend truncates the indent, the first statement in a line must match the breakpoint

[Peter Wang]

(In reply to comment #14)
> (From update of attachment 161964 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161964&action=review
> 
> Can you provide a test for this?
Most of the cases in path LayoutTests/inspector/debugger are the cases of JS with indent, since the JS code of them are formatted. 
And LayoutTests/inspector/debugger/script-formatter-breakpoints.html can be used to verify the case of unformatted JS code.
Actually, I've done DRT test for my patch using Qt build. 
 
> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:144
> > +        m_lastExecutedLine = lineNumber;
> 
> has* methods should not modify the object, this is why in particular it was marked const.
ok, I'll put it in another place.

> > Source/WebCore/bindings/js/ScriptDebugServer.cpp:162
> > +        // Since frontend truncates the indent, so the first statement in a line must matches the breakpoint (line, 0).
> 
> typo: Since frontend truncates the indent, the first statement in a line must match the breakpoint
sorry, thx.

[Peter Wang]

Created attachment 166157 [details]
Patch

[Charles Wei]

Comment on attachment 166157 [details]
Patch

commit

[WebKit Review Bot]

Comment on attachment 166157 [details]
Patch

Clearing flags on attachment: 166157

Committed r130615: <http://trac.webkit.org/changeset/130615>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.