DevTools: Support asynchronous call stacks on backend.

DevTools: Support asynchronous call stacks on backend.

Track asynchronous call stacks for setTimeout/setInterval and requestAnimationFrame on the backend.

BUG=272416, 326479
R=pfeldman@chromium.org, yurys@chromium.org, caseq@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163266

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163353

[aandrey, 2013-11-18 10:52:25]

Async call stacks are off by default.
Added Debugger.enableAsyncCallStacks protocol command.

[yurys, 2013-11-20 01:23:43]

Please provide some overview of the implementation in the description.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
File Source/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
Source/core/inspector/AsyncCallStackTracker.h:46: friend class
AsyncCallStackTracker;
It seems that this class and the iterator should be nested in 
AsyncCallStackTracker. This way you wouldn't need this friendship and could make
all fields public.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
Source/core/inspector/AsyncCallStackTracker.h:48: explicit AsyncCallStack(const
RefPtr<AsyncCallStack>& next);
const RefPtr<AsyncCallStack>& ->  AsyncCallStack*

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
Source/core/inspector/AsyncCallStackTracker.h:62: AsyncCallStackIterator(const
RefPtr<AsyncCallStack>& head, const RefPtr<AsyncCallStack>& tail);
Same thing, please use raw pointers instead of const RefPtr& or PassRefPtr if
you are releasing ref on the caller's side.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
Source/core/inspector/AsyncCallStackTracker.h:90: const RefPtr<AsyncCallStack>&
requestAsyncCallStack();
Should return raw pointer or PassRefPtr instead.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Ins...
Source/core/inspector/InspectorDebuggerAgent.cpp:969: RefPtr<CallFrame> last =
CallFrame::runtimeCast(lastCallFrames->asArray()->get(lastCallFrames->length() -
1));
Wouldn't lastCallFrames->get(lastCallFrames->length() - 1) work?

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
Source/devtools/protocol.json:3254: "name": "enableAsyncCallStacks",
setAsyncCallStackDepth?

[loislo, 2013-11-20 01:39:17]

https://codereview.chromium.org/74063002/diff/90001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/74063002/diff/90001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:215: v8::HandleScope scope(m_isolate);
it is not clear for me why do you need this async part.
If I understand things properly the debugger agent will get a call when the code

will setTimeout so you will be able to catch the stack the same way as timeline
does that.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorDebuggerAgent.cpp (left):

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Ins...
Source/core/inspector/InspectorDebuggerAgent.cpp:445: void
InspectorDebuggerAgent::getBacktrace(ErrorString* errorString,
RefPtr<Array<TypeBuilder::Debugger::CallFrame> >& callFrames)
please move these changes  into another patch.

[yurys, 2013-11-20 14:07:57]

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
Source/devtools/protocol.json:2962: { "name": "asyncCallFrames", "type":
"array", "items": { "$ref": "CallFrame" }, "optional": true, "hidden": true,
"description": "Asynchronous call stack, if any." }
Why is it property on CallFrame, I believe it should be another parameter on
"paused" event.

[aandrey, 2013-11-20 15:20:01]

PTAL

https://codereview.chromium.org/74063002/diff/90001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/74063002/diff/90001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptDebugServer.cpp:215: v8::HandleScope scope(m_isolate);
On 2013/11/20 01:39:18, loislo wrote:
> it is not clear for me why do you need this async part.
> If I understand things properly the debugger agent will get a call when the
code
> 
> will setTimeout so you will be able to catch the stack the same way as
timeline
> does that.

There are two kind of stacks: console stack (Console.CallFrame in protocol.json)
and debugger stack (Debugger.CallFrame).

Timeline catches the console stack, while I need the debugger stack.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
File Source/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
Source/core/inspector/AsyncCallStackTracker.h:46: friend class
AsyncCallStackTracker;
On 2013/11/20 01:23:43, yurys wrote:
> It seems that this class and the iterator should be nested in 
> AsyncCallStackTracker. This way you wouldn't need this friendship and could
make
> all fields public.

Done.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
Source/core/inspector/AsyncCallStackTracker.h:48: explicit AsyncCallStack(const
RefPtr<AsyncCallStack>& next);
On 2013/11/20 01:23:43, yurys wrote:
> const RefPtr<AsyncCallStack>& ->  AsyncCallStack*

Done.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
Source/core/inspector/AsyncCallStackTracker.h:62: AsyncCallStackIterator(const
RefPtr<AsyncCallStack>& head, const RefPtr<AsyncCallStack>& tail);
On 2013/11/20 01:23:43, yurys wrote:
> Same thing, please use raw pointers instead of const RefPtr& or PassRefPtr if
> you are releasing ref on the caller's side.

Done.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Asy...
Source/core/inspector/AsyncCallStackTracker.h:90: const RefPtr<AsyncCallStack>&
requestAsyncCallStack();
On 2013/11/20 01:23:43, yurys wrote:
> Should return raw pointer or PassRefPtr instead.

Done.

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorDebuggerAgent.cpp (left):

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Ins...
Source/core/inspector/InspectorDebuggerAgent.cpp:445: void
InspectorDebuggerAgent::getBacktrace(ErrorString* errorString,
RefPtr<Array<TypeBuilder::Debugger::CallFrame> >& callFrames)
On 2013/11/20 01:39:18, loislo wrote:
> please move these changes  into another patch.

Extracted to https://codereview.chromium.org/77173006

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://codereview.chromium.org/74063002/diff/90001/Source/core/inspector/Ins...
Source/core/inspector/InspectorDebuggerAgent.cpp:969: RefPtr<CallFrame> last =
CallFrame::runtimeCast(lastCallFrames->asArray()->get(lastCallFrames->length() -
1));
On 2013/11/20 01:23:43, yurys wrote:
> Wouldn't lastCallFrames->get(lastCallFrames->length() - 1) work?

Nope. TypeBuilder::Array::get is protected as well as the parent's
JSONArrayBase::get. Thus we need to cast to JSONArray.

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
Source/devtools/protocol.json:2962: { "name": "asyncCallFrames", "type":
"array", "items": { "$ref": "CallFrame" }, "optional": true, "hidden": true,
"description": "Asynchronous call stack, if any." }
On 2013/11/20 14:07:57, yurys wrote:
> Why is it property on CallFrame, I believe it should be another parameter on
> "paused" event.

Here is the list of commands and events that return debugger callFrames:

Debugger.setScriptSource
Debugger.restartFrame
Debugger.getBacktrace
Debugger.paused

In all of them we'd like to send async stacks.

Besides, should you want to return it separately, it will be of type
{Array.<Array.<CallFrame>>} - quite complex and I doubt our code generator
supports this at the moment.

Ideally we should have created a Debugger.StackTrace class as we did for the
Console.StackTrace. If we had done it, this is where the async call frames would
live.

To preserve the backward compatibility, this is the best solution I could think
of.

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
Source/devtools/protocol.json:3254: "name": "enableAsyncCallStacks",
On 2013/11/20 01:23:43, yurys wrote:
> setAsyncCallStackDepth?

Done.

[aandrey, 2013-11-20 15:48:28]

> Please provide some overview of the implementation in the description.

Updated the description.

[vsevik, 2013-11-27 13:58:58]

I think an implementation storing an array of RefPtr<AsyncCallStack> for each
timer will have a comparable memory consumption and significantly simpler
AsyncCallStackTracker implementation.
Did you consider it?

[aandrey, 2013-11-27 16:02:14]

On 2013/11/27 13:58:58, vsevik wrote:
> I think an implementation storing an array of RefPtr<AsyncCallStack> for each
> timer will have a comparable memory consumption and significantly simpler
> AsyncCallStackTracker implementation.
> Did you consider it?

I did, and I did not find it significantly simpler, if simpler at all.
Besides the memory consumption and would be slower.

[yurys, 2013-11-28 09:18:15]

