Bug 36839 – Web Inspector: Adds the ability to get the function symbol name when looking up call location.

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

This patch adds the ability to get the function symbol name when looking up the call location for records sent by the InspectorTimelineAgent.

We are still only getting the top call frame. I would like to follow up with a change to grab the entire call stack, but that may require a bit of refactoring. I am going to talk to Pavel and Yury a bit more before working on that change since it may mean touching Console and ConsoleMessage.

Created attachment 52060 [details]
Path to add function symbol name to call location

Created attachment 52062 [details]
Forgot to update ChangeLog for LayoutTests. New patchset with update.

[Pavel Feldman]

Comment on attachment 52060 [details]
Path to add function symbol name to call location

deprecating the first patch.

[Pavel Feldman]

Comment on attachment 52062 [details]
Forgot to update ChangeLog for LayoutTests. New patchset with update.

> -    if (!V8Proxy::sourceName(*sourceName) || !V8Proxy::sourceLineNumber(*sourceLineNumber))
> +    if (!V8Proxy::sourceName(*sourceName) || !V8Proxy::sourceLineNumber(*sourceLineNumber) || !V8Proxy::funcName(*funcName))

This looks very inefficient - we should try fetching all at a time.

> +    DEFINE_STATIC_LOCAL(const char*, frameSourceLine,
>          ("function frameSourceLine(exec_state) {"
>          "  return exec_state.frame(0).sourceLine();"

This looks like a very old WebKit. Recent ones require  checks for frame count.

> +bool V8Proxy::funcName(String& result)
> +{

I think we are going to have a bunch of methods like this and I am not sure we should put them into V8Proxy. There is a nice new ScriptDebugServer class in bindings that abstracts us from JS engine. It is nearly blank now, but we are working on filling it. I think all three methods should be defined there and it should own the utility context business as a whole. It would give us more flexibility and we'll be able to fetch all the info you need within single call (such as top stack frames, etc.). We should move ScriptCallStack::callLocation there as well.

(In reply to comment #4)
> (From update of attachment 52062 [details])
> > -    if (!V8Proxy::sourceName(*sourceName) || !V8Proxy::sourceLineNumber(*sourceLineNumber))
> > +    if (!V8Proxy::sourceName(*sourceName) || !V8Proxy::sourceLineNumber(*sourceLineNumber) || !V8Proxy::funcName(*funcName))
> 
> This looks very inefficient - we should try fetching all at a time.
> 
> > +    DEFINE_STATIC_LOCAL(const char*, frameSourceLine,
> >          ("function frameSourceLine(exec_state) {"
> >          "  return exec_state.frame(0).sourceLine();"
> 
> This looks like a very old WebKit. Recent ones require  checks for frame count.
> 
> > +bool V8Proxy::funcName(String& result)
> > +{
> 
> I think we are going to have a bunch of methods like this and I am not sure we
> should put them into V8Proxy. There is a nice new ScriptDebugServer class in
> bindings that abstracts us from JS engine. It is nearly blank now, but we are
> working on filling it. I think all three methods should be defined there and it
> should own the utility context business as a whole. It would give us more
> flexibility and we'll be able to fetch all the info you need within single call
> (such as top stack frames, etc.). We should move ScriptCallStack::callLocation
> there as well.

This sounds good. Do you want me to refactor this to fetch everything in a single call in this patch?

I believe that returning a single JavaScript object would mean that we need to make the StackFrame information opaque to C++... or else we will have a fairly unmaintainable change cross platform.

ScriptCallStack and ScriptCallFrame both have API that expose the stack frame information in C++ land. This gets more complicated since Console and ConsoleMessage depend on being able to read this information in C++ when talking to the Inspector.

I am interested in your opinion on what this refactor should look like. Should I also ping Yurys?

Created attachment 52645 [details]
Updated patchset according to Pavel's suggestions

Moves ownership of the utility context from V8Proxy to ScriptDebugServer.

Adds the ability to grab the function name for the top call frame. Also refactors such that we only enter the utility context once to get the line number, script name, and function name.

[Yury Semikhatsky]

Comment on attachment 52645 [details]
Updated patchset according to Pavel's suggestions

> +    // Compile JavaScript function for retrieving the source line, the source
> +    // name and the symbol name for the top JavaScript stack frame.
> +    DEFINE_STATIC_LOCAL(const char*, lastCallFrame,
> +        ("function lastCallFrame(exec_state) {"
> +        "  var frame = exec_state.frame(0);"
Please check that the stack is not empty before accessing first frame. TOT WebKit already contains exec_state.frameCount() check, it seem that you copy needs update.

> +    
> +    // Function for retrieving the source name, line number and function name for the top
> +    // JavaScript stack frame.
> +    //
> +    // It will return true if the caller information was successfully retrieved and written
> +    // into the function parameters, otherwise the function will return false. It may
> +    // fail due to a stck overflow in the underlying JavaScript implentation, handling
> +    // of such exception is up to the caller.
> +    static bool lastCallFrame(String& sourceName, int& lineNumber, String& funcName);
>  
Consider naming it topCallFrame.


Please add frameCount check, otherwise r+.

Created attachment 52655 [details]
Merging up to r. 57156 and adding guard for frameCount.

[WebKit Review Bot]

Attachment 52655 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/ScriptDebugServer.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/ScriptDebugServer.h:99:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 11 files


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

Created attachment 52668 [details]
Renamed stuff according to feedback. Fixed WebKit style issues.

[Yury Semikhatsky]

Comment on attachment 52668 [details]
Renamed stuff according to feedback. Fixed WebKit style issues.

Looks good. Next time when you're uploading updated patch please mark previous one as obsolete if it doesn't require further review.

[WebKit Commit Bot]

Comment on attachment 52668 [details]
Renamed stuff according to feedback. Fixed WebKit style issues.

Clearing flags on attachment: 52668

Committed r57196: <http://trac.webkit.org/changeset/57196>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

Reopening since the patch was rolled out due to layout test failure in Chromium(https://bugs.webkit.org/show_bug.cgi?id=37196).

Created attachment 52773 [details]
Fixes layout test failures.

This patch includes the fixes for the inspector layout tests submitted in https://bugs.webkit.org/show_bug.cgi?id=37192.

I have also resolved the plugin layout test failures that were a result of returning failure when there was no JS on the call stack. The old code that used to live in V8Proxy would return success and set the line number to 0 and the source name to "undefined". I have adjusted this change to match those semantics.

[WebKit Commit Bot]

Comment on attachment 52773 [details]
Fixes layout test failures.

Rejecting patch 52773 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12639 test cases.
fast/canvas/webgl/index-validation-copies-indices.html -> failed

Exiting early after 1 failures. 5081 tests run.
105.15s total testing time

5080 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1666215

[Eric Seidel (no email)]

Comment on attachment 52773 [details]
Fixes layout test failures.

Flaky test.

[WebKit Commit Bot]

Comment on attachment 52773 [details]
Fixes layout test failures.

Clearing flags on attachment: 52773

Committed r57231: <http://trac.webkit.org/changeset/57231>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

*** Bug 37192 has been marked as a duplicate of this bug. ***