Bug 36839 - Web Inspector: Adds the ability to get the function symbol name when looking up call location.
Summary: Web Inspector: Adds the ability to get the function symbol name when looking ...
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:
: 37192 (view as bug list)
Depends on: 37196
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-30 11:13 PDT by jaimeyap
Modified: 2010-04-07 14:30 PDT (History)
7 users (show)

See Also:


Attachments
Path to add function symbol name to call location (9.82 KB, patch)
2010-03-30 11:28 PDT, jaimeyap
no flags Details | Formatted Diff | Diff
Forgot to update ChangeLog for LayoutTests. New patchset with update. (9.92 KB, patch)
2010-03-30 11:34 PDT, jaimeyap
pfeldman: review-
Details | Formatted Diff | Diff
Updated patchset according to Pavel's suggestions (18.55 KB, patch)
2010-04-06 09:33 PDT, jaimeyap
yurys: review-
Details | Formatted Diff | Diff
Merging up to r. 57156 and adding guard for frameCount. (16.65 KB, patch)
2010-04-06 11:59 PDT, jaimeyap
no flags Details | Formatted Diff | Diff
Renamed stuff according to feedback. Fixed WebKit style issues. (17.13 KB, patch)
2010-04-06 14:36 PDT, jaimeyap
no flags Details | Formatted Diff | Diff
Fixes layout test failures. (20.71 KB, patch)
2010-04-07 12:53 PDT, jaimeyap
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jaimeyap 2010-03-30 11:13:31 PDT
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.
Comment 1 jaimeyap 2010-03-30 11:28:25 PDT
Created attachment 52060 [details]
Path to add function symbol name to call location
Comment 2 jaimeyap 2010-03-30 11:34:11 PDT
Created attachment 52062 [details]
Forgot to update ChangeLog for LayoutTests. New patchset with update.
Comment 3 Pavel Feldman 2010-03-30 12:01:19 PDT
Comment on attachment 52060 [details]
Path to add function symbol name to call location

deprecating the first patch.
Comment 4 Pavel Feldman 2010-03-30 12:13:01 PDT
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.
Comment 5 jaimeyap 2010-04-05 08:07:34 PDT
(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?
Comment 6 jaimeyap 2010-04-06 09:33:04 PDT
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.
Comment 7 Yury Semikhatsky 2010-04-06 09:57:05 PDT
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+.
Comment 8 jaimeyap 2010-04-06 11:59:48 PDT
Created attachment 52655 [details]
Merging up to r. 57156 and adding guard for frameCount.
Comment 9 WebKit Review Bot 2010-04-06 12:00:59 PDT
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.
Comment 10 jaimeyap 2010-04-06 14:36:26 PDT
Created attachment 52668 [details]
Renamed stuff according to feedback. Fixed WebKit style issues.
Comment 11 Yury Semikhatsky 2010-04-06 23:52:04 PDT
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.
Comment 12 WebKit Commit Bot 2010-04-07 00:20:17 PDT
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>
Comment 13 WebKit Commit Bot 2010-04-07 00:20:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Yury Semikhatsky 2010-04-07 05:00:50 PDT
Reopening since the patch was rolled out due to layout test failure in Chromium(https://bugs.webkit.org/show_bug.cgi?id=37196).
Comment 15 jaimeyap 2010-04-07 12:53:19 PDT
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.
Comment 16 WebKit Commit Bot 2010-04-07 13:57:11 PDT
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
Comment 17 Eric Seidel (no email) 2010-04-07 13:59:58 PDT
Comment on attachment 52773 [details]
Fixes layout test failures.

Flaky test.
Comment 18 WebKit Commit Bot 2010-04-07 14:21:00 PDT
Comment on attachment 52773 [details]
Fixes layout test failures.

Clearing flags on attachment: 52773

Committed r57231: <http://trac.webkit.org/changeset/57231>
Comment 19 WebKit Commit Bot 2010-04-07 14:21:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Eric Seidel (no email) 2010-04-07 14:30:43 PDT
*** Bug 37192 has been marked as a duplicate of this bug. ***