Improvements to causal async stack traces

Improvements to causal async stack traces

UX improvements:

- [x] Stop printing the suffix <%s_async_body> and <%s_async_gen_body> for the generated closures.

Fixes #28743

- [x] Don't include the duplicate frame below the asynchronous suspension marker.

Fixes #28742

Bug fixes:

- [x] Fix service protocol enum naming so that it is consistent with other enums.

Fixes #28726

Misc:

- [x] Stop using package:stack_trace now that the VM does it for us.

BUG=
R=asiva@google.com, devoncarew@google.com, rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/8538d81473d81e46d0bc8ea82607581ebff65390

[Cutch, 2017-02-10 21:10:39]

Description was changed from

==========
Improvements to causal async stack traces

UX improvements:

- [x] Stop printing the suffix <%s_async_body> and <%s_async_gen_body> for the
generated closures.

Fixes #28743

- [x] Don't include the duplicate frame below the asynchronous suspension
marker.

Fixes #28742

Bug fixes:

- [x] Fix service protocol enum naming so that it is consistent with other
enums.

Fixes #28726

Misc:

- [x] Stop using package:stack_trace now that the VM does it for us.

BUG=
==========

to

==========
Improvements to causal async stack traces

UX improvements:

- [x] Stop printing the suffix <%s_async_body> and <%s_async_gen_body> for the
generated closures.

Fixes #28743

- [x] Don't include the duplicate frame below the asynchronous suspension
marker.

Fixes #28742

Bug fixes:

- [x] Fix service protocol enum naming so that it is consistent with other
enums.

Fixes #28726

Misc:

- [x] Stop using package:stack_trace now that the VM does it for us.

BUG=
==========

[Cutch, 2017-02-10 21:10:39]

johnmccutchan@google.com changed reviewers:
+ asiva@google.com, devoncarew@google.com, rmacnak@google.com

[devoncarew, 2017-02-10 21:15:13]

awesome! (lgtm for the enum name changes)

[rmacnak, 2017-02-10 21:27:14]

lgtm w/ qualified name test

