DevTools: Capture async stacks for event listeners.

DevTools: Capture async stacks for event listeners.

BUG=272416
R=yurys@chromium.org

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

[aandrey, 2013-12-12 15:43:41]

PTAL

[yurys, 2013-12-16 15:03:54]

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

https://codereview.chromium.org/114033002/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:713: if
(m_asyncCallStackTracker.isEnabled())
We should be able to avoid this check by setting debugger agent to
InstrumentingAgents only when async tracker is on. Do we have debugger
instrumentation for other events?

[aandrey, 2013-12-16 15:20:21]

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

https://codereview.chromium.org/114033002/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:713: if
(m_asyncCallStackTracker.isEnabled())
On 2013/12/16 15:03:55, yurys wrote:
> We should be able to avoid this check by setting debugger agent to
> InstrumentingAgents only when async tracker is on. Do we have debugger
> instrumentation for other events?

The Debugger agent can function both w/ and w/o async stack tracking feature.

This check here is just an optimization, so that we should not call to
scriptDebugServer unless needed.

[yurys, 2013-12-17 13:01:58]

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (left):

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:128:
ASSERT(!m_currentAsyncCallChain);
Why did you remove this assert? Can it fail in a wrong place?

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

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:61: void add(EventTarget*
eventTarget, const AtomicString& eventType, const EventListenerAsyncCallChain&
item)
addEventListenerData?

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:106: friend class
AsyncCallStackTracker;
All these field should be public since ExecutionContextData is an implementation
detail of AsyncCallStackTracker

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

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:718: {
 if (m_asyncCallStackTracker.isEnabled()) ?

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:724:
m_asyncCallStackTracker.didRemoveAllEventListeners(eventTarget);
ditto

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:729:
m_asyncCallStackTracker.willHandleEvent(eventTarget, eventType, listener,
useCapture);
ditto

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:734:
m_asyncCallStackTracker.didFireAsyncCall();
and here

[aandrey, 2013-12-17 13:07:48]

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (left):

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:128:
ASSERT(!m_currentAsyncCallChain);
On 2013/12/17 13:01:59, yurys wrote:
> Why did you remove this assert? Can it fail in a wrong place?

We can receive nested willHandleEvent() callbacks, thus this ASSERT is not valid
there. I'm not sure about willFireTimer() though.

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

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:718: {
On 2013/12/17 13:01:59, yurys wrote:
>  if (m_asyncCallStackTracker.isEnabled()) ?

This check is inside the AsyncCallStackTracker method now. Do you suggest moving
them all into the debugger agent?

[aandrey, 2013-12-17 13:24:02]

PTAL

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (left):

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:128:
ASSERT(!m_currentAsyncCallChain);
On 2013/12/17 13:01:59, yurys wrote:
> Why did you remove this assert? Can it fail in a wrong place?

Done.

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

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:61: void add(EventTarget*
eventTarget, const AtomicString& eventType, const EventListenerAsyncCallChain&
item)
On 2013/12/17 13:01:59, yurys wrote:
> addEventListenerData?

Done.

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:106: friend class
AsyncCallStackTracker;
On 2013/12/17 13:01:59, yurys wrote:
> All these field should be public since ExecutionContextData is an
implementation
> detail of AsyncCallStackTracker

Done.

[yurys, 2013-12-18 09:20:10]

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

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:718: {
On 2013/12/17 13:07:48, aandrey wrote:
> On 2013/12/17 13:01:59, yurys wrote:
> >  if (m_asyncCallStackTracker.isEnabled()) ?
> 
> This check is inside the AsyncCallStackTracker method now. Do you suggest
moving
> them all into the debugger agent?
You should be consistent in placing this check either inside
AsyncCallStackTracker or outside like in 
InspectorDebuggerAgent::didInstallTimer. At the moment it is a mix.

[yurys, 2013-12-18 09:22:14]

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (left):

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:128:
ASSERT(!m_currentAsyncCallChain);
On 2013/12/17 13:07:48, aandrey wrote:
> On 2013/12/17 13:01:59, yurys wrote:
> > Why did you remove this assert? Can it fail in a wrong place?
> 
> We can receive nested willHandleEvent() callbacks, thus this ASSERT is not
valid
> there. I'm not sure about willFireTimer() though.

I agree that it make sense for willHandleEvent which can be nested but how can
we get willFireTimer or willFireAnimationFrame nested in another event?

[aandrey, 2013-12-18 09:32:08]

PTAL

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (left):

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:128:
ASSERT(!m_currentAsyncCallChain);
On 2013/12/18 09:22:15, yurys wrote:
> On 2013/12/17 13:07:48, aandrey wrote:
> > On 2013/12/17 13:01:59, yurys wrote:
> > > Why did you remove this assert? Can it fail in a wrong place?
> > 
> > We can receive nested willHandleEvent() callbacks, thus this ASSERT is not
> valid
> > there. I'm not sure about willFireTimer() though.
> 
> I agree that it make sense for willHandleEvent which can be nested but how can
> we get willFireTimer or willFireAnimationFrame nested in another event?

I think they can not. Added those ASSERT's back.

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

https://codereview.chromium.org/114033002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorDebuggerAgent.cpp:718: {
On 2013/12/18 09:20:11, yurys wrote:
> On 2013/12/17 13:07:48, aandrey wrote:
> > On 2013/12/17 13:01:59, yurys wrote:
> > >  if (m_asyncCallStackTracker.isEnabled()) ?
> > 
> > This check is inside the AsyncCallStackTracker method now. Do you suggest
> moving
> > them all into the debugger agent?
> You should be consistent in placing this check either inside
> AsyncCallStackTracker or outside like in 
> InspectorDebuggerAgent::didInstallTimer. At the moment it is a mix.

Done. Moved them all to the Agent.

[yurys, 2013-12-18 09:57:53]

Cool, LGTM!

[aandrey, 2013-12-18 10:10:35]

Committed patchset #7 manually as r164080 (presubmit successful).