WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
36839
Web Inspector: Adds the ability to get the function symbol name when looking up call location.
https://bugs.webkit.org/show_bug.cgi?id=36839
Summary
Web Inspector: Adds the ability to get the function symbol name when looking ...
jaimeyap
Reported
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.
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
jaimeyap
Comment 1
2010-03-30 11:28:25 PDT
Created
attachment 52060
[details]
Path to add function symbol name to call location
jaimeyap
Comment 2
2010-03-30 11:34:11 PDT
Created
attachment 52062
[details]
Forgot to update ChangeLog for LayoutTests. New patchset with update.
Pavel Feldman
Comment 3
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.
Pavel Feldman
Comment 4
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.
jaimeyap
Comment 5
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?
jaimeyap
Comment 6
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.
Yury Semikhatsky
Comment 7
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+.
jaimeyap
Comment 8
2010-04-06 11:59:48 PDT
Created
attachment 52655
[details]
Merging up to r. 57156 and adding guard for frameCount.
WebKit Review Bot
Comment 9
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.
jaimeyap
Comment 10
2010-04-06 14:36:26 PDT
Created
attachment 52668
[details]
Renamed stuff according to feedback. Fixed WebKit style issues.
Yury Semikhatsky
Comment 11
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.
WebKit Commit Bot
Comment 12
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
>
WebKit Commit Bot
Comment 13
2010-04-07 00:20:23 PDT
All reviewed patches have been landed. Closing bug.
Yury Semikhatsky
Comment 14
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
).
jaimeyap
Comment 15
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.
WebKit Commit Bot
Comment 16
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
Eric Seidel (no email)
Comment 17
2010-04-07 13:59:58 PDT
Comment on
attachment 52773
[details]
Fixes layout test failures. Flaky test.
WebKit Commit Bot
Comment 18
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
>
WebKit Commit Bot
Comment 19
2010-04-07 14:21:07 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 20
2010-04-07 14:30:43 PDT
***
Bug 37192
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug