DevTools: Async call stacks for window.postMessage()

DevTools: Async call stacks for window.postMessage()

This introduces a more general instrumentation approach for tracing async calls
that could also be an entry point for public Debug API.

This patch also allows showing debugger async call stacks across the execution
contexts.

BUG=393175, 272416
R=pfeldman, yurys

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

[yurys, 2014-07-21 12:45:32]

https://codereview.chromium.org/404953003/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptFunctionCall.cpp (right):

https://codereview.chromium.org/404953003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptFunctionCall.cpp:48: appendArgument();
Looks like it should be done in a separate patch.

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:131: m_asyncOperationId =
InspectorInstrumentation::traceAsyncOperationStarting(executionContext(),
"postMessage");
If we are OK with storing async id in the instrumented instance, are you
planning to switch other async stacks instrumentation to the new API?

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:146: InspectorInstrumentationCookie cookie
=
InspectorInstrumentation::traceAsyncOperationCompletedCallbackStarting(executionContext(),
m_asyncOperationId);
Why do you need to merge two hooks into one?

[aandrey, 2014-07-22 11:08:43]

ptal

https://codereview.chromium.org/404953003/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptFunctionCall.cpp (right):

https://codereview.chromium.org/404953003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptFunctionCall.cpp:48: appendArgument();
On 2014/07/21 12:45:32, yurys wrote:
> Looks like it should be done in a separate patch.

Done. Extracted to http://crrev.com/412443002

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:131: m_asyncOperationId =
InspectorInstrumentation::traceAsyncOperationStarting(executionContext(),
"postMessage");
On 2014/07/21 12:45:32, yurys wrote:
> If we are OK with storing async id in the instrumented instance, are you
> planning to switch other async stacks instrumentation to the new API?

Yes, something like ExecutionContextTask/FileSystemAPI should be easily
switchable, but for others like setTimeout/requestAnimFrame, we  would still
need to have a place to match timeoutId to asyncOperationId, so it makes no
sense to switch.

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:146: InspectorInstrumentationCookie cookie
=
InspectorInstrumentation::traceAsyncOperationCompletedCallbackStarting(executionContext(),
m_asyncOperationId);
On 2014/07/21 12:45:32, yurys wrote:
> Why do you need to merge two hooks into one?

To have only one instrumentation call here. Actually this "combined" method is
the most useful, but should we want to expose it in public API we should
separate it to be flexible.

[yurys, 2014-07-22 13:09:44]

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:146: InspectorInstrumentationCookie cookie
=
InspectorInstrumentation::traceAsyncOperationCompletedCallbackStarting(executionContext(),
m_asyncOperationId);
Why is it marked as completed only here? Shouldn't it be done where the
PostMessageTimer is created and scheduled?

[aandrey, 2014-07-22 13:32:15]

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/404953003/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:146: InspectorInstrumentationCookie cookie
=
InspectorInstrumentation::traceAsyncOperationCompletedCallbackStarting(executionContext(),
m_asyncOperationId);
On 2014/07/22 13:09:43, yurys wrote:
> Why is it marked as completed only here? Shouldn't it be done where the
> PostMessageTimer is created and scheduled?

No, it's on the right place.
The async operation "completed" means that there will be no more callbacks, and
we can delete the async stack trace associated with the operation. Until this
place we do need that async stack and cannot say the operation is "completed"
before this place.

The traceAsyncOperationCompletedCallbackStarting actually does:
1) traceAsyncCallbackStarting    => we save the async stack as "current"
2) traceAsyncOperationCompleted  => we delete the async stack

[yurys, 2014-07-22 13:51:43]

lgtm

[aandrey, 2014-07-22 14:41:15]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-07-22 14:41:25]

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

[commit-bot: I haz the power, 2014-07-22 15:47:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-22 16:54:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 16:54:37]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[aandrey, 2014-07-22 20:13:48]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-07-22 20:14:29]

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

[commit-bot: I haz the power, 2014-07-22 20:51:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-22 21:39:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 21:39:55]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[aandrey, 2014-07-22 21:48:48]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-07-22 21:49:30]

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

[commit-bot: I haz the power, 2014-07-22 21:58:14]

Change committed as 178698