[inspector] introduced Debugger.scheduleStepIntoAsync

[inspector] introduced Debugger.scheduleStepIntoAsync

This method could be called on pause and will do stepInto next scheduled callback if any will happen until next break.
First implementation support only callbacks chained by Promise.prototype.then.

BUG=chromium:432469
R=yangguo@chromium.org,dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2723273002
Cr-Commit-Position: refs/heads/master@{#43616}
Committed: https://chromium.googlesource.com/v8/v8/+/47276d3db3dab6b4f1dd2ddb8b8088ccc0d0673e

[kozy, 2017-03-01 21:02:49]

Dmitry and Yang, please take a look.

This method will be used with 'green dots' and types of break locations to
experimenting with solution for async stepping.

And, as a little bonus, inspector tests powered by async await, JS proxies and
everything else achieve synergy effect and finally you can just read through
test without needing to jump around :)

[dgozman, 2017-03-01 23:03:58]

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/js_protoc...
src/inspector/js_protocol.json:580: "description": "Steps into next scheduled
async task if any is scheduled before next pause.",
Let's describe when this one returns.

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:765: return;
return;
return;

just to be sure ;-)

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:631: int id, int parentId, bool breakable) {
Do we really need this?

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:638: if ((breakable && type ==
v8::debug::kDebugPromiseCreated && parentPtr) ||
Let's move checks inside of stepping method.

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:670: void
V8Debugger::steppingOnPromiseEvent(v8::Local<v8::Context> context,
handleAsyncTaskStepping

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:685: agent->stepIntoAsyncWasScheduled();
if (agent->shouldBreakInScheduledAsyncTask())
  m_taskWithScheduledBreak = task;

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:692: } else {
DCHECK(type == didHandle)

https://codereview.chromium.org/2723273002/diff/40001/test/inspector/debugger...
File test/inspector/debugger/schedule-step-into-async.js (right):

https://codereview.chromium.org/2723273002/diff/40001/test/inspector/debugger...
test/inspector/debugger/schedule-step-into-async.js:57: await
Protocol.Debugger.stepOver();
Should not await stepping most of the time.

[kozy, 2017-03-02 00:53:04]

all done. please take another look!

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/js_protoc...
src/inspector/js_protocol.json:580: "description": "Steps into next scheduled
async task if any is scheduled before next pause.",
On 2017/03/01 23:03:57, dgozman wrote:
> Let's describe when this one returns.

Done.

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:765: return;
On 2017/03/01 23:03:57, dgozman wrote:
> return;
> return;
> 
> just to be sure ;-)

Done.

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:631: int id, int parentId, bool breakable) {
On 2017/03/01 23:03:57, dgozman wrote:
> Do we really need this?

Yes, but we could make it better and called created_by_user - is true when
promise is created in user not blackboxed code.

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:638: if ((breakable && type ==
v8::debug::kDebugPromiseCreated && parentPtr) ||
On 2017/03/01 23:03:57, dgozman wrote:
> Let's move checks inside of stepping method.

Done.

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:670: void
V8Debugger::steppingOnPromiseEvent(v8::Local<v8::Context> context,
On 2017/03/01 23:03:57, dgozman wrote:
> handleAsyncTaskStepping

Done.

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:685: agent->stepIntoAsyncWasScheduled();
On 2017/03/01 23:03:57, dgozman wrote:
> if (agent->shouldBreakInScheduledAsyncTask())
>   m_taskWithScheduledBreak = task;

Done.

https://codereview.chromium.org/2723273002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:692: } else {
On 2017/03/01 23:03:57, dgozman wrote:
> DCHECK(type == didHandle)

Done.

https://codereview.chromium.org/2723273002/diff/40001/test/inspector/debugger...
File test/inspector/debugger/schedule-step-into-async.js (right):

https://codereview.chromium.org/2723273002/diff/40001/test/inspector/debugger...
test/inspector/debugger/schedule-step-into-async.js:57: await
Protocol.Debugger.stepOver();
On 2017/03/01 23:03:57, dgozman wrote:
> Should not await stepping most of the time.

Done.

[dgozman, 2017-03-03 19:47:06]

Mostly lgtm. The only interesting question is at line
src/inspector/v8-debugger.cc:691

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug-interfa...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug-interfa...
src/debug/debug-interface.h:107: bool HasNonBlackboxedFrameOnStack(Isolate*
isolate);
nit: while we are here, let's rename this one as well?

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:2004: // We need to skip top frame which contains
instrumentation.
Looks a bit sketchy, as it heavily relies on the implementation of Promise.all
and Promise.race. Not sure how to make it better though.

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug.h#newco...
src/debug/debug.h:354: bool HasNonBlackboxedFrameOnStack();
Let's have AllFramesOnStackAreBlackboxed() - less negatives is usually easier to
understand.

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/js_protoc...
src/inspector/js_protocol.json:582: "description": "Steps into next scheduled
async task if any is scheduled before next pause. Returns success when async
task is actually scheduled, returns error if no task was executed or another
scheduleStepIntoAsync was called.",
... if no tasks were scheduled ...

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1265: Response::Error("No async task was
scheduled."));
No async tasks were scheduled before pause.

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:685: }
return;

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:689: } else if (isScheduledTask) {
return;

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:691: agent->cancelPauseOnNextStatement();
Would that interfere with stepping? Should we even cancel here?

https://codereview.chromium.org/2723273002/diff/80001/test/inspector/debugger...
File test/inspector/debugger/schedule-step-into-async.js (right):

https://codereview.chromium.org/2723273002/diff/80001/test/inspector/debugger...
test/inspector/debugger/schedule-step-into-async.js:13: function testSimple() {
Do we test for a couple step-ins before the task is actually triggered?

var p = Promise.resolve();
debugger;       // scheduleAsyncStepIn, stepIn
var a = 1;      // stepIn
p.then(v => v)  // stepIn goes into callback.

[kozy, 2017-03-03 20:17:02]

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:2004: // We need to skip top frame which contains
instrumentation.
On 2017/03/03 19:47:06, dgozman wrote:
> Looks a bit sketchy, as it heavily relies on the implementation of Promise.all
> and Promise.race. Not sure how to make it better though.

We skip not a Promise.all or Promise.race here, we skip .then call and it looks
fine to me. It's rely only on idea that promise created instrumentation placed
inside of .then implementation.

[kozy, 2017-03-03 23:14:02]

all done, Yang, please take another look.

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug-interfa...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug-interfa...
src/debug/debug-interface.h:107: bool HasNonBlackboxedFrameOnStack(Isolate*
isolate);
On 2017/03/03 19:47:06, dgozman wrote:
> nit: while we are here, let's rename this one as well?

Done.

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2723273002/diff/80001/src/debug/debug.h#newco...
src/debug/debug.h:354: bool HasNonBlackboxedFrameOnStack();
On 2017/03/03 19:47:06, dgozman wrote:
> Let's have AllFramesOnStackAreBlackboxed() - less negatives is usually easier
to
> understand.

Done.

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/js_protoc...
src/inspector/js_protocol.json:582: "description": "Steps into next scheduled
async task if any is scheduled before next pause. Returns success when async
task is actually scheduled, returns error if no task was executed or another
scheduleStepIntoAsync was called.",
On 2017/03/03 19:47:06, dgozman wrote:
> ... if no tasks were scheduled ...

Done.

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:1265: Response::Error("No async task was
scheduled."));
On 2017/03/03 19:47:06, dgozman wrote:
> No async tasks were scheduled before pause.

Done.

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:685: }
On 2017/03/03 19:47:06, dgozman wrote:
> return;

Done.

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:689: } else if (isScheduledTask) {
On 2017/03/03 19:47:06, dgozman wrote:
> return;

Done.

https://codereview.chromium.org/2723273002/diff/80001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:691: agent->cancelPauseOnNextStatement();
On 2017/03/03 19:47:06, dgozman wrote:
> Would that interfere with stepping? Should we even cancel here?

Should not interfere with traditional stepping and we're ready if it interferes
with native breakpoints which use scheduled/cancel API too.

https://codereview.chromium.org/2723273002/diff/80001/test/inspector/debugger...
File test/inspector/debugger/schedule-step-into-async.js (right):

https://codereview.chromium.org/2723273002/diff/80001/test/inspector/debugger...
test/inspector/debugger/schedule-step-into-async.js:13: function testSimple() {
On 2017/03/03 19:47:06, dgozman wrote:
> Do we test for a couple step-ins before the task is actually triggered?
> 
> var p = Promise.resolve();
> debugger;       // scheduleAsyncStepIn, stepIn
> var a = 1;      // stepIn
> p.then(v => v)  // stepIn goes into callback.

But if I scheduledAsyncStepIn at debugger statement and make stepIn - execution
will be paused in var a = 1; and no async task would be scheduled in between -
it means that scheduleAsyncStepIn will return an error.

[kozy, 2017-03-03 23:14:08]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 23:14:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 23:35:23]

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

[commit-bot: I haz the power, 2017-03-03 23:35:24]

Dry run: Try jobs failed on following builders:
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_linux_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng_...)

[kozy, 2017-03-03 23:48:52]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 23:48:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 00:13:26]

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

[commit-bot: I haz the power, 2017-03-04 00:13:26]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-03-06 14:53:07]

non-inspector part LGTM.

[kozy, 2017-03-06 16:01:58]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-03-06 16:01:58]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2723273002/#ps120001
(title: "fixed tests")

[commit-bot: I haz the power, 2017-03-06 16:02:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 16:28:27]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488816118000410,
"parent_rev": "e96b5ed423bd6c566c1b11d54303c29cdfd473c7", "commit_rev":
"47276d3db3dab6b4f1dd2ddb8b8088ccc0d0673e"}

[commit-bot: I haz the power, 2017-03-06 16:28:33]

Description was changed from

==========
[inspector] introduced Debugger.scheduleStepIntoAsync

This method could be called on pause and will do stepInto next scheduled
callback if any will happen until next break.
First implementation support only callbacks chained by Promise.prototype.then.

BUG=chromium:432469
R=yangguo@chromium.org,dgozman@chromium.org
==========

to

==========
[inspector] introduced Debugger.scheduleStepIntoAsync

This method could be called on pause and will do stepInto next scheduled
callback if any will happen until next break.
First implementation support only callbacks chained by Promise.prototype.then.

BUG=chromium:432469
R=yangguo@chromium.org,dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2723273002
Cr-Commit-Position: refs/heads/master@{#43616}
Committed:
https://chromium.googlesource.com/v8/v8/+/47276d3db3dab6b4f1dd2ddb8b8088ccc0d...
==========

[commit-bot: I haz the power, 2017-03-06 16:28:34]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/47276d3db3dab6b4f1dd2ddb8b8088ccc0d...