DevTools: Support XHR async call stacks in the debugger.

DevTools: Support XHR async call stacks in the debugger.

With this we will collect XHR async call stacks at the point of
calling XHR.send() rather than at XHR.addEventListener() or at
assigning XHR.onreadystatechange.

For XMLHttpRequestUpload event target callbacks show async call stacks
of the owner XHR object.

Drive-by: Do strict counting of the nested willHandleEvent callbacks.

BUG=272416
R=yurys@chromium.org

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

[aandrey, 2013-12-23 06:00:13]

ping?

[yurys, 2013-12-23 09:11:57]

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

https://codereview.chromium.org/118293002/diff/40001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:88: maxAsyncCallStackDepth =
maxAsyncCallStackDepth || 4;
Please make it a required parameter for better readability and less implicit
assumptions.

https://codereview.chromium.org/118293002/diff/40001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:108:
InspectorTest.beginResultBufferring();
I must have overlooked this begin/endResultBuffering in one of the previous
patches sorry about that, but it seems to be adding unnecessary complexity to
the test framework. Can you change the utility code so that it accepted explicit
output buffer it it needs one or return accumulated output as function result
and didn't require changing basic output behavior?

https://codereview.chromium.org/118293002/diff/40001/LayoutTests/inspector/de...
File LayoutTests/inspector/debugger/async-callstack-xhrs.html (right):

https://codereview.chromium.org/118293002/diff/40001/LayoutTests/inspector/de...
LayoutTests/inspector/debugger/async-callstack-xhrs.html:23: var count =
sendXHR.__count = (sendXHR.__count || 0) + 1;
Why not simply put count into the global scope, or at least use single leading
underscore: _count. Also you can init it when defining as sendXHR._count = 0 and
simplify this expression to  var count = ++sendXHR._count;

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

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:120: static XMLHttpRequest*
xmlHttpRequestFor(EventTarget* eventTarget)
toXmlHttpRequest ?

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:203:
setCurrentAsyncCallChain(0);
Can we test this branch?

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:298: DEFINE_STATIC_LOCAL(String,
xhrSendName, ("XMLHttpRequest.send"));
This is not thread-safe and may be called on Worker's thread.

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:313: if (xhr == eventTarget &&
eventType == EventTypeNames::loadend)
What is xhr == eventTarget? Is it check for upload vs download?

[aandrey, 2013-12-23 12:09:37]

PTAL

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

https://codereview.chromium.org/118293002/diff/40001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:88: maxAsyncCallStackDepth =
maxAsyncCallStackDepth || 4;
On 2013/12/23 09:11:57, yurys wrote:
> Please make it a required parameter for better readability and less implicit
> assumptions.

Done.

https://codereview.chromium.org/118293002/diff/40001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:108:
InspectorTest.beginResultBufferring();
On 2013/12/23 09:11:57, yurys wrote:
> I must have overlooked this begin/endResultBuffering in one of the previous
> patches sorry about that, but it seems to be adding unnecessary complexity to
> the test framework. Can you change the utility code so that it accepted
explicit
> output buffer it it needs one or return accumulated output as function result
> and didn't require changing basic output behavior?

Done.

https://codereview.chromium.org/118293002/diff/40001/LayoutTests/inspector/de...
File LayoutTests/inspector/debugger/async-callstack-xhrs.html (right):

https://codereview.chromium.org/118293002/diff/40001/LayoutTests/inspector/de...
LayoutTests/inspector/debugger/async-callstack-xhrs.html:23: var count =
sendXHR.__count = (sendXHR.__count || 0) + 1;
On 2013/12/23 09:11:57, yurys wrote:
> Why not simply put count into the global scope, or at least use single leading
> underscore: _count. Also you can init it when defining as sendXHR._count = 0
and
> simplify this expression to  var count = ++sendXHR._count;

Done.

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

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:120: static XMLHttpRequest*
xmlHttpRequestFor(EventTarget* eventTarget)
On 2013/12/23 09:11:57, yurys wrote:
> toXmlHttpRequest ?

Done.

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:203:
setCurrentAsyncCallChain(0);
On 2013/12/23 09:11:57, yurys wrote:
> Can we test this branch?

Added new test: LayoutTests/inspector/debugger/async-callstack-middle-run.html

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:298: DEFINE_STATIC_LOCAL(String,
xhrSendName, ("XMLHttpRequest.send"));
On 2013/12/23 09:11:57, yurys wrote:
> This is not thread-safe and may be called on Worker's thread.

Done. I also removed other DEFINE_STATIC_LOCAL's

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:313: if (xhr == eventTarget &&
eventType == EventTypeNames::loadend)
On 2013/12/23 09:11:57, yurys wrote:
> What is xhr == eventTarget? Is it check for upload vs download?

Yes, essentially.
The "loadend" on XHR is the corresponding "did" event to willLoadXHR. We should
store XHR data between the two.

[yurys, 2013-12-23 12:40:30]

LGTM,

please add to the description note that we need to show call stack at xhr.send
vs. xhr.onreadystatechange listener setup.

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

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:203:
setCurrentAsyncCallChain(0);
On 2013/12/23 12:09:38, aandrey wrote:
> On 2013/12/23 09:11:57, yurys wrote:
> > Can we test this branch?
> 
> Added new test: LayoutTests/inspector/debugger/async-callstack-middle-run.html

Thank you!

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:313: if (xhr == eventTarget &&
eventType == EventTypeNames::loadend)
On 2013/12/23 12:09:38, aandrey wrote:
> On 2013/12/23 09:11:57, yurys wrote:
> > What is xhr == eventTarget? Is it check for upload vs download?
> 
> Yes, essentially.
> The "loadend" on XHR is the corresponding "did" event to willLoadXHR. We
should
> store XHR data between the two.

Consider assigning it to a named variable like isUpload = (xhr != eventTarget);
for better readability.

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

https://codereview.chromium.org/118293002/diff/90001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:160:
InspectorTest.captureStackTraceIntoString = function(callFrames,
asyncStackTrace, options)
Consider making it private.

[aandrey, 2013-12-23 13:01:28]

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

https://codereview.chromium.org/118293002/diff/40001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:313: if (xhr == eventTarget &&
eventType == EventTypeNames::loadend)
On 2013/12/23 12:40:31, yurys wrote:
> On 2013/12/23 12:09:38, aandrey wrote:
> > On 2013/12/23 09:11:57, yurys wrote:
> > > What is xhr == eventTarget? Is it check for upload vs download?
> > 
> > Yes, essentially.
> > The "loadend" on XHR is the corresponding "did" event to willLoadXHR. We
> should
> > store XHR data between the two.
> 
> Consider assigning it to a named variable like isUpload = (xhr !=
eventTarget);
> for better readability.

Assigned to isXHRDownload.

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

https://codereview.chromium.org/118293002/diff/90001/LayoutTests/http/tests/i...
LayoutTests/http/tests/inspector/debugger-test.js:160:
InspectorTest.captureStackTraceIntoString = function(callFrames,
asyncStackTrace, options)
On 2013/12/23 12:40:31, yurys wrote:
> Consider making it private.

I call it directly from the new test.

[commit-bot: I haz the power, 2013-12-23 13:01:41]

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

[commit-bot: I haz the power, 2013-12-23 13:59:15]

Change committed as 164291