Bug 37502 – Web Inspector: Removes public callLocation API from ScriptCallStack and replaces with ability to get top 10 stack frames

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

This patch removes public callLocation API from ScriptCallStack and replaces it with a function for getting top 10 stack frames, returned as a String.

ScriptCallStack::callLocation() had a null impl previously in JSC.  ScriptCallStack::stackTrace also has a null impl in JSC, so nothing has changed there.

I have updated the TimelinePanel UI to consume this stack trace instead of the callerLocation fields that used to be set on timeline records. I left it displaying only the top call frame since there are some UI considerations there that would probably be better addressed in a subsequent patch.

Created attachment 53257 [details]
patch

I would like feedback on the general approach. One thing to note is that my measurements show that grabbing the stack trace only adds 1 to 3ms in the worse case to a node. This can add up for traces with hundreds or thousands of nodes. So I have implemented a simple cache for the current stack trace in InspectorTimelineAgent.cpp

The rationale is that the JS Stack trace, once acquired will be unchanged until we run some javascript. This means we invalidate the cached stack trace on ScriptEvaluation, FunctionCalls, or when we pop the record stack.

[Pavel Feldman]

Comment on attachment 53257 [details]
patch

> +    static bool stackTrace(String& stackTrace);

Should be String* as in other places returning values.

>  InspectorTimelineAgent::InspectorTimelineAgent(InspectorFrontend* frontend)
> -    : m_frontend(frontend)
> +    : m_frontend(frontend),
> +      m_currentStackTrace(),

No need to init this one.

> +      m_hasStackTrace(false)

Is following true?

m_hasStackTrace == !m_currentStackTrace.isEmpty()

>  {
>      ++s_instanceCount;
>      ScriptGCEvent::addEventListener(this);
> @@ -57,7 +60,7 @@ InspectorTimelineAgent::InspectorTimelin
>  void InspectorTimelineAgent::pushGCEventRecords()
>  {
>      for (GCEvents::iterator i = m_gcEvents.begin(); i != m_gcEvents.end(); ++i) {
> -        ScriptObject record = TimelineRecordFactory::createGenericRecord(m_frontend, i->startTime);
> +        ScriptObject record = TimelineRecordFactory::createGenericRecord(m_frontend, i->startTime, getCurrentStackTrace());

Present stack has nothing to do with the stack while doing GC.

> +    if (m_recordStack.isEmpty()) {
>          m_frontend->addRecordToTimeline(record);
> -    else {
> +        invalidateStackTrace();
> +    } else {
>          TimelineRecordEntry parent = m_recordStack.last();
>          parent.children.set(parent.children.length(), record);
>      }

No need for { } in single-line statements.

> +String* InspectorTimelineAgent::getCurrentStackTrace()
> +{
> +    if (m_currentStackTrace.isEmpty()) {

I wonder if you covered all the cases where stack needs to be invalidated. How often do we get nested records where this cache is actually helping?

[Ilya Tikhonovsky]

> Index: WebCore/ChangeLog
> +
> +String* InspectorTimelineAgent::getCurrentStackTrace()
> +{
> +    if (m_currentStackTrace.isEmpty()) {       
> +        m_hasStackTrace = ScriptCallStack::stackTrace(&m_currentStackTrace);
> +        // So that we don't attempt to regrab a strack trace when there is none to grab.
> +        if (!m_hasStackTrace)
> +            m_currentStackTrace = "NotEmpty";
> +    }
> +    if (m_hasStackTrace)
> +        return &m_currentStackTrace;
> +    else
> +        return 0;
> +}
> +
> +void InspectorTimelineAgent::invalidateStackTrace() {
> +    m_currentStackTrace = "";
> +    m_hasStackTrace = false;
> +}

We can have a bit complex situation as example with two listeners (an event with two nested callFunction events will be generated).
If the first one generate an install timer event then for the second willCallFunction event we assign the call stack of install timer event.

I think it might be interesting to cache call stack in the parent event record but I'm not sure that we will have no problem.


> @@ -811,8 +812,11 @@ WebInspector.TimelinePanel.FormattedReco
>      } else if (record.type === recordTypes.TimerFire) {
>          var timerInstalledRecord = timerRecords[record.data.timerId];
>          if (timerInstalledRecord) {
> -            this.callSiteScriptName = timerInstalledRecord.callerScriptName;
> -            this.callSiteScriptLine = timerInstalledRecord.callerScriptLine;
> +            var stackTrace = timerInstalledRecord.stackTrace;
> +            if (stackTrace && stackTrace[0]) {
> +                this.callSiteScriptName = stackTrace[0].scriptName;
> +                this.callSiteScriptLine = stackTrace[0].scriptLine + 1;                
> +            }


We can't use +1 for scriptLine here because JSC numbers are starting from 1. It should be done in V8 specific code.



> @@ -918,8 +922,10 @@ WebInspector.TimelinePanel.FormattedReco
>                  recordContentTable.appendChild(this._createRow(WebInspector.UIString("Details"), this.details));
>          }
>  
> -        if (this.type !== recordTypes.GCEvent && this.callerScriptName) {
> -            var link = WebInspector.linkifyResourceAsNode(this.callerScriptName, "scripts", this.callerScriptLine);
> +        if (this.type !== recordTypes.GCEvent && this.stackTrace) {
> +            var callerScriptName = this.stackTrace[0] ? this.stackTrace[0].scriptName : "";
> +            var callerScriptLine = this.stackTrace[0] ? this.stackTrace[0].scriptLine + 1 : 0;


Ditto.

I forgot to reply on the bug itself. I for some reason thought replying via email would post the response. Re-posting here:

(In reply to comment #2)
> (From update of attachment 53257 [details])
> > +    static bool stackTrace(String& stackTrace);
> 
> Should be String* as in other places returning values.
> 
> >  InspectorTimelineAgent::InspectorTimelineAgent(InspectorFrontend* frontend)
> > -    : m_frontend(frontend)
> > +    : m_frontend(frontend),
> > +      m_currentStackTrace(),
> 
> No need to init this one.
> 
> > +      m_hasStackTrace(false)
> 
> Is following true?
> 
> m_hasStackTrace == !m_currentStackTrace.isEmpty()

Not necessarily. Consider the case where the the method in the utility context fails to set a value for the stack trace (in the case where there is no stack trace). That is, we try to retrieve a stack trace, but for some reason the returned value is the empty String.

We do not want to have to re-enter the utility context for each node if we have already attempted to retrieve a stack trace, so we need to keep some separate state reflecting the condition "I have checked for a stack trace, and failed to get one.". This is separate from simply saying "The stack is invalid" (empty) or "the stack is valid" (set to some value).

> 
> >  {
> >      ++s_instanceCount;
> >      ScriptGCEvent::addEventListener(this);
> > @@ -57,7 +60,7 @@ InspectorTimelineAgent::InspectorTimelin
> >  void InspectorTimelineAgent::pushGCEventRecords()
> >  {
> >      for (GCEvents::iterator i = m_gcEvents.begin(); i != m_gcEvents.end(); ++i) {
> > -        ScriptObject record = TimelineRecordFactory::createGenericRecord(m_frontend, i->startTime);
> > +        ScriptObject record = TimelineRecordFactory::createGenericRecord(m_frontend, i->startTime, getCurrentStackTrace());
> 
> Present stack has nothing to do with the stack while doing GC.
> 

Good point.

> > +    if (m_recordStack.isEmpty()) {
> >          m_frontend->addRecordToTimeline(record);
> > -    else {
> > +        invalidateStackTrace();
> > +    } else {
> >          TimelineRecordEntry parent = m_recordStack.last();
> >          parent.children.set(parent.children.length(), record);
> >      }
> 
> No need for { } in single-line statements.
> 
> > +String* InspectorTimelineAgent::getCurrentStackTrace()
> > +{
> > +    if (m_currentStackTrace.isEmpty()) {
> 
> I wonder if you covered all the cases where stack needs to be invalidated. 

I did miss a few cases. I will update the patch in the morning. I also need to invalidate when popping function call and eval script.

How
> often do we get nested records where this cache is actually helping?

Alot! You should see Wave or GMail. The pattern of UI construction where using innerHTML for lots of small widgets causes literally hundreds of parseHTML nodes. For nearly all complicated traces, caching the trace  makes a very large difference. It is literally the difference between a few milliseconds versus sometimes several seconds of overhead.

(In reply to comment #3)
> > Index: WebCore/ChangeLog
> > +
> > +String* InspectorTimelineAgent::getCurrentStackTrace()
> > +{
> > +    if (m_currentStackTrace.isEmpty()) {       
> > +        m_hasStackTrace = ScriptCallStack::stackTrace(&m_currentStackTrace);
> > +        // So that we don't attempt to regrab a strack trace when there is none to grab.
> > +        if (!m_hasStackTrace)
> > +            m_currentStackTrace = "NotEmpty";
> > +    }
> > +    if (m_hasStackTrace)
> > +        return &m_currentStackTrace;
> > +    else
> > +        return 0;
> > +}
> > +
> > +void InspectorTimelineAgent::invalidateStackTrace() {
> > +    m_currentStackTrace = "";
> > +    m_hasStackTrace = false;
> > +}
> 
> We can have a bit complex situation as example with two listeners (an event
> with two nested callFunction events will be generated).
> If the first one generate an install timer event then for the second
> willCallFunction event we assign the call stack of install timer event.

Do the willCallFunctions nest within eachother? Or will they be peers? If they are peers then it will be sufficient to invalidate the stack when popping willCallFunctions. 

That being said though, I think I had a rather large flaw in my reasoning about re-entering javascript. The approach that I thought would work was to invalidate:
- When pushing willCallFunction or EvalScript.
- When popping willCallFunction or EvalScript.
- When popping the entire record stack, completing a top level event.

But the issue is that control can return to javascript anytime within either of these two events. I am starting to think that caching the stack trace is in fact incorrect in the common case. It only "helps" in the cases where layout and style recalculation run side-by-side before returning control to JS, and in the case where nodes nest before returning control to JS. The case I intended to optimize, which is the common case where we have many peer nodes that are children of some JavaScript execution, would in fact be incorrect to no re-grab stack traces each time.

I will experiment with reducing the number of stack frames we stringify to see what overhead we still see. I think I will take out the cache in my subsequent patch.

> 
> I think it might be interesting to cache call stack in the parent event record
> but I'm not sure that we will have no problem.
> 
> 
> > @@ -811,8 +812,11 @@ WebInspector.TimelinePanel.FormattedReco
> >      } else if (record.type === recordTypes.TimerFire) {
> >          var timerInstalledRecord = timerRecords[record.data.timerId];
> >          if (timerInstalledRecord) {
> > -            this.callSiteScriptName = timerInstalledRecord.callerScriptName;
> > -            this.callSiteScriptLine = timerInstalledRecord.callerScriptLine;
> > +            var stackTrace = timerInstalledRecord.stackTrace;
> > +            if (stackTrace && stackTrace[0]) {
> > +                this.callSiteScriptName = stackTrace[0].scriptName;
> > +                this.callSiteScriptLine = stackTrace[0].scriptLine + 1;                
> > +            }
> 
> 
> We can't use +1 for scriptLine here because JSC numbers are starting from 1. It
> should be done in V8 specific code.
> 

Yes ofcourse :).

> 
> 
> > @@ -918,8 +922,10 @@ WebInspector.TimelinePanel.FormattedReco
> >                  recordContentTable.appendChild(this._createRow(WebInspector.UIString("Details"), this.details));
> >          }
> >  
> > -        if (this.type !== recordTypes.GCEvent && this.callerScriptName) {
> > -            var link = WebInspector.linkifyResourceAsNode(this.callerScriptName, "scripts", this.callerScriptLine);
> > +        if (this.type !== recordTypes.GCEvent && this.stackTrace) {
> > +            var callerScriptName = this.stackTrace[0] ? this.stackTrace[0].scriptName : "";
> > +            var callerScriptLine = this.stackTrace[0] ? this.stackTrace[0].scriptLine + 1 : 0;
> 
> 
> Ditto.

Will fix.

Created attachment 55774 [details]
uses new v8 stacktrace api.

This patch has the same intended functionality, but it uses the new StackTrace API in V8.

It is very fast, and it removes the need to the utility context in ScriptCallStack. I would like some comments before requesting commit-queue.

[WebKit Review Bot]

Attachment 55774 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2231171

(In reply to comment #7)
> Attachment 55774 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/2231171

This V8 API just got rolled into chromium. I am assuming this build failure it due to the webkit try bots needing a Deps roll.

[David Levin]

(In reply to comment #8)
> (In reply to comment #7)
> > Attachment 55774 [details] [details] did not build on chromium:
> > Build output: http://webkit-commit-queue.appspot.com/results/2231171
> 
> This V8 API just got rolled into chromium. I am assuming this build failure it due to the webkit try bots needing a Deps roll.

You likely need to change this file: http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS to pick up a new version of chromium (and that will in turn pick up the new v8 rev).

[Pavel Feldman]

Comment on attachment 55774 [details]
uses new v8 stacktrace api.

Great job! Make sure you roll the WebKit deps bots as suggested by David.

WebCore/bindings/v8/ScriptCallStack.cpp:97
 +      if (trace.IsEmpty() || !trace->GetFrameCount()) {
no { } needed

WebCore/bindings/v8/ScriptCallStack.cpp:98
 +        return false;
wrong indent

WebCore/inspector/TimelineRecordFactory.cpp:53
 +      if (ScriptCallStack::stackTrace(5, frontend->scriptState(), stackTrace)) {
no { } needed

Created attachment 55848 [details]
Fixes style issues and updates chromium deps

[WebKit Commit Bot]

Comment on attachment 55848 [details]
Fixes style issues and updates chromium deps

Rejecting patch 55848 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Pavel Feldman', u'--force']" exit_code: 1
Last 500 characters of output:
youtTests/inspector/timeline-test.js
patching file LayoutTests/platform/chromium-win/inspector/timeline-event-dispatch-expected.txt
patching file LayoutTests/platform/chromium-win/inspector/timeline-mark-timeline-expected.txt
patching file LayoutTests/platform/chromium-win/inspector/timeline-network-resource-expected.txt
patching file LayoutTests/platform/chromium-win/inspector/timeline-paint-expected.txt
patching file LayoutTests/platform/chromium-win/inspector/timeline-parse-html-expected.txt

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

[Eric Seidel (no email)]

Comment on attachment 55774 [details]
uses new v8 stacktrace api.

Cleared Pavel Feldman's review+ from obsolete attachment 55774 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 56252 [details]
undoes the DEPS change

The previous patch rolled the chromium deps, but someone else rolled it before the commit queue got to it, and thus it conflicted.

This patch simply undoes the DEPS change since the DEPS in webkit should support the appropriate version of chromium.

Comment on attachment 55848 [details]
Fixes style issues and updates chromium deps

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 59177)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,28 @@
> [phone]  Jaime Yap  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Removes public callLocation API and utility context from ScriptCallStack.
> +        Adds ability to grab the top 5 JavaScript stack frames for each timeline
> +        record using new fast V8 stack trace API.
> +        https://bugs.webkit.org/show_bug.cgi?id=37502
> +
> +        * bindings/js/ScriptCallStack.cpp:
> +        (WebCore::ScriptCallStack::stackTrace):
> +        * bindings/js/ScriptCallStack.h:
> +        * bindings/v8/ScriptArray.h:
> +        (WebCore::ScriptArray::ScriptArray):
> +        * bindings/v8/ScriptCallStack.cpp:
> +        (WebCore::ScriptCallStack::callLocation):
> +        (WebCore::ScriptCallStack::stackTrace):
> +        * bindings/v8/ScriptCallStack.h:
> +        * inspector/TimelineRecordFactory.cpp:
> +        (WebCore::TimelineRecordFactory::createGenericRecord):
> +        * inspector/front-end/TimelinePanel.js:
> +        (WebInspector.TimelinePanel.FormattedRecord):
> +        (WebInspector.TimelinePanel.FormattedRecord.prototype._generatePopupContent):
> +        (WebInspector.TimelinePanel.FormattedRecord.prototype._getRecordDetails):
> +
>  2010-05-11  Mark Rowe  <[e-mail]>
>  
>          Fix the world.
> Index: WebCore/bindings/js/ScriptCallStack.cpp
> ===================================================================
> --- WebCore/bindings/js/ScriptCallStack.cpp	(revision 59174)
> +++ WebCore/bindings/js/ScriptCallStack.cpp	(working copy)
> @@ -101,7 +101,7 @@ void ScriptCallStack::initialize()
>      m_initialized = true;
>  }
>  
> -bool ScriptCallStack::callLocation(String*, int*, String*)
> +bool ScriptCallStack::stackTrace(int, ScriptState*, ScriptArray&)
>  {
>      return false;
>  }
> Index: WebCore/bindings/js/ScriptCallStack.h
> ===================================================================
> --- WebCore/bindings/js/ScriptCallStack.h	(revision 59174)
> +++ WebCore/bindings/js/ScriptCallStack.h	(working copy)
> @@ -31,6 +31,7 @@
>  #ifndef ScriptCallStack_h
>  #define ScriptCallStack_h
>  
> +#include "ScriptArray.h"
>  #include "ScriptCallFrame.h"
>  #include "ScriptState.h"
>  #include "ScriptString.h"
> @@ -53,7 +54,7 @@ namespace WebCore {
>          // frame retrieval methods
>          const ScriptCallFrame &at(unsigned);
>          unsigned size();
> -        static bool callLocation(String*, int*, String*);
> +        static bool stackTrace(int, ScriptState*, ScriptArray&);
>  
>      private:
>          void initialize();
> Index: WebCore/bindings/v8/ScriptArray.h
> ===================================================================
> --- WebCore/bindings/v8/ScriptArray.h	(revision 59174)
> +++ WebCore/bindings/v8/ScriptArray.h	(working copy)
> @@ -41,6 +41,7 @@ namespace WebCore {
>      class ScriptArray : public ScriptObject {
>      public:
>          ScriptArray(ScriptState* scriptState, v8::Handle<v8::Array>);
> +        ScriptArray() {};
>          virtual ~ScriptArray() {}
>  
>          bool set(unsigned index, const ScriptObject&);
> Index: WebCore/bindings/v8/ScriptCallStack.cpp
> ===================================================================
> --- WebCore/bindings/v8/ScriptCallStack.cpp	(revision 59174)
> +++ WebCore/bindings/v8/ScriptCallStack.cpp	(working copy)
> @@ -31,6 +31,7 @@
>  #include "config.h"
>  #include "ScriptCallStack.h"
>  
> +#include "ScriptScope.h"
>  #include "ScriptController.h"
>  #include "ScriptDebugServer.h"
>  #include "V8Binding.h"
> @@ -41,8 +42,6 @@
>  
>  namespace WebCore {
>  
> -v8::Persistent<v8::Context> ScriptCallStack::s_utilityContext;
> -
>  ScriptCallStack* ScriptCallStack::create(const v8::Arguments& arguments, unsigned skipArgumentCount) {
>      String sourceName;
>      int sourceLineNumber;
> @@ -54,9 +53,22 @@ ScriptCallStack* ScriptCallStack::create
>  
>  bool ScriptCallStack::callLocation(String* sourceName, int* sourceLineNumber, String* functionName)
>  {
> -    if (!topStackFrame(*sourceName, *sourceLineNumber, *functionName))
> -        return false;
> -    *sourceLineNumber += 1;
> +    v8::HandleScope scope;
> +    v8::Context::Scope contextScope(v8::Context::GetCurrent());
> +    v8::Handle<v8::StackTrace> stackTrace(v8::StackTrace::CurrentStackTrace(1));
> +    if (stackTrace.IsEmpty())
> +        return false;
> +    if (stackTrace->GetFrameCount() <= 0) {
> +        // Fallback to setting lineNumber to 0, and source and function name to "undefined".
> +        *sourceName = toWebCoreString(v8::Undefined());
> +        *sourceLineNumber = 0;
> +        *functionName = toWebCoreString(v8::Undefined());
> +        return true;
> +    }
> +    v8::Handle<v8::StackFrame> frame = stackTrace->GetFrame(0);
> +    *sourceName = toWebCoreString(frame->GetScriptName());
> +    *sourceLineNumber = frame->GetLineNumber();
> +    *functionName = toWebCoreString(frame->GetFunctionName());
>      return true;
>  }
>  
> @@ -78,66 +90,13 @@ const ScriptCallFrame& ScriptCallStack::
>      return m_lastCaller;
>  }
>  
> -// Create the utility context for holding JavaScript functions used internally
> -// which are not visible to JavaScript executing on the page.
> -void ScriptCallStack::createUtilityContext()
> +bool ScriptCallStack::stackTrace(int frameLimit, ScriptState* state, ScriptArray& stackTrace)
>  {
> -    ASSERT(s_utilityContext.IsEmpty());
> -
> -    v8::HandleScope scope;
> -    v8::Handle<v8::ObjectTemplate> globalTemplate = v8::ObjectTemplate::New();
> -    s_utilityContext = v8::Context::New(0, globalTemplate);
> -    v8::Context::Scope contextScope(s_utilityContext);
> -
> -    // 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*, topStackFrame,
> -        ("function topStackFrame(exec_state) {"
> -        "  if (!exec_state.frameCount())"
> -        "      return undefined;"
> -        "  var frame = exec_state.frame(0);"
> -        "  var func = frame.func();"
> -        "  var scriptName;"
> -        "  if (func.resolved() && func.script())"
> -        "      scriptName = func.script().name();"
> -        "  return [scriptName, frame.sourceLine(), (func.name() || func.inferredName())];"
> -        "}"));
> -    v8::Script::Compile(v8::String::New(topStackFrame))->Run();
> -}
> -
> -bool ScriptCallStack::topStackFrame(String& sourceName, int& lineNumber, String& functionName)
> -{
> -    v8::HandleScope scope;
> -    v8::Handle<v8::Context> v8UtilityContext = utilityContext();
> -    if (v8UtilityContext.IsEmpty())
> -        return false;
> -    v8::Context::Scope contextScope(v8UtilityContext);
> -    v8::Handle<v8::Function> topStackFrame;
> -    topStackFrame = v8::Local<v8::Function>::Cast(v8UtilityContext->Global()->Get(v8::String::New("topStackFrame")));
> -    if (topStackFrame.IsEmpty())
> -        return false;
> -    v8::Handle<v8::Value> value = v8::Debug::Call(topStackFrame);
> -    if (value.IsEmpty())
> -        return false;
> -    // If there is no top stack frame, we still return success, but fill the input params with defaults.
> -    if (value->IsUndefined()) {
> -      // Fallback to setting lineNumber to 0, and source and function name to "undefined".
> -      sourceName = toWebCoreString(value);
> -      lineNumber = 0;
> -      functionName = toWebCoreString(value);
> -      return true;
> -    }
> -    if (!value->IsArray())
> -        return false;
> -    v8::Local<v8::Object> jsArray = value->ToObject();
> -    v8::Local<v8::Value> sourceNameValue = jsArray->Get(0);
> -    v8::Local<v8::Value> lineNumberValue = jsArray->Get(1);
> -    v8::Local<v8::Value> functionNameValue = jsArray->Get(2);
> -    if (sourceNameValue.IsEmpty() || lineNumberValue.IsEmpty() || functionNameValue.IsEmpty())
> +    ScriptScope scope(state);
> +    v8::Handle<v8::StackTrace> trace(v8::StackTrace::CurrentStackTrace(frameLimit));
> +    if (trace.IsEmpty() || !trace->GetFrameCount())
>          return false;
> -    sourceName = toWebCoreString(sourceNameValue);
> -    lineNumber = lineNumberValue->Int32Value();
> -    functionName = toWebCoreString(functionNameValue);
> +    stackTrace = ScriptArray(state, trace->AsArray());
>      return true;
>  }
>  
> Index: WebCore/bindings/v8/ScriptCallStack.h
> ===================================================================
> --- WebCore/bindings/v8/ScriptCallStack.h	(revision 59174)
> +++ WebCore/bindings/v8/ScriptCallStack.h	(working copy)
> @@ -31,6 +31,7 @@
>  #ifndef ScriptCallStack_h
>  #define ScriptCallStack_h
>  
> +#include "ScriptArray.h"
>  #include "ScriptCallFrame.h"
>  #include "ScriptState.h"
>  #include "ScriptValue.h"
> @@ -47,7 +48,16 @@ public:
>      static ScriptCallStack* create(const v8::Arguments&, unsigned skipArgumentCount = 0);
>      ~ScriptCallStack();
>  
> -    static bool callLocation(String* sourceName, int* sourceLineNumber, String* functionName);
> +    // 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);
>  
>      const ScriptCallFrame& at(unsigned) const;
>      // FIXME: implement retrieving and storing call stack trace
> @@ -59,30 +69,10 @@ public:
>  private:
>      ScriptCallStack(const v8::Arguments& arguments, unsigned skipArgumentCount, String sourceName, int sourceLineNumber, String funcName);
>  
> -    // 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 stack overflow in the underlying JavaScript implementation, handling
> -    // of such exception is up to the caller.
> -    static bool topStackFrame(String& sourceName, int& lineNumber, String& functionName);
> -
> -    static void createUtilityContext();
> -
> -     // Returns a local handle of the utility context.
> -    static v8::Local<v8::Context> utilityContext()
> -    {
> -      if (s_utilityContext.IsEmpty())
> -          createUtilityContext();
> -      return v8::Local<v8::Context>::New(s_utilityContext);
> -    }
> +    static bool callLocation(String* sourceName, int* sourceLineNumber, String* functionName);
>  
>      ScriptCallFrame m_lastCaller;
>      ScriptState* m_scriptState;
> -
> -    // Utility context holding JavaScript functions used internally.
> -    static v8::Persistent<v8::Context> s_utilityContext;
>  };
>  
>  } // namespace WebCore
> Index: WebCore/inspector/TimelineRecordFactory.cpp
> ===================================================================
> --- WebCore/inspector/TimelineRecordFactory.cpp	(revision 59174)
> +++ WebCore/inspector/TimelineRecordFactory.cpp	(working copy)
> @@ -49,14 +49,9 @@ ScriptObject TimelineRecordFactory::crea
>      ScriptObject record = frontend->newScriptObject();
>      record.set("startTime", startTime);
>  
> -    String sourceName;
> -    int sourceLineNumber;
> -    String functionName;
> -    if (ScriptCallStack::callLocation(&sourceName, &sourceLineNumber, &functionName) && sourceName != "undefined") {
> -        record.set("callerScriptName", sourceName);
> -        record.set("callerScriptLine", sourceLineNumber);
> -        record.set("callerFunctionName", functionName);
> -    }
> +    ScriptArray stackTrace;
> +    if (ScriptCallStack::stackTrace(5, frontend->scriptState(), stackTrace))
> +        record.set("stackTrace", stackTrace);
>      return record;
>  }
>  
> Index: WebCore/inspector/front-end/TimelinePanel.js
> ===================================================================
> --- WebCore/inspector/front-end/TimelinePanel.js	(revision 59174)
> +++ WebCore/inspector/front-end/TimelinePanel.js	(working copy)
> @@ -800,8 +800,7 @@ WebInspector.TimelinePanel.FormattedReco
>      this._selfTime = this.endTime - this.startTime;
>      this._lastChildEndTime = this.endTime;
>      this.originalRecordForTests = record;
> -    this.callerScriptName = record.callerScriptName;
> -    this.callerScriptLine = record.callerScriptLine;
> +    this.stackTrace = record.stackTrace;
>      this.totalHeapSize = record.totalHeapSize;
>      this.usedHeapSize = record.usedHeapSize;
>  
> @@ -830,8 +829,11 @@ WebInspector.TimelinePanel.FormattedReco
>      } else if (record.type === recordTypes.TimerFire) {
>          var timerInstalledRecord = timerRecords[record.data.timerId];
>          if (timerInstalledRecord) {
> -            this.callSiteScriptName = timerInstalledRecord.callerScriptName;
> -            this.callSiteScriptLine = timerInstalledRecord.callerScriptLine;
> +            if (timerInstalledRecord.stackTrace) {
> +                var callSite = timerInstalledRecord.stackTrace[0];            
> +                this.callSiteScriptName = callSite.scriptName;
> +                this.callSiteScriptLine = callSite.lineNumber;
> +            }
>              this.timeout = timerInstalledRecord.timeout;
>              this.singleShot = timerInstalledRecord.singleShot;
>          }
> @@ -931,8 +933,10 @@ WebInspector.TimelinePanel.FormattedReco
>          if (this.data.scriptName && this.type !== recordTypes.FunctionCall)
>              contentHelper._appendLinkRow("Function Call", this.data.scriptName, this.data.scriptLine);
>  
> -        if (this.callerScriptName && this.type !== recordTypes.GCEvent)
> -            contentHelper._appendLinkRow("Caller", this.callerScriptName, this.callerScriptLine);
> +        if (this.stackTrace && this.type !== recordTypes.GCEvent) {
> +            var callSite = this.stackTrace[0];
> +            contentHelper._appendLinkRow("Caller", callSite.scriptName, callSite.lineNumber);
> +        }
>  
>          if (this.usedHeapSize)
>              contentHelper._appendTextRow("Used Heap Size", WebInspector.UIString("%s of %s", Number.bytesToString(this.usedHeapSize), Number.bytesToString(this.totalHeapSize)));
> @@ -955,7 +959,8 @@ WebInspector.TimelinePanel.FormattedReco
>                  return record.data.width + "\u2009\u00d7\u2009" + record.data.height;
>              case WebInspector.TimelineAgent.RecordType.TimerInstall:
>              case WebInspector.TimelineAgent.RecordType.TimerRemove:
> -                return this.callerScriptName ? WebInspector.linkifyResourceAsNode(this.callerScriptName, "scripts", this.callerScriptLine, "", "") : record.data.timerId;
> +                var callSite = this.stackTrace;
> +                return callSite ? WebInspector.linkifyResourceAsNode(callSite.scriptName, "scripts", callSite.lineNumber, "", "") : record.data.timerId;
>              case WebInspector.TimelineAgent.RecordType.ParseHTML:
>              case WebInspector.TimelineAgent.RecordType.RecalculateStyles:
>                  return this.callerScriptName ? WebInspector.linkifyResourceAsNode(this.callerScriptName, "scripts", this.callerScriptLine, "", "") : null;
> Index: WebKit/chromium/DEPS
> ===================================================================
> --- WebKit/chromium/DEPS	(revision 59174)
> +++ WebKit/chromium/DEPS	(working copy)
> @@ -32,7 +32,7 @@
>  
>  vars = {
>    'chromium_svn': 'http://src.chromium.org/svn/trunk/src',
> -  'chromium_rev': '46805',
> +  'chromium_rev': '46991',
>  
>    'pthreads-win32_rev': '26716',
>  }
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 59177)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> [phone]  Jaime Yap  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test expectations changed due to new stackTrace field on timeline records.
> +        https://bugs.webkit.org/show_bug.cgi?id=37502
> +
> +        * inspector/timeline-test.js:
> +        * platform/chromium-win/inspector/timeline-event-dispatch-expected.txt:
> +        * platform/chromium-win/inspector/timeline-mark-timeline-expected.txt:
> +        * platform/chromium-win/inspector/timeline-network-resource-expected.txt:
> +        * platform/chromium-win/inspector/timeline-paint-expected.txt:
> +        * platform/chromium-win/inspector/timeline-parse-html-expected.txt:
> +
>  2010-05-11  Dimitri Glazkov  <[e-mail]>
>  
>          Reviewed by Darin Adler.
> Index: LayoutTests/inspector/timeline-test.js
> ===================================================================
> --- LayoutTests/inspector/timeline-test.js	(revision 59174)
> +++ LayoutTests/inspector/timeline-test.js	(working copy)
> @@ -8,9 +8,7 @@ var timelineNonDeterministicProps = { 
>      identifier : 1,
>      startTime : 1,
>      width : 1,
> -    callerScriptName: 1,
> -    callerScriptLine: 1,
> -    callerFunctionName: 1,
> +    stackTrace: 1,
>      url : 1,
>      usedHeapSize: 1,
>      totalHeapSize: 1,
> Index: LayoutTests/platform/chromium-win/inspector/timeline-event-dispatch-expected.txt
> ===================================================================
> --- LayoutTests/platform/chromium-win/inspector/timeline-event-dispatch-expected.txt	(revision 59174)
> +++ LayoutTests/platform/chromium-win/inspector/timeline-event-dispatch-expected.txt	(working copy)
> @@ -3,9 +3,7 @@ Tests the Timeline API instrumentation o
>  Test Mouse Target
>  EventDispatch Properties:
>  + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
>  + data : {
>  +- type : mousedown
>  + }
> Index: LayoutTests/platform/chromium-win/inspector/timeline-mark-timeline-expected.txt
> ===================================================================
> --- LayoutTests/platform/chromium-win/inspector/timeline-mark-timeline-expected.txt	(revision 59174)
> +++ LayoutTests/platform/chromium-win/inspector/timeline-mark-timeline-expected.txt	(working copy)
> @@ -2,9 +2,7 @@ Tests the Timeline API mark feature
>  
>  MarkTimeline Properties:
>  + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
>  + data : {
>  +- message : MARK TIMELINE
>  + }
> Index: LayoutTests/platform/chromium-win/inspector/timeline-network-resource-expected.txt
> ===================================================================
> --- LayoutTests/platform/chromium-win/inspector/timeline-network-resource-expected.txt	(revision 59174)
> +++ LayoutTests/platform/chromium-win/inspector/timeline-network-resource-expected.txt	(working copy)
> @@ -3,9 +3,7 @@ Tests the Timeline API instrumentation o
>  
>  ResourceSendRequest Properties:
>  + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
>  + data : {
>  +- identifier : * DEFINED *
>  +- url : * DEFINED *
> Index: LayoutTests/platform/chromium-win/inspector/timeline-paint-expected.txt
> ===================================================================
> --- LayoutTests/platform/chromium-win/inspector/timeline-paint-expected.txt	(revision 59174)
> +++ LayoutTests/platform/chromium-win/inspector/timeline-paint-expected.txt	(working copy)
> @@ -2,9 +2,7 @@ Tests the Timeline API instrumentation o
>  
>  Paint Properties:
>  + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
>  + data : {
>  +- x : 0
>  +- y : 0
> Index: LayoutTests/platform/chromium-win/inspector/timeline-parse-html-expected.txt
> ===================================================================
> --- LayoutTests/platform/chromium-win/inspector/timeline-parse-html-expected.txt	(revision 59174)
> +++ LayoutTests/platform/chromium-win/inspector/timeline-parse-html-expected.txt	(working copy)
> @@ -2,9 +2,7 @@ Tests the Timeline API instrumentation o
>  
>  ParseHTML Properties:
>  + startTime : * DEFINED *
> -+ callerScriptName : * DEFINED *
> -+ callerScriptLine : * DEFINED *
> -+ callerFunctionName : * DEFINED *
> ++ stackTrace : * DEFINED *
>  + data : {
>  +- length : 9
>  +- startLine : 0

R-ing this obsolete patch so that it no longer shows up in commit queue.

Comment on attachment 56252 [details]
undoes the DEPS change

Changeset 59570 (see https://bugs.webkit.org/show_bug.cgi?id=39086) is going to conflict with this change when eseidel pumps the commit queue.

Argg. I am C- and R- ing again. Will update the patch after rebasing.

Can someone land this by hand when I update the patch? Its been stomped on twice due to commit latency :).

Created attachment 56398 [details]
Rebased against r59670

Ill give this another shot.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/timeline-test.js
	M	LayoutTests/platform/chromium-win/inspector/timeline-event-dispatch-expected.txt
	M	LayoutTests/platform/chromium-win/inspector/timeline-mark-timeline-expected.txt
	M	LayoutTests/platform/chromium-win/inspector/timeline-network-resource-expected.txt
	M	LayoutTests/platform/chromium-win/inspector/timeline-paint-expected.txt
	M	LayoutTests/platform/chromium-win/inspector/timeline-parse-html-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/ScriptCallStack.cpp
	M	WebCore/bindings/js/ScriptCallStack.h
	M	WebCore/bindings/v8/ScriptArray.h
	M	WebCore/bindings/v8/ScriptCallStack.cpp
	M	WebCore/bindings/v8/ScriptCallStack.h
	M	WebCore/inspector/TimelineRecordFactory.cpp
	M	WebCore/inspector/front-end/TimelinePanel.js
Committed r60083

[WebKit Review Bot]

http://trac.webkit.org/changeset/60083 might have broken Chromium Linux Release