DevTools: Implement async call stacks for Promises.

DevTools: Implement async call stacks for Promises.

BUG=272416
R=yurys, vsevik, yusukesuzuki, yhirano

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

[yhirano, 2014-01-14 04:54:38]

Would you explain what InspectorInstrumentation::didPostPromiseTask is and when
we should call it?

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:265:
InspectorInstrumentation::didPostPromiseTask(context, this, true);
Why is the third parameter always true?

[aandrey, 2014-01-14 05:35:41]

On 2014/01/14 04:54:38, yhirano wrote:
> Would you explain what InspectorInstrumentation::didPostPromiseTask is and
when
> we should call it?

The didPostPromiseTask saves the current JS call stack that will be the async
stack when the corresponding async event is fired.
This async stack is "alive" between the corresponding willPerformPromiseTask and
didPerformPromiseTask calls of the same task.

In other words, the InspectorInstrumentation::didPostPromiseTask should be
called for every ExecutionContext::postTask(ExecutionContextTask*) call if this
task is related to Promise implementation.
The corresponding willPerformPromiseTask and didPerformPromiseTask should wrap
the ExecutionContextTask::performTask(ExecutionContext*).

[aandrey, 2014-01-14 05:35:52]

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:265:
InspectorInstrumentation::didPostPromiseTask(context, this, true);
On 2014/01/14 04:54:38, yhirano wrote:
> Why is the third parameter always true?

This indicates that the Promise originatorState is Fulfilled. It's always the
case for UpdateDerivedTask.

[yhirano, 2014-01-15 02:09:30]

1: For the following code,
 var resolve, reject;
 new Promise(function(res, rej) {  // (a)
    resolve = res;
    reject = rej;
 }).then(function(x) { // (b)
   return x + 1;
 }).then(function(x) { // (c)
   return x + 2;
 }).then(function(x) { // (d)
   console.log('x = ' + x);
   debugger;
 });
 resolve(3);

The async call stacks consist of (a) and (d) and don't contain (b) and (c).
Is it OK?

2: For the following code,
 var resolve, reject;
 new Promise(function(res, rej) {
    resolve = res;
    reject = rej;
 }).then(function(x) {
    return Promise.rejected(8);
 }).then(function(x) {
   console.log('resolved');
   debugger;
 }, function() {
   console.log('rejected');
   debugger; // (x)
 });
 resolve(3);

There is [Promise.fulfilled] at (x) on the async stacks and 'rejected' is
printed.
I think it is confusing, isn't it?

[yhirano, 2014-01-15 02:12:59]

On 2014/01/15 02:09:30, yhirano wrote:
> 1: For the following code,
>  var resolve, reject;
>  new Promise(function(res, rej) {  // (a)
>     resolve = res;
>     reject = rej;
>  }).then(function(x) { // (b)
>    return x + 1;
>  }).then(function(x) { // (c)
>    return x + 2;
>  }).then(function(x) { // (d)
>    console.log('x = ' + x);
>    debugger;
>  });
>  resolve(3);
> 
> The async call stacks consist of (a) and (d) and don't contain (b) and (c).
> Is it OK?
> 
> 2: For the following code,
>  var resolve, reject;
>  new Promise(function(res, rej) {
>     resolve = res;
>     reject = rej;
>  }).then(function(x) {
>     return Promise.rejected(8);
>  }).then(function(x) {
>    console.log('resolved');
>    debugger;
>  }, function() {
>    console.log('rejected');
>    debugger; // (x)
>  });
>  resolve(3);
> 
> There is [Promise.fulfilled] at (x) on the async stacks and 'rejected' is
> printed.
> I think it is confusing, isn't it?

Sorry, Promise.rejected(8) should be Promise.reject(8).

[aandrey, 2014-01-15 06:28:23]

On 2014/01/15 02:09:30, yhirano wrote:
> 1: For the following code,
>  var resolve, reject;
>  new Promise(function(res, rej) {  // (a)
>     resolve = res;
>     reject = rej;
>  }).then(function(x) { // (b)
>    return x + 1;
>  }).then(function(x) { // (c)
>    return x + 2;
>  }).then(function(x) { // (d)
>    console.log('x = ' + x);
>    debugger;
>  });
>  resolve(3);
> 
> The async call stacks consist of (a) and (d) and don't contain (b) and (c).
> Is it OK?

Yes, it is expected. How would you suggest we should improve the result?

I'd collect the stacks at return points of (b) and (c), but right now it's
difficult technically, and should be supported on the V8 side first (set
breakpoint on return point).

> 2: For the following code,
>  var resolve, reject;
>  new Promise(function(res, rej) {
>     resolve = res;
>     reject = rej;
>  }).then(function(x) {
>     return Promise.rejected(8);
>  }).then(function(x) {
>    console.log('resolved');
>    debugger;
>  }, function() {
>    console.log('rejected');
>    debugger; // (x)
>  });
>  resolve(3);
> 
> There is [Promise.fulfilled] at (x) on the async stacks and 'rejected' is
> printed.
> I think it is confusing, isn't it?

It is expected, since [Promise.fulfilled] at (x) points to the "resolve(3)".
We don't capture "return Promise.reject(8);" for the same reason as above.
What would you expect to see in this example?