https://codereview.chromium.org/74063002/diff/190001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/74063002/diff/190001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:183: // Defer modifying the path
count.
Why do you need to defer here, can't we simply check that
m_currentAsyncCallStack.m_pathCount == 0 and delete m_currentAsyncCallStack (in
fact we won't need to do this check explicitly as m_currentAsyncCallStack should
have only on refCount == 1 if its m_pathCount == 0).

https://codereview.chromium.org/74063002/diff/190001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:192: if (current->m_pathCount <=
0) {
If m_pathCount == 0 we should dispose of current, shouldn't we?

https://codereview.chromium.org/74063002/diff/190001/Source/core/inspector/In...
File Source/core/inspector/InspectorDebuggerAgent.cpp (right):

https://codereview.chromium.org/74063002/diff/190001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:663:
scriptDebugServer().requestAsyncCallFrames();
Why do we need this roundtrip into the debug server, it seems that we should be
able to ge current call stack using synchronous call to createScriptCallStack or
its modified version if you need ScriptValues. Also it is not clear whether we
should hold live frame values or just a snapshot.

[vsevik, 2013-11-28 10:17:40]

On 2013/11/27 16:02:14, aandrey wrote:
> On 2013/11/27 13:58:58, vsevik wrote:
> > I think an implementation storing an array of RefPtr<AsyncCallStack> for
each
> > timer will have a comparable memory consumption and significantly simpler
> > AsyncCallStackTracker implementation.
> > Did you consider it?
> 
> I did, and I did not find it significantly simpler, if simpler at all.
> Besides the memory consumption and would be slower.

You would not need modifyPathCount method at all and it is complex: it is
already causing reviewers questions :).
Memory consumption: 4 RefPtr objects per timer vs. 1 RefPtr object per timer and
1 extra int field per call stack - I don't see a clear benefit here.
Why would it be slower? Creating 4 RefPtr objects instead of traversing the tree
to update path count doesn't seem to be an issue.

[yurys, 2013-11-28 10:30:33]

On 2013/11/28 10:17:40, vsevik wrote:
> On 2013/11/27 16:02:14, aandrey wrote:
> > On 2013/11/27 13:58:58, vsevik wrote:
> > > I think an implementation storing an array of RefPtr<AsyncCallStack> for
> each
> > > timer will have a comparable memory consumption and significantly simpler
> > > AsyncCallStackTracker implementation.
> > > Did you consider it?
> > 
> > I did, and I did not find it significantly simpler, if simpler at all.
> > Besides the memory consumption and would be slower.
> 
> You would not need modifyPathCount method at all and it is complex: it is
> already causing reviewers questions :).
> Memory consumption: 4 RefPtr objects per timer vs. 1 RefPtr object per timer
and
> 1 extra int field per call stack - I don't see a clear benefit here.
> Why would it be slower? Creating 4 RefPtr objects instead of traversing the
tree
> to update path count doesn't seem to be an issue.

I agree that Vsevolod's approach sounds more clear as it wouldn't require manual
reference counting. Memory overhead should be negligible too.

[yurys, 2013-11-28 10:35:40]

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/74063002/diff/90001/Source/devtools/protocol....
Source/devtools/protocol.json:2962: { "name": "asyncCallFrames", "type":
"array", "items": { "$ref": "CallFrame" }, "optional": true, "hidden": true,
"description": "Asynchronous call stack, if any." }
On 2013/11/20 15:20:02, aandrey wrote:
> On 2013/11/20 14:07:57, yurys wrote:
> > Why is it property on CallFrame, I believe it should be another parameter on
> > "paused" event.
> 
> Here is the list of commands and events that return debugger callFrames:
> 
> Debugger.setScriptSource
> Debugger.restartFrame
> Debugger.getBacktrace
> Debugger.paused
> 
> In all of them we'd like to send async stacks.
> 
I'd rather add explicit async call stack property there. asyncCallFrames
property on CallFrame looks very confusing.