https://codereview.chromium.org/2690683002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2690683002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7070: if (!fun.IsAsyncGenerator() &&
!fun.IsAsyncFunction()) {
Do this for a sync generator as well?

https://codereview.chromium.org/2690683002/diff/1/tests/language/vm/causal_as...
File tests/language/vm/causal_async_exception_stack_test.dart (right):

https://codereview.chromium.org/2690683002/diff/1/tests/language/vm/causal_as...
tests/language/vm/causal_async_exception_stack_test.dart:30:
expect(st.toString(), stringContainsInOrder([
Since we're futzing with qualified names, add also a test that includes a local
function in the stack. Something like

main() {
  inner() async {
     await thrower();
  }
  inner();
}

should give

thrower
<gap>
main.inner
<gap>
main

[Cutch, 2017-02-11 00:02:40]

https://codereview.chromium.org/2690683002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2690683002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7070: if (!fun.IsAsyncGenerator() &&
!fun.IsAsyncFunction()) {
On 2017/02/10 21:27:14, rmacnak wrote:
> Do this for a sync generator as well?

Done.

https://codereview.chromium.org/2690683002/diff/1/tests/language/vm/causal_as...
File tests/language/vm/causal_async_exception_stack_test.dart (right):

https://codereview.chromium.org/2690683002/diff/1/tests/language/vm/causal_as...
tests/language/vm/causal_async_exception_stack_test.dart:30:
expect(st.toString(), stringContainsInOrder([
On 2017/02/10 21:27:14, rmacnak wrote:
> Since we're futzing with qualified names, add also a test that includes a
local
> function in the stack. Something like
> 
> main() {
>   inner() async {
>      await thrower();
>   }
>   inner();
> }
> 
> should give
> 
> thrower
> <gap>
> main.inner
> <gap>
> main
> 

Done.

[siva, 2017-02-11 00:24:33]

lgtm

https://codereview.chromium.org/2690683002/diff/20001/tests/language/vm/causa...
File tests/language/vm/causal_async_exception_stack_test.dart (right):

https://codereview.chromium.org/2690683002/diff/20001/tests/language/vm/causa...
tests/language/vm/causal_async_exception_stack_test.dart:38: ]));
might be interesting to add
      try {
         await thrower();
      } catch(e, st) {
         expect(st.toString(), stringContainsInOrder([
           'thrower',
           '<asynchronous suspension>',
           'main',
         ]));
      }

This checks that the linking works and we don't retain any of the
previous links (foo, generator etc.)

[rmacnak, 2017-02-11 00:29:38]

Per discussion with Siva, some interesting test cases. Not directly related to
this CL and fine to include in a later CL. I expect these already pass or we'd
be suffering heavy memory leaks.

foo3() async => throw "foo"
bar3() async => throw "bar"

foo2() async => foo3()
bar2() async => bar3()

foo() async => foo2()
bar() async => bar2()

test1() async {
  // main -> foo -> foo2 -> foo3
  // main -> bar -> bar2 -> bar3
  // These run interleaved, check their stack traces don't become mixed.
  var a = foo();
  var b = bar();

  try { await a; } catch(e, st) { st has foo1,2,3 but not bar1,2,3 }
  try { await b; } catch(e, st) { st has bar1,2,3 but not foo1,2,3 }
}

test2() async {
  // main -> foo -> foo2 -> foo3
  // main -> bar -> bar2 -> bar3
  // These run sequentially, check the former stack trace didn't get linked to
from the latter stack trace.

  try { await foo(); } catch(e, st) { st has foo1,2,3 but not bar1,2,3 }
  try { await bar(); } catch(e, st) { st has bar1,2,3 but not foo1,2,3 }
}

[Cutch, 2017-02-11 01:14:28]

Test cases added. PTAL.

https://codereview.chromium.org/2690683002/diff/20001/tests/language/vm/causa...
File tests/language/vm/causal_async_exception_stack_test.dart (right):

https://codereview.chromium.org/2690683002/diff/20001/tests/language/vm/causa...
tests/language/vm/causal_async_exception_stack_test.dart:38: ]));
On 2017/02/11 00:24:33, siva wrote:
> might be interesting to add
>       try {
>          await thrower();
>       } catch(e, st) {
>          expect(st.toString(), stringContainsInOrder([
>            'thrower',
>            '<asynchronous suspension>',
>            'main',
>          ]));
>       }
> 
> This checks that the linking works and we don't retain any of the
> previous links (foo, generator etc.)

Done.

[rmacnak, 2017-02-11 01:22:29]

still LGTM

[siva, 2017-02-11 01:24:33]

still LGTM

[Cutch, 2017-02-14 08:57:03]

Description was changed from

==========
Improvements to causal async stack traces

UX improvements:

- [x] Stop printing the suffix <%s_async_body> and <%s_async_gen_body> for the
generated closures.

Fixes #28743

- [x] Don't include the duplicate frame below the asynchronous suspension
marker.

Fixes #28742

Bug fixes:

- [x] Fix service protocol enum naming so that it is consistent with other
enums.

Fixes #28726

Misc:

- [x] Stop using package:stack_trace now that the VM does it for us.

BUG=
==========

to

==========
Improvements to causal async stack traces

UX improvements:

- [x] Stop printing the suffix <%s_async_body> and <%s_async_gen_body> for the
generated closures.

Fixes #28743

- [x] Don't include the duplicate frame below the asynchronous suspension
marker.

Fixes #28742

Bug fixes:

- [x] Fix service protocol enum naming so that it is consistent with other
enums.

Fixes #28726

Misc:

- [x] Stop using package:stack_trace now that the VM does it for us.

BUG=
R=asiva@google.com, devoncarew@google.com, rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/8538d81473d81e46d0bc8ea82607581ebff65390
==========

[Cutch, 2017-02-14 08:57:04]

Committed patchset #4 (id:60001) manually as
8538d81473d81e46d0bc8ea82607581ebff65390 (presubmit successful).