[yhirano, 2014-01-15 13:28:54]

Thank you very much, I understand.

> I'd collect the stacks at return points of (b) and (c),
> but right now it's difficult technically, and should be
> supported on the V8 side first (set breakpoint on return
> point).
I agree.

https://codereview.chromium.org/131823003/diff/1/LayoutTests/inspector/debugg...
File LayoutTests/inspector/debugger/async-callstack-promises.html (right):

https://codereview.chromium.org/131823003/diff/1/LayoutTests/inspector/debugg...
LayoutTests/inspector/debugger/async-callstack-promises.html:25: function
settledPromise(value)
Let's use Promise.resolve and Promise.reject.

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:236:
TaskPerformScopeForInstrumentation performTaskScope(context, this);
Can you place this statement after L240?
It is good not to touch a stopped context.

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:281:
TaskPerformScopeForInstrumentation performTaskScope(context, this);
ditto as L236

[aandrey, 2014-01-15 14:49:52]

https://codereview.chromium.org/131823003/diff/1/LayoutTests/inspector/debugg...
File LayoutTests/inspector/debugger/async-callstack-promises.html (right):

https://codereview.chromium.org/131823003/diff/1/LayoutTests/inspector/debugg...
LayoutTests/inspector/debugger/async-callstack-promises.html:25: function
settledPromise(value)
On 2014/01/15 13:28:55, yhirano wrote:
> Let's use Promise.resolve and Promise.reject.

Added a separate test. The idea was to call those explicitly to check if they
get into the async stacks.

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/131823003/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:236:
TaskPerformScopeForInstrumentation performTaskScope(context, this);
On 2014/01/15 13:28:55, yhirano wrote:
> Can you place this statement after L240?
> It is good not to touch a stopped context.

Will be memory leak, as we need to clean up our internal instrumentation data.

We do not execute any of the ExecutionContext method. Instrumentation classes
will be accessed via: toDocument(context)->page()->inspectorController(), which
should be harmless even on stopped contexts.

[aandrey, 2014-01-15 16:23:18]

On 2014/01/15 13:28:54, yhirano wrote:
> Thank you very much, I understand.
> 
> > I'd collect the stacks at return points of (b) and (c),
> > but right now it's difficult technically, and should be
> > supported on the V8 side first (set breakpoint on return
> > point).
> I agree.

Although this implementation is helpless as is in the following case:

Promise.resolve("\"{}\"")
    .then(JSON.parse)
    .then(JSON.parse)
    .then(JSON.parse)
    .then(JSON.parse)
    .then(JSON.parse)
    .catch(function(e) {
        debugger;
    });

Stack trace:

    (anonymous function) (promise_sync:8)
    [Promise.Resolved]
    (anonymous function) (promise_sync:2)

No idea which JSON.parse call threw the exception.

Collecting stack traces on return points will not help, since DevTools does not
show call frames pointing to internal scripts.

[yhirano, 2014-01-16 04:10:12]

lgtm

https://codereview.chromium.org/131823003/diff/170001/LayoutTests/inspector/d...
File LayoutTests/inspector/debugger/async-callstack-promises.html (right):

https://codereview.chromium.org/131823003/diff/170001/LayoutTests/inspector/d...
LayoutTests/inspector/debugger/async-callstack-promises.html:136:
Promise.resolve(1).then(thenCallback, errorCallback)
missing semicolon;

https://codereview.chromium.org/131823003/diff/170001/LayoutTests/inspector/d...
LayoutTests/inspector/debugger/async-callstack-promises.html:137:
Promise.reject(Error("2")).then(thenCallback, errorCallback)
ditto

[yurys, 2014-01-16 12:49:20]

lgtm

https://codereview.chromium.org/131823003/diff/250001/LayoutTests/inspector/d...
File LayoutTests/inspector/debugger/async-callstack-promises.html (right):

https://codereview.chromium.org/131823003/diff/250001/LayoutTests/inspector/d...
LayoutTests/inspector/debugger/async-callstack-promises.html:48: var functions =
[doTestPromiseConstructor, doTestSettledPromises, doTestChainedPromises,
doTestPromiseAll, doTestThrowFromChain, doTestPromiseResolveAndReject];
Will these functions be always executed in this order?

[commit-bot: I haz the power, 2014-01-16 13:42:25]

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

[aandrey, 2014-01-16 14:15:27]

https://codereview.chromium.org/131823003/diff/250001/LayoutTests/inspector/d...
File LayoutTests/inspector/debugger/async-callstack-promises.html (right):

https://codereview.chromium.org/131823003/diff/250001/LayoutTests/inspector/d...
LayoutTests/inspector/debugger/async-callstack-promises.html:48: var functions =
[doTestPromiseConstructor, doTestSettledPromises, doTestChainedPromises,
doTestPromiseAll, doTestThrowFromChain, doTestPromiseResolveAndReject];
On 2014/01/16 12:49:20, yurys wrote:
> Will these functions be always executed in this order?

You mean the "debugger" breaks?
Answer: NO.
We sort the collected async stacks to avoid test flakiness.
Thus, the order in "functions" does not matter.

[commit-bot: I haz the power, 2014-01-16 14:54:26]

Change committed as 165226