Bug 121214 – Keep sourceID and position info in DebuggerCallFrame instead of the Inspector's JavaScriptCallFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Currently, our debugger callbacks explicitly passes sourceID and position (line and column) info to the Inspector along with the DebuggerCallFrame.  This info (the DebuggerCallFrame, sourceID, and position) is then stored in the Inspector's JavaScriptCallFrame.  Technically, we can just get this info from the DebuggerCallFrame.  There's no need to explicitly pass the sourceID and position info to the Inspector and have it stored in its JavaScriptCallFrame.

For the current implementation, JSC is not getting line and column info from the stack yet.  Instead the line and column number are explicitly passed in as arguments to the debug hooks.  For now, we'll continue to use the debug hook line and column info.  To do this, we'll store the values in the DebuggerCallFrame.  At a later date, we should have the DebuggerCallFrame get the info directly from the CallFrame based on its bytecodeOffset.

This refactoring is part of the work to move ScriptDebugServer functionality  into JSC.

[Mark Lam]

Created attachment 211403 [details]
the patch.

This patch has run the layout tests with no new failures.

[Oliver Hunt]

Comment on attachment 211403 [details]
the patch.

Can't we simply kill debuggercallframe and rely on the stack walking instead?  The removes (essentially) a duplicate type, and saves computing/storing line and column info unnecessarily.

[Mark Lam]

(In reply to comment #2)
> (From update of attachment 211403 [details])
> Can't we simply kill debuggercallframe and rely on the stack walking instead?  The removes (essentially) a duplicate type, and saves computing/storing line and column info unnecessarily.

One step at a time.  I did this refactor because I found it to help organize the data in a way that allows me to move ScriptDebugServer functionality into JSC.  I'm trying to stay focus on changing the Inspector to debugger interface first.  I'll change the internal implementation of the debugger later after I've changed the interface to allow it.  This is a stepping stone to get there.

[Geoffrey Garen]

> Can't we simply kill debuggercallframe and rely on the stack walking instead?

I think DebuggerCallFrame has one good feature: it distinguishes standard data in CallFrame from data that is only present when a stack frame has been put in debugger mode. For example, I think the debugger should get a "fake" activation, and not the real thing. We could accomplish that through something like CallFrame::debuggerActivation(), and not have two types. But I think it might be valuable to have a separate type for this interface. DebuggerCallFrame implies that everything is in a debuggable state, which will diverge from normal state over time. It also clarifies who relies on debug info.

[Geoffrey Garen]

Comment on attachment 211403 [details]
the patch.

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

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:659
> +        DebuggerCallFrame debuggerCallFrame(callFrame, line, 0, exceptionValue);

Why not provide a real column number here?

[Mark Lam]

(In reply to comment #5)
> (From update of attachment 211403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211403&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/interpreter/Interpreter.cpp:659
> > +        DebuggerCallFrame debuggerCallFrame(callFrame, line, 0, exceptionValue);
> 
> Why not provide a real column number here?

I was just keeping parity with the pre-existing code, but it should be easy to provide a real column number there.  I'll change it and run it thru the layout tests again.

[Mark Lam]

Thanks for the review.  Added the change to use the real column number in Interpreter::unwind().  Passes layout tests with no new failures.

Landed in r155622: <http://trac.webkit.org/r155622>.