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
Forgot to update ChangeLog for LayoutTests. New patchset with update. (9.92 KB, patch)
2010-03-30 11:34 PDT, jaimeyap
pfeldman: review-
Updated patchset according to Pavel's suggestions (18.55 KB, patch)
2010-04-06 09:33 PDT, jaimeyap
yurys: review-
Merging up to r. 57156 and adding guard for frameCount. (16.65 KB, patch)
2010-04-06 11:59 PDT, jaimeyap
no flags
Renamed stuff according to feedback. Fixed WebKit style issues. (17.13 KB, patch)
2010-04-06 14:36 PDT, jaimeyap
no flags
Fixes layout test failures. (20.71 KB, patch)
2010-04-07 12:53 PDT, jaimeyap
no flags
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.