Web Inspector [JSC]: Implement ScriptCallStack::stackTrace

[Pavel Feldman]

// Returns false if there is no running JavaScript or if fetching the stack failed.
    // Sets stackTrace to be an array of stack frame objects.
    // A stack frame object looks like:
    // {
    //   scriptName: <file name for the associated script resource>
    //   functionName: <name of the JavaScript function>
    //   lineNumber: <1 based line number>
    //   column: <1 based column offset on the line>
    // }
    static bool stackTrace(int frameLimit, ScriptState* state, ScriptArray& stackTrace);

This is used by Timeline records.

[Timothy Hatcher]

This is something V8 has, that we need to make JSC have.

[Michael Schneider]

What is the status of this bug? I assume the missing implementation is now in WebKit/Source/WebCore/bindings/js/ScriptCallStackFactory.cpp, createScriptCallStack(size_t, bool)?

I was looking into this a bit and hoped that the following implementation would work:

PassRefPtr<ScriptCallStack> createScriptCallStack(size_t maxStackSize, bool)
{
    JSC::ExecState* exec = JSMainThreadExecState::currentState();
    if (exec == 0)
      return 0;
    return createScriptCallStack(exec, maxStackSize);
}

Unfortunately, JSMainThreadExecState::currentState() tracks the entry points in to JavaScript only, thus we see the bottom of the stack only, which is empty. Given the current use of currentState(), I assume that JSMainThreadExecState::currentState() might as well return the top stack frame without breaking existing clients. Also, I think it would be fair for the currentState() method's contract to state that the return value is the top-most frame in case we left the context of the script engine only. While in the context of JSC, we have an ExecState handy anyways and can generate a stack trace from that, if needed; currentState() would only be required to return any frame of the current stak in that case, which could still be the bottom frame. If updating the currentState() is not feasible for some reason, the current frame could be stored in JSGlobalData or another JSMainThreadExecState structure.

Currently, I see three options to achieve this:
1) Track the exit points: Is the assumption correct that leaving JSC is only possible via implementations of ExecutableBase? If so, these implementations could be used as a guard to update JSMainThreadExecState::currentState().
2) Track the current stack frame whenever code is executed with a new stack frame. It seems as if there are not too may places in the code which do this. This could, however, add a speed-penalty to the execution.
3) Make the ExecState a double linked list, which could be used to walk to the top of the stack if required. However, I'm not sure if this would work for JITed code, and it does not sound like a good idea anyways :)

Is there another, better way to achieve this? @ggaren: can you estimate how much work it would be to provide a patch for this?

[Geoffrey Garen]

> Currently, I see three options to achieve this:
> 1) Track the exit points: Is the assumption correct that leaving JSC is only possible via implementations of ExecutableBase? If so, these implementations could be used as a guard to update JSMainThreadExecState::currentState().

Not quite. ExecutableBase is just the data structure that represents executable code. Entering and exiting executable code is managed by the JIT and the interpreter.

> 2) Track the current stack frame whenever code is executed with a new stack frame. It seems as if there are not too may places in the code which do this. This could, however, add a speed-penalty to the execution.

Indeed, this is a known performance regression.

> 3) Make the ExecState a double linked list, which could be used to walk to the top of the stack if required. However, I'm not sure if this would work for JITed code, and it does not sound like a good idea anyways :)

This just begs the original question, since you need a pointer to the head of the list for this to work for you.

I think the right way to do this is for the JIT and Interpreter to track entry and exit points into the VM, but not track intermediate call frame changes that happen within the VM.

> @ggaren: can you estimate how much work it would be to provide a patch for this?

For someone unfamiliar with the code, my guess is a few weeks to a month.

[Michael Schneider]

Created attachment 100721 [details]
Diff of changes done to track exit points in code generated out of IDL files.

(In reply to comment #3)

> This just begs the original question, since you need a pointer to the head of the list for this to work for you.

I would assume one could start walking from WebCore's JSMainThreadExecState::currentState(). Not a JSC feature, but what I would expect to be correct for the timeline.

> I think the right way to do this is for the JIT and Interpreter to track entry and exit points into the VM, but not track intermediate call frame changes that happen within the VM.

I started looking around in the JSC code, but then it occurred to me that a simpler, WebCore only solution could do it: Currently, JSMainThreadExecState tracks selected entry points to JSC. The exit points are defined by IDL, so why not logging the ExecState in the code generated out of these definitions? I tested this (for now just function calls and custom property getters/setters). I used it with the timeline and got reasonable stack traces with it.

@ggaren: Is this a feasible way of doing this? By doing so I can avoid introducing a thread global state and  runtime overhead to the JSC. I guess the question is how many exit points I actually covered by this change and how hard it is to cover the rest. Please let me know what you think of this approach, so that I can either refine it - or start working on a thread state patch for JSC, if it turns out that this approach is bad.

[Oliver Hunt]

(In reply to comment #4)
> Created an attachment (id=100721) [details]
> Diff of changes done to track exit points in code generated out of IDL files.
> 
> (In reply to comment #3)
> 
> > This just begs the original question, since you need a pointer to the head of the list for this to work for you.
> 
> I would assume one could start walking from WebCore's JSMainThreadExecState::currentState(). Not a JSC feature, but what I would expect to be correct for the timeline.
> 
> > I think the right way to do this is for the JIT and Interpreter to track entry and exit points into the VM, but not track intermediate call frame changes that happen within the VM.
> 
> I started looking around in the JSC code, but then it occurred to me that a simpler, WebCore only solution could do it: Currently, JSMainThreadExecState tracks selected entry points to JSC. The exit points are defined by IDL, so why not logging the ExecState in the code generated out of these definitions? I tested this (for now just function calls and custom property getters/setters). I used it with the timeline and got reasonable stack traces with it.
> 
> @ggaren: Is this a feasible way of doing this? By doing so I can avoid introducing a thread global state and  runtime overhead to the JSC. I guess the question is how many exit points I actually covered by this change and how hard it is to cover the rest. Please let me know what you think of this approach, so that I can either refine it - or start working on a thread state patch for JSC, if it turns out that this approach is bad.

We're working on implementing Error.stack that will introduce logic to JSC to allow it to provide correct stack traces at all times, across arbitrary boundaries.  This will eventually lead to dropping the JSC-inspectors current mechanism for tracking callframes, etc when debugging, and just generally lead to a better life for all.

[Michael Schneider]

(In reply to comment #5)
> We're working on implementing Error.stack that will introduce logic to JSC to allow it to provide correct stack traces at all times, across arbitrary boundaries.  This will eventually lead to dropping the JSC-inspectors current mechanism for tracking callframes, etc when debugging, and just generally lead to a better life for all.

That is great news - do you have a timeline on when this is going to be committed?
Will this change allow to get the stack trace even without the respective JSC::ExecState handy, ie will it be easy to implement ScriptCallStackFactory::createScriptCallStack(size_t, bool)?

[Oliver Hunt]

(In reply to comment #6)
> (In reply to comment #5)
> > We're working on implementing Error.stack that will introduce logic to JSC to allow it to provide correct stack traces at all times, across arbitrary boundaries.  This will eventually lead to dropping the JSC-inspectors current mechanism for tracking callframes, etc when debugging, and just generally lead to a better life for all.
> 
> That is great news - do you have a timeline on when this is going to be committed?
> Will this change allow to get the stack trace even without the respective JSC::ExecState handy, ie will it be easy to implement ScriptCallStackFactory::createScriptCallStack(size_t, bool)?

Technically yes, but only due to commonJSGlobalData.  It would be much better if you could provide either a World pointer or callframe.

[Timothy Hatcher]

Bug 73099 is another feature that needed this support in JSC before it will work.

[sfa]

Created attachment 142716 [details]
patch for PR40118

[Early Warning System Bot]

Comment on attachment 142716 [details]
patch for PR40118

Attachment 142716 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12725509

[Early Warning System Bot]

Comment on attachment 142716 [details]
patch for PR40118

Attachment 142716 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12722817

[Gyuyoung Kim]

Comment on attachment 142716 [details]
patch for PR40118

Attachment 142716 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12719871

[Build Bot]

Comment on attachment 142716 [details]
patch for PR40118

Attachment 142716 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12725513

[Gustavo Noronha (kov)]

Comment on attachment 142716 [details]
patch for PR40118

Attachment 142716 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12727385

[Build Bot]

Comment on attachment 142716 [details]
patch for PR40118

Attachment 142716 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12716919

[sfa]

Created attachment 142747 [details]
patch for PR40118

Comment on attachment 142747 [details]
patch for PR40118

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

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:60
> +    if (exec) {

any reason not to just return createScriptCallStack(exec, maxStackSize)?

[Oliver Hunt]

Comment on attachment 142747 [details]
patch for PR40118

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

> Source/JavaScriptCore/interpreter/Interpreter.h:180
> +        UString getSourceURL() const
> +        {
> +            bool hasSourceURLInfo = !sourceURL.isNull() && !sourceURL.isEmpty();
> +            String traceLine;
> +
> +            switch (codeType) {
> +            case StackFrameEvalCode:
> +            case StackFrameFunctionCode:
> +            case StackFrameGlobalCode:
> +                if (hasSourceURLInfo)
> +                    traceLine = sourceURL.ascii().data();
> +                else
> +                    traceLine = "";
> +                break;
> +            case StackFrameNativeCode:
> +                traceLine = "[native code]";
> +                break;
> +            default:
> +                traceLine = "";
> +            }
> +            return traceLine.impl();
> +        }
> +        UString getFunctionName(CallFrame* callFrame) const
> +        {
> +            String traceLine;
> +            JSObject* stackFrameCallee = callee.get();
> +
> +            switch (codeType) {
> +            case StackFrameEvalCode:
> +                traceLine = "eval code";
> +                break;
> +            case StackFrameNativeCode: {
> +                if (callee) {
> +                    UString functionName = getCalculatedDisplayName(callFrame, stackFrameCallee);
> +                    traceLine = functionName.ascii().data();
> +                } else
> +                    traceLine = "";
> +                break;
> +            }
> +            case StackFrameFunctionCode: {
> +                UString functionName = getCalculatedDisplayName(callFrame, stackFrameCallee);
> +                traceLine = functionName.ascii().data();
> +                break;
> +            }
> +            case StackFrameGlobalCode:
> +                traceLine = "global code";
> +                break;
> +            default:
> +                traceLine = "";
> +            }
> +            return traceLine.impl();
> +        }
> +        unsigned getLineNumber() const
> +        {
> +            return line > -1 ? line : 0;
> +        }
>      };

StackFrame already has a toString method, this just seems like a huge amount of code duplication, if there are problems with the current toString behaviour we should just fix that.

>> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:60
>> +    if (exec) {
> 
> any reason not to just return createScriptCallStack(exec, maxStackSize)?

Yeah, this api should just take a World as input -- taking a world allows us to get the global data, and allows us in future to give each world its own globla data.

[sfa]

(In reply to comment #18)
> (From update of attachment 142747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142747&action=review
> 
> >      };
> 
> StackFrame already has a toString method, this just seems like a huge amount of code duplication, if there are problems with the current toString behaviour we should just fix that.

The construction of the Inspector stack trace needs the fn/url/line as separate items so I could have asked for the toString() and parsed it to extract the items but the toString() format has the optional '@' and could be problematic to parse reliably. That lead me to create the fn/url/line member fns by extracting the logic from toString() but refactoring toString() in terms of the member fns adds inefficiency. I don't mind doing it if you feel that is best (I agree it is best from the maintenance viewpoint).

I could submit a followup patch (to this PR) that refactors toString() to use the member fns:

        UString toString(CallFrame* callFrame) const
        {   String traceLine;
            UString functionName = getFunctionName(callFrame);
            UString sourceURL = getSourceURL();
            char const *urlSeparator = ( functionName.isEmpty() || sourceURL.isEmpty() ) ? "" : "@";
            if (line > -1)
                traceLine = String::format("%s%s%s:%d", functionName.ascii().data(), urlSeparator, sourceURL.ascii().data(), line);
            else
                traceLine = String::format("%s%s%s", functionName.ascii().data(), urlSeparator, sourceURL.ascii().data());
            return traceLine.impl();
        }

[sfa]

Created attachment 143824 [details]
Patch

[Early Warning System Bot]

Comment on attachment 143824 [details]
Patch

Attachment 143824 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12803162

[Early Warning System Bot]

Comment on attachment 143824 [details]
Patch

Attachment 143824 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12800196

[Build Bot]

Comment on attachment 143824 [details]
Patch

Attachment 143824 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12803165

[sfa]

Created attachment 143840 [details]
Patch

[Yong Li]

Comment on attachment 143840 [details]
Patch

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

I'm not a reviewer, neither so familiar with this part of code. But I am trying to give some coding style suggestions:

> Source/JavaScriptCore/interpreter/Interpreter.h:87
> +            char const *urlSeparator = (functionName.isEmpty() || sourceURL.isEmpty()) ? "" : "@";

const char* is more popular in webkit rather than char const*. I don't know why "Type *varName" didn't trigger a style error, but usually we use "Type* varName".

> Source/JavaScriptCore/interpreter/Interpreter.h:91
> +            if (line > -1)
> +                traceLine = String::format("%s%s%s:%d", functionName.ascii().data(), urlSeparator, sourceURL.ascii().data(), line);
> +            else
> +                traceLine = String::format("%s%s%s", functionName.ascii().data(), urlSeparator, sourceURL.ascii().data());

Could functionName.ascii().data() or sourceURL.ascii().data() be null? I would use StringBuilder rather than String::format.

> Source/JavaScriptCore/interpreter/Interpreter.h:113
> +                traceLine = "";
> +            }

should we put a break here?

> Source/JavaScriptCore/interpreter/Interpreter.h:146
> +        unsigned getLineNumber() const

webkit code perfers foo() to getFoo() in this case there is no argument. same thing for getFunctionName and getSourceURL

Also, any reason why we need to make these get functions inline? I would move them to .cpp file

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:60
> +    JSC::ExecState* exec = JSMainThreadExecState::currentState();
> +    if (exec) {

could be if (JSC::ExecState* exec = JSMainThreadExecState::currentState()) {

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:63
> +        for (Vector<StackFrame>::iterator iter = stackTrace.begin(); iter < stackTrace.end(); iter++) {

should be const_iterator?

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:65
> +            frames.append(ScriptCallFrame(ustringToString(level.getFunctionName(exec)), ustringToString(level.getSourceURL()), level.getLineNumber()));

So finally we need a WTF::String. Why do we return a UString in getSourceURL?

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:70
> +    if (!frames.size() && !emptyIsAllowed) {

frame.isEmpty() is better.

[Oliver Hunt]

Comment on attachment 143840 [details]
Patch

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

r- here, far too much of your code is blindly assuming ascii names, UString and String are both utf16, calling ascii potentially loses data so you shouldn't be calling it outside of debug code.

>> Source/JavaScriptCore/interpreter/Interpreter.h:87
>> +            char const *urlSeparator = (functionName.isEmpty() || sourceURL.isEmpty()) ? "" : "@";
> 
> const char* is more popular in webkit rather than char const*. I don't know why "Type *varName" didn't trigger a style error, but usually we use "Type* varName".

Why are you modifying the toString behaviour of stackframe?

> Source/JavaScriptCore/interpreter/Interpreter.h:138
> +        UString getFunctionName(CallFrame* callFrame) const
> +        {
>              String traceLine;
>              JSObject* stackFrameCallee = callee.get();
>  
>              switch (codeType) {
>              case StackFrameEvalCode:
> -                if (hasSourceURLInfo) {
> -                    traceLine = hasLineInfo ? String::format("eval code@%s:%d", sourceURL.ascii().data(), line) 
> -                                            : String::format("eval code@%s", sourceURL.ascii().data());
> -                } else
> -                    traceLine = String::format("eval code");
> +                traceLine = "eval code";
>                  break;
>              case StackFrameNativeCode: {
>                  if (callee) {
>                      UString functionName = getCalculatedDisplayName(callFrame, stackFrameCallee);
> -                    traceLine = String::format("%s@[native code]", functionName.ascii().data());
> +                    traceLine = functionName.ascii().data();
>                  } else
> -                    traceLine = "[native code]";
> +                    traceLine = "";
>                  break;
>              }
>              case StackFrameFunctionCode: {
>                  UString functionName = getCalculatedDisplayName(callFrame, stackFrameCallee);
> -                if (hasSourceURLInfo) {
> -                    traceLine = hasLineInfo ? String::format("%s@%s:%d", functionName.ascii().data(), sourceURL.ascii().data(), line)
> -                                            : String::format("%s@%s", functionName.ascii().data(), sourceURL.ascii().data());
> -                } else
> -                    traceLine = String::format("%s\n", functionName.ascii().data());
> +                traceLine = functionName.ascii().data();
>                  break;
>              }
>              case StackFrameGlobalCode:

You're making this just store the function name, so there's no reason copy into a separate 8 bit buffer.  It's a) expensive, and b) potentially data losing.  just use ustringToString() that's going to be a trivial increment  of a refcount.

>> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:60
>> +    if (exec) {
> 
> could be if (JSC::ExecState* exec = JSMainThreadExecState::currentState()) {

A better approach would be
if (!exec)
    return ...;

[Yong Li]

(In reply to comment #26)
> A better approach would be
> if (!exec)
>     return ...;

I noticed there are still some functional code running for !exec case. They are probably unnecessary though.

[sfa]

Created attachment 148334 [details]
Patch

[WebKit Review Bot]

Attachment 148334 [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/JavaScriptCore/interpreter/Interpreter.h:81:  The parameter name "callFrame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/interpreter/Interpreter.h:83:  The parameter name "callFrame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[sfa]

New patch submitted, it is hoped that this one addresses all the comments. The refactoring of toString was done based on previous comments. I maintained the style used in the other prototypes of Interpreter.h also WRT argument naming.

[Build Bot]

Comment on attachment 148334 [details]
Patch

Attachment 148334 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12987089

[sfa]

Created attachment 148339 [details]
Patch

[Build Bot]

Comment on attachment 148339 [details]
Patch

Attachment 148339 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12990015

[Build Bot]

Comment on attachment 148339 [details]
Patch

Attachment 148339 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12976633

[sfa]

Created attachment 148347 [details]
Patch

[Build Bot]

Comment on attachment 148347 [details]
Patch

Attachment 148347 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12976644

[sfa]

Created attachment 148352 [details]
Patch

[sfa]

The StackTrace member functions are back to being inline in the current patch to cleanup the Win and Mac64 linkage issues.

[sfa]

Created attachment 149291 [details]
Patch

[Yong Li]

Comment on attachment 149291 [details]
Patch

looks good to me

[WebKit Review Bot]

Comment on attachment 149291 [details]
Patch

Clearing flags on attachment: 149291

Committed r121359: <http://trac.webkit.org/changeset/121359>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 90115

[Konrad Piascik]

(In reply to comment #43)
> Re-opened since this is blocked by 90115

Can someone please attach or link to the failing tests?

[sfa]

Created attachment 150821 [details]
Patch

[Yong Li]

Comment on attachment 150821 [details]
Patch

LGTM

[sfa]

Created attachment 150830 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150830 [details]
Patch

Clearing flags on attachment: 150830

Committed r121871: <http://trac.webkit.org/changeset/121871>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.