Rework error handling in the service protocol and in Observatory.

Rework error handling in the service protocol and in Observatory.

In JSON RPC, error results and regular results are distinguished structurally.  We now do this in the service protocol instead of returning a regular result with the "Error" type.

VM:

- Add PrintError method to JSONStream.  Use this to generate all errors in the service protocol.

- Fix some bugs with "limit" parameter handling.

- Change return value for heap rpcs when they find a collected/expired reference.

- Update tests.

Observatory:

- Add app.handleExceptions -- this is a widely-used exception handler that puts up a notification when there is an uncaught/unexpected exception.  Added a top-level zone catch that uses this handler, as well as explicit uses of this handler at various points in the code.  Most pages use this implicitly or explictly, except for the debugger page, which displays exceptions in the console.

[ General rule, when a function returns a Future, generally let the error pass through.  If a function does async work but doesn't return the Future, then it should handle errors explicitly by calling app.handleExceptions, etc. ]

- Fixed notifications.  They were generally broken.  Some hard-coded type names needed to be fixed, some hacks needed to be removed.

- Add a new class RpcException and a variety of subclasses which capture the error cases when making an rpc.

- Redo refresh buttons so their callbacks return Futures instead of taking a "done" closure.  Cleaner.  Allows us to move error handling outside of the callback.

- Drop ServiceError, ServiceException and the associated views.

- Redo notifications so that event and exception notifications are supported.

- No longer null out the vm when it is disconnected.

- Remove notifications when we visit the vm-connect page.  Only show the "Proceeding will lose current page" dialog when we aren't on the vm-connect page already.

- Improve error message on cpu profile page when profiling is disabled on the VM.

- Allow variables to wrap on debugger page.

- Make eval-box and eval-link tolerant of errors during rpc.  Make the errors display nicely.

- When a breakpoint is marked "not possible", make sure it disappears.  This got broken during script view refactor.

- Drop vm's errors and exceptions streams.  No longer needed.

R=johnmccutchan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=45792

[turnidge, 2015-05-12 19:05:23]

turnidge@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2015-05-13 17:50:10]

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/app/application.dart (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/app/application.dart:79: bool
isPauseEvent(ServiceEvent event) {
this method should be on event.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/eval_link.dart (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/eval_link.dart:32: busy = false;
is this busy = false; necessary? won't the whenComplete handle this?

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/nav_bar.html (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/nav_bar.html:323: event.eventType ==
'PauseInterrupted' ||
Can you move this logic outside of the template? i.e. event.isPauseEvent() We
shouldn't have these string constants inside html where the analyzer can't help
us.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/script_inset.dart (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/script_inset.dart:497: return new
Future.error(e, st);
Maybe cleaner as:

if (e is ServiceRpcException && se.code == ...) {
  return new Future.value(null);
}

return new Future.error(e, st);

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/service_error_view.html (left):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/service_error_view.html:1: <link
rel="import" href="../../../../packages/polymer/polymer.html">
update observatory_sources.gypi

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/service_exception_view.dart (left):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/service_exception_view.dart:1: // Copyright
(c) 2013, the Dart project authors.  Please see the AUTHORS file
observatory_sources.gypi

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/service_exception_view.html (left):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/service_exception_view.html:1: <link
rel="import" href="../../../../packages/polymer/polymer.html">
observatory_sources.gypi

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/te...
File runtime/observatory/tests/service/bad_web_socket_address_test.dart (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/te...
runtime/observatory/tests/service/bad_web_socket_address_test.dart:19:
Logger.root.onRecord.listen((LogRecord rec) {
Should this log setup code be inside test_helper?

https://codereview.chromium.org/1120133002/diff/120001/runtime/vm/json_stream.cc
File runtime/vm/json_stream.cc (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/vm/json_stream...
runtime/vm/json_stream.cc:138: char* buffer =
isolate->current_zone()->Alloc<char>(len + 1);
zone->PrintToString ?

https://codereview.chromium.org/1120133002/diff/120001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/vm/service.cc#...
runtime/vm/service.cc:1149: PrintSentinel(js, "objects/expired", "<expired>");
Should PrintSentinel take an enum and handle setting the id and preview fields?
Would put all of the constant strings printed below into one place.

[turnidge, 2015-05-14 17:53:44]

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/app/application.dart (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/app/application.dart:79: bool
isPauseEvent(ServiceEvent event) {
On 2015/05/13 17:50:09, Cutch wrote:
> this method should be on event.

Done.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/eval_link.dart (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/eval_link.dart:32: busy = false;
On 2015/05/13 17:50:09, Cutch wrote:
> is this busy = false; necessary? won't the whenComplete handle this?

Done.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/nav_bar.html (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/nav_bar.html:323: event.eventType ==
'PauseInterrupted' ||
On 2015/05/13 17:50:09, Cutch wrote:
> Can you move this logic outside of the template? i.e. event.isPauseEvent() We
> shouldn't have these string constants inside html where the analyzer can't
help
> us.

Done.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/script_inset.dart (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/script_inset.dart:497: return new
Future.error(e, st);
On 2015/05/13 17:50:09, Cutch wrote:
> Maybe cleaner as:
> 
> if (e is ServiceRpcException && se.code == ...) {
>   return new Future.value(null);
> }
> 
> return new Future.error(e, st);

Rewritten.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/service_error_view.html (left):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/service_error_view.html:1: <link
rel="import" href="../../../../packages/polymer/polymer.html">
On 2015/05/13 17:50:09, Cutch wrote:
> update observatory_sources.gypi

Done.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/service_exception_view.dart (left):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/service_exception_view.dart:1: // Copyright
(c) 2013, the Dart project authors.  Please see the AUTHORS file
On 2015/05/13 17:50:09, Cutch wrote:
> observatory_sources.gypi

Done.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/service_exception_view.html (left):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/li...
runtime/observatory/lib/src/elements/service_exception_view.html:1: <link
rel="import" href="../../../../packages/polymer/polymer.html">
On 2015/05/13 17:50:09, Cutch wrote:
> observatory_sources.gypi

Done.

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/te...
File runtime/observatory/tests/service/bad_web_socket_address_test.dart (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/observatory/te...
runtime/observatory/tests/service/bad_web_socket_address_test.dart:19:
Logger.root.onRecord.listen((LogRecord rec) {
On 2015/05/13 17:50:09, Cutch wrote:
> Should this log setup code be inside test_helper?

This test doesn't use test_helper.

https://codereview.chromium.org/1120133002/diff/120001/runtime/vm/json_stream.cc
File runtime/vm/json_stream.cc (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/vm/json_stream...
runtime/vm/json_stream.cc:138: char* buffer =
isolate->current_zone()->Alloc<char>(len + 1);
On 2015/05/13 17:50:10, Cutch wrote:
> zone->PrintToString ?

We would need to add a new version of zone->PrintToString that took va_list --
zone->VPrintToString or something.

https://codereview.chromium.org/1120133002/diff/120001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1120133002/diff/120001/runtime/vm/service.cc#...
runtime/vm/service.cc:1149: PrintSentinel(js, "objects/expired", "<expired>");
On 2015/05/13 17:50:10, Cutch wrote:
> Should PrintSentinel take an enum and handle setting the id and preview
fields?
> Would put all of the constant strings printed below into one place.

Done.

[Cutch, 2015-05-14 18:29:04]

lgtm

[turnidge, 2015-05-14 19:35:28]

Committed patchset #9 (id:160001) manually as 45792 (presubmit successful).