> Besides, should you want to return it separately, it will be of type
> {Array.<Array.<CallFrame>>} - quite complex and I doubt our code generator
> supports this at the moment.
> 
> Ideally we should have created a Debugger.StackTrace class as we did for the
> Console.StackTrace. If we had done it, this is where the async call frames
would
> live.
> 
> To preserve the backward compatibility, this is the best solution I could
think
> of.

To preserve backward compatibility you could use Debugger.StackTrace for async
call stacks only.

[aandrey, 2013-11-29 13:35:38]

Re-implemented using Deque<RefPtr<...>> to store the async traces.

Removed protocol.json bindings from this patch - it's an independent refactoring
work that will be coming in next patches.

PTAL.

[yurys, 2013-12-03 13:41:08]

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:211:
listener->didRequestAsyncCallFrames(currentCallFrame());
I believe we can implement this synchronously by passing a callback to
requestAsyncCallFrames instead of adding new method to the ScriptDebugListener.
You should be able to store the callback in a field and call it from inside
handleAsyncCallFrames.

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:404: if (isPaused())
How can we get here not being paused?

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptDebugServer.h (right):

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.h:83: bool requestAsyncCallFrames();
It should be requestCallStack, from the ScriptDebugServer's stand point I don't
see why the call stack is async.

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:178: while
(trace->m_callStacks.size() > maxDepth)
Isn't it just 'if'?

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:46:
WTF_MAKE_NONCOPYABLE(AsyncCallStackTracker); WTF_MAKE_FAST_ALLOCATED;
Why does it need to be marked as WTF_MAKE_FAST_ALLOCATED ?

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:48: struct AsyncCallStack : public
RefCounted<AsyncCallStack> {
You can leave only forward declaration of the structs here in the header and
move actual declaration to the .cpp

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:56: struct AsyncCallTrace : public
RefCounted<AsyncCallTrace> {
Maybe AsyncCallStackChain or AsyncCallChain?

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:71: AsyncCallTraceIterator(const
AsyncCallTrace& t) : m_callStacks(t.m_callStacks) { }
How is this iterator going to be used? Wouldn't it be more clear to use
AsyncCallTrace directly?

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:77: inline bool isEnabled() const
{ return m_maxAsyncCallStackDepth; }
Please remove 'inline' as it doesn't add anything.

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:90: void
didRequestAsyncCallFrames(ScriptValue callFrames);
setCurrentCallStack ?

[aandrey, 2013-12-04 12:45:46]

Made the request synchronous.
PTAL.

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:211:
listener->didRequestAsyncCallFrames(currentCallFrame());
On 2013/12/03 13:41:08, yurys wrote:
> I believe we can implement this synchronously by passing a callback to
> requestAsyncCallFrames instead of adding new method to the
ScriptDebugListener.
> You should be able to store the callback in a field and call it from inside
> handleAsyncCallFrames.

Done.

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptDebugServer.h (right):

https://codereview.chromium.org/74063002/diff/240001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.h:83: bool requestAsyncCallFrames();
On 2013/12/03 13:41:08, yurys wrote:
> It should be requestCallStack, from the ScriptDebugServer's stand point I
don't
> see why the call stack is async.

Done.

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:178: while
(trace->m_callStacks.size() > maxDepth)
On 2013/12/03 13:41:08, yurys wrote:
> Isn't it just 'if'?

The max depth limit can be changed at any time via the protocol API.
Here we shrink the traces lazily.

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:46:
WTF_MAKE_NONCOPYABLE(AsyncCallStackTracker); WTF_MAKE_FAST_ALLOCATED;
On 2013/12/03 13:41:08, yurys wrote:
> Why does it need to be marked as WTF_MAKE_FAST_ALLOCATED ?

removed

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:48: struct AsyncCallStack : public
RefCounted<AsyncCallStack> {
On 2013/12/03 13:41:08, yurys wrote:
> You can leave only forward declaration of the structs here in the header and
> move actual declaration to the .cpp

its became part of public API

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:56: struct AsyncCallTrace : public
RefCounted<AsyncCallTrace> {
On 2013/12/03 13:41:08, yurys wrote:
> Maybe AsyncCallStackChain or AsyncCallChain?

renamed to AsyncCallChain

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:71: AsyncCallTraceIterator(const
AsyncCallTrace& t) : m_callStacks(t.m_callStacks) { }
On 2013/12/03 13:41:08, yurys wrote:
> How is this iterator going to be used? Wouldn't it be more clear to use
> AsyncCallTrace directly?

Done.

https://codereview.chromium.org/74063002/diff/240001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:77: inline bool isEnabled() const
{ return m_maxAsyncCallStackDepth; }
On 2013/12/03 13:41:08, yurys wrote:
> Please remove 'inline' as it doesn't add anything.

Done.

[yurys, 2013-12-04 15:25:51]

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector/debugger-test.js (right):

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:157: asyncStackTrace =
asyncStackTrace.asyncStackTrace;
initiatorStackTrace would be a better name for asyncStackTrace, wdyt?

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/inspector/de...
File LayoutTests/inspector/debugger/async-callstack-expected.txt (right):

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/inspector/de...
LayoutTests/inspector/debugger/async-callstack-expected.txt:22: Call stack:
It is not obvious from this output what is the expected result, hence there is a
good chance the test will be silently rebaselined in case of text failure.

https://codereview.chromium.org/74063002/diff/310001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/74063002/diff/310001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:343: return
ScriptValue(v8::Null(m_isolate), m_isolate);
Why not ScriptValue() ? As additional benefit you wouldn't need isObject() check
on the caller's side/

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:51:
dataLogF("AsyncCallStack::AsyncCallStack() %u\n", ++totalAsyncCallStacks);
What's the point in this dataLogF, why not printf?

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:86: if (!isEnabled() ||
!validateCallFrames(callFrames))
ASSERT(isEnabled()) as you already check it on the caller's side.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:96: if (!isEnabled() || timerId
<= 0)
How can timerId be <= 0 ? Maybe ASSERT as in didInstallTimer?

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:51: const ScriptValue&
callFrames() const { return m_callFrames; }
Just return it by value.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:62: const AsyncCallStackVector&
callStacks() const { return m_callStacks; }
ditto.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:74: void didInstallTimer(int
timerId, bool singleShot, ScriptValue callFrames);
const ScriptValue&

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:78: void
didRequestAnimationFrame(int callbackId, ScriptValue callFrames);
const ScriptValue&

[aandrey, 2013-12-04 15:57:25]

PTAL

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector/debugger-test.js (right):

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:157: asyncStackTrace =
asyncStackTrace.asyncStackTrace;
On 2013/12/04 15:25:52, yurys wrote:
> initiatorStackTrace would be a better name for asyncStackTrace, wdyt?

for which one? anyway "initiator" would seem confusing to me in place of any of
these

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/inspector/de...
File LayoutTests/inspector/debugger/async-callstack-expected.txt (right):

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/inspector/de...
LayoutTests/inspector/debugger/async-callstack-expected.txt:22: Call stack:
On 2013/12/04 15:25:52, yurys wrote:
> It is not obvious from this output what is the expected result, hence there is
a
> good chance the test will be silently rebaselined in case of text failure.

This output is the expected result.
What do you suggest exactly?

https://codereview.chromium.org/74063002/diff/310001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/74063002/diff/310001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:343: return
ScriptValue(v8::Null(m_isolate), m_isolate);
On 2013/12/04 15:25:52, yurys wrote:
> Why not ScriptValue() ? As additional benefit you wouldn't need isObject()
check
> on the caller's side/

Because this is the API of this method (see the same value returned above), and
our code does check for isNull() in the call sites.
I don't want to change the existing API in this patch, since it seems to be
error prone.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:51:
dataLogF("AsyncCallStack::AsyncCallStack() %u\n", ++totalAsyncCallStacks);
On 2013/12/04 15:25:52, yurys wrote:
> What's the point in this dataLogF, why not printf?

it's the same essentially. plus there is a way to change stdout/stderr to a file

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:86: if (!isEnabled() ||
!validateCallFrames(callFrames))
On 2013/12/04 15:25:52, yurys wrote:
> ASSERT(isEnabled()) as you already check it on the caller's side.

Done.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:96: if (!isEnabled() || timerId
<= 0)
On 2013/12/04 15:25:52, yurys wrote:
> How can timerId be <= 0 ? Maybe ASSERT as in didInstallTimer?

Easy:
  window.clearTimeout(-5);

But we do want to pass the invalid ids to agents, for example to
DOMDebuggerAgent, to stop on clearTimeout, thus we can't filter this out in
DOMTimer.cpp

Timer ID in didInstallTimer comes from DOMTimer.cpp, thus should always be
valid.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:51: const ScriptValue&
callFrames() const { return m_callFrames; }
On 2013/12/04 15:25:52, yurys wrote:
> Just return it by value.

why? :)

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:74: void didInstallTimer(int
timerId, bool singleShot, ScriptValue callFrames);
On 2013/12/04 15:25:52, yurys wrote:
> const ScriptValue&

Done.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:78: void
didRequestAnimationFrame(int callbackId, ScriptValue callFrames);
On 2013/12/04 15:25:52, yurys wrote:
> const ScriptValue&

Done.

[yurys, 2013-12-05 11:37:47]

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector/debugger-test.js (right):

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:157: asyncStackTrace =
asyncStackTrace.asyncStackTrace;
On 2013/12/04 15:57:26, aandrey wrote:
> On 2013/12/04 15:25:52, yurys wrote:
> > initiatorStackTrace would be a better name for asyncStackTrace, wdyt?
> 
> for which one? anyway "initiator" would seem confusing to me in place of any
of
> these

For those which are asyncStackTrace at the moment, including protocol.

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/inspector/de...
File LayoutTests/inspector/debugger/async-callstack-expected.txt (right):

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/inspector/de...
LayoutTests/inspector/debugger/async-callstack-expected.txt:22: Call stack:
On 2013/12/04 15:57:26, aandrey wrote:
> On 2013/12/04 15:25:52, yurys wrote:
> > It is not obvious from this output what is the expected result, hence there
is
> a
> > good chance the test will be silently rebaselined in case of text failure.
> 
> This output is the expected result.
> What do you suggest exactly?

Having explicit SUCCESS: or FAIL: prefix in the output would make it clear that
the test started failing and rebaselining wouldn't help.

https://codereview.chromium.org/74063002/diff/310001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/74063002/diff/310001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptDebugServer.cpp:343: return
ScriptValue(v8::Null(m_isolate), m_isolate);
On 2013/12/04 15:57:26, aandrey wrote:
> On 2013/12/04 15:25:52, yurys wrote:
> > Why not ScriptValue() ? As additional benefit you wouldn't need isObject()
> check
> > on the caller's side/
> 
> Because this is the API of this method (see the same value returned above),
and
> our code does check for isNull() in the call sites.
> I don't want to change the existing API in this patch, since it seems to be
> error prone.

OK, I don't mind if this is addressed in another CL.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:51:
dataLogF("AsyncCallStack::AsyncCallStack() %u\n", ++totalAsyncCallStacks);
On 2013/12/04 15:57:26, aandrey wrote:
> On 2013/12/04 15:25:52, yurys wrote:
> > What's the point in this dataLogF, why not printf?
> 
> it's the same essentially. plus there is a way to change stdout/stderr to a
file

Then printf should be preferred, I don't see dataLogF to be used outside of some
basic classes in WTF.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:96: if (!isEnabled() || timerId
<= 0)
On 2013/12/04 15:57:26, aandrey wrote:
> On 2013/12/04 15:25:52, yurys wrote:
> > How can timerId be <= 0 ? Maybe ASSERT as in didInstallTimer?
> 
> Easy:
>   window.clearTimeout(-5);
> 
> But we do want to pass the invalid ids to agents, for example to
> DOMDebuggerAgent, to stop on clearTimeout, thus we can't filter this out in
> DOMTimer.cpp
> 
> Timer ID in didInstallTimer comes from DOMTimer.cpp, thus should always be
> valid.

Got it, thanks for explaining.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:51: const ScriptValue&
callFrames() const { return m_callFrames; }
On 2013/12/04 15:57:26, aandrey wrote:
> On 2013/12/04 15:25:52, yurys wrote:
> > Just return it by value.
> 
> why? :)

Because it is a bad idea in general to return something by reference (as the
object may go away leaving the reference in a broken state) and it is
discouraged by the style guide.

[aandrey, 2013-12-05 13:03:39]

PTAL.

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/http/tests/i...
File LayoutTests/http/tests/inspector/debugger-test.js (right):

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:157: asyncStackTrace =
asyncStackTrace.asyncStackTrace;
On 2013/12/05 11:37:48, yurys wrote:
> On 2013/12/04 15:57:26, aandrey wrote:
> > On 2013/12/04 15:25:52, yurys wrote:
> > > initiatorStackTrace would be a better name for asyncStackTrace, wdyt?
> > 
> > for which one? anyway "initiator" would seem confusing to me in place of any
> of
> > these
> 
> For those which are asyncStackTrace at the moment, including protocol.

Out of these two I like "async" more.
Moreover it took me some time before I understood what you mean by "initiator".
I think "async" will be more recognizable, given that some debuggers already use
this term, e.g. in VS it's "Async Call".

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/inspector/de...
File LayoutTests/inspector/debugger/async-callstack-expected.txt (right):

https://codereview.chromium.org/74063002/diff/310001/LayoutTests/inspector/de...
LayoutTests/inspector/debugger/async-callstack-expected.txt:22: Call stack:
On 2013/12/05 11:37:48, yurys wrote:
> On 2013/12/04 15:57:26, aandrey wrote:
> > On 2013/12/04 15:25:52, yurys wrote:
> > > It is not obvious from this output what is the expected result, hence
there
> is
> > a
> > > good chance the test will be silently rebaselined in case of text failure.
> > 
> > This output is the expected result.
> > What do you suggest exactly?
> 
> Having explicit SUCCESS: or FAIL: prefix in the output would make it clear
that
> the test started failing and rebaselining wouldn't help.
> 

Too hard to do.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:51:
dataLogF("AsyncCallStack::AsyncCallStack() %u\n", ++totalAsyncCallStacks);
On 2013/12/05 11:37:48, yurys wrote:
> On 2013/12/04 15:57:26, aandrey wrote:
> > On 2013/12/04 15:25:52, yurys wrote:
> > > What's the point in this dataLogF, why not printf?
> > 
> > it's the same essentially. plus there is a way to change stdout/stderr to a
> file
> 
> Then printf should be preferred, I don't see dataLogF to be used outside of
some
> basic classes in WTF.

Done.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.h (right):

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:51: const ScriptValue&
callFrames() const { return m_callFrames; }
On 2013/12/05 11:37:48, yurys wrote:
> On 2013/12/04 15:57:26, aandrey wrote:
> > On 2013/12/04 15:25:52, yurys wrote:
> > > Just return it by value.
> > 
> > why? :)
> 
> Because it is a bad idea in general to return something by reference (as the
> object may go away leaving the reference in a broken state) and it is
> discouraged by the style guide. 

Done.

https://codereview.chromium.org/74063002/diff/310001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.h:62: const AsyncCallStackVector&
callStacks() const { return m_callStacks; }
On 2013/12/04 15:25:52, yurys wrote:
> ditto.

Done.

[yurys, 2013-12-05 13:28:16]

lgtm

[commit-bot: I haz the power, 2013-12-05 13:30:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aandrey@chromium.org/74063002/340001

[commit-bot: I haz the power, 2013-12-05 14:59:24]

Change committed as 163266

[commit-bot: I haz the power, 2013-12-06 15:29:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aandrey@chromium.org/74063002/360001

[commit-bot: I haz the power, 2013-12-06 17:40:52]

Change committed as 163353