Add an error listener on isolates.

Add an error listener on isolates.

While any error listener is attached to an isolate, uncaught errors are reported to the listener through his supplied sendPort.

The current format for the error is
  [error.toString(), stack == null ? null : stack.toString()]
This is wrapped on the receiving side.

The isolate exposes the errors as a broadcast stream of error events.

R=floitsch@google.com

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

[floitsch, 2014-02-17 14:21:39]

I like where this is going, but I don't necessarily agree with the semantics
yet:
I think that registering an error-handler should be separate from making errors
fatal.

However, maybe we could make the errors-getter a combination of multiple calls:
- setErrorsAreFatal(false).
- registerShutdown() -> linked to stream.close()
- registerError() -> linked to stream-events

This would make the errors-getter a convenient shortcut, while still allowing
more distinct operations.

https://codereview.chromium.org/169703002/diff/1/sdk/lib/_internal/lib/isolat...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/169703002/diff/1/sdk/lib/_internal/lib/isolat...
sdk/lib/_internal/lib/isolate_helper.dart:274: final Capability
inspectCapability = new Capability();
Getting error-messages should not require full inspection capabilities.
Just errorCapability?

[Lasse Reichstein Nielsen, 2014-02-18 09:48:19]

On 2014/02/17 14:21:39, floitsch wrote:
> I like where this is going, but I don't necessarily agree with the semantics
> yet:
> I think that registering an error-handler should be separate from making
errors
> fatal.
> 
> However, maybe we could make the errors-getter a combination of multiple
calls:
> - setErrorsAreFatal(false).
> - registerShutdown() -> linked to stream.close()
> - registerError() -> linked to stream-events

I believe this was a suggestion from the VM team, when Søren talked to them
while over there.
This way, an uncaught error is fatal, but a remotely reported error isn't
considered uncaught.
This matches what we do with errors in other async cases - the error isn't the
problem, it is only a problem if we don't handle it.

The registerError would then make errors not fatal.
The registerShutdown would get a message containing the fatal error, if there is
one, null if termination was peaceful.

I like the model.

> This would make the errors-getter a convenient shortcut, while still allowing
> more distinct operations.

This would reduce the number of operations and keep it consistent with other
error handling.

[Lasse Reichstein Nielsen, 2014-02-18 09:48:57]

https://codereview.chromium.org/169703002/diff/1/sdk/lib/_internal/lib/isolat...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/169703002/diff/1/sdk/lib/_internal/lib/isolat...
sdk/lib/_internal/lib/isolate_helper.dart:274: final Capability
inspectCapability = new Capability();
What is "full inspection capabilities"? Mirrors?
I expect to have three capabilities:
- inspect (onError, ping/isalive, onShutdown)
- pause (pause, resume)
- terminate (kill)
If we want mirrors on a remote isolate, we should have a fourth capability, but
I would call that "nirrorCapability".

I could rename "inspect" to "listen" since it is more like listening on events
from the isolate than it is actively looking at it.

[Lasse Reichstein Nielsen, 2014-05-13 07:57:35]

PTAL

[Lasse Reichstein Nielsen, 2014-05-13 09:24:40]

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:419: print("[[$errorMsg,
($url:$line${column == null ? "" : ":$column"})
$error(${error.runtimeType})]]");
Whoops, debug print.

[floitsch, 2014-05-13 12:47:45]

Fyi.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:383: // If no listeners have been
registered, consider the error unhandled.
I don't think this is true anymore.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:392: if (JS("var", "#.onerror",
globalThis) == null) {
Isn't there a better way, if something else already installed a handler?

We could:
- have a static bool hasInstalledHandler, to know if we already installed it,
and then either
  * overwrite the old one.
  * provide an error message.
  * try to run both in some order.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:394:
convertDartClosureToJS(catchUncaughtError, 5));
what's the "5" for?

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:417: void catchUncaughtError(String
errorMsg, String url, int line,
Should be a bool-function.
Return false, if error should propagate. true otherwise.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:419: print("[[$errorMsg,
($url:$line${column == null ? "" : ":$column"})
$error(${error.runtimeType})]]");
long line.
debug-print?

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:680: print(isolate.delayedEvents);
debug print?

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:684: try {
You could simplify by always letting the error-through, and just decide in the
error-handler if the error is fatal.

[Lasse Reichstein Nielsen, 2014-05-13 13:02:07]

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:392: if (JS("var", "#.onerror",
globalThis) == null) {
On 2014/05/13 12:47:46, floitsch wrote:
> Isn't there a better way, if something else already installed a handler?

There is:
  window.addEventListener("error", func, true)
Initial testing found it to be not as effective as onerror in some browsers, but
I'll check it again.

> 
> We could:
> - have a static bool hasInstalledHandler, to know if we already installed it,
> and then either
>   * overwrite the old one.
>   * provide an error message.
>   * try to run both in some order.

There should only ever be one function installed by this code.
It might be simpler to have a boolean than it is to test the previous state
against the current state to see if it should be installed.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:394:
convertDartClosureToJS(catchUncaughtError, 5));
Number of arguments passed to the Dart function from the JS wrapper.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:417: void catchUncaughtError(String
errorMsg, String url, int line,
Not sure that works for onerror, but I'll check it.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:680: print(isolate.delayedEvents);
Indeed.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:684: try {
True. This place guarantees a good stack trace, which the onerror event doesn't
always give, so if possible I try to catch it here.

I'll check if it really makes a difference.

[Lasse Reichstein Nielsen, 2014-05-15 11:46:42]

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:392: if (JS("var", "#.onerror",
globalThis) == null) {
The problem was Firefox, where the event based approach gets no information at
all (not even the message). 
For now, we'll have to use onerror in order to get any useful information about
the error.

https://codereview.chromium.org/169703002/diff/60001/sdk/lib/_internal/lib/is...
sdk/lib/_internal/lib/isolate_helper.dart:394:
convertDartClosureToJS(catchUncaughtError, 5));
I'll change the function to not use optional arguments.

[Lasse Reichstein Nielsen, 2014-05-15 11:46:52]

New implementation. PTAL again.

[floitsch, 2014-05-15 12:15:25]

LGTM although I'm concerned about the not having "break on uncaught error".

https://codereview.chromium.org/169703002/diff/100001/sdk/lib/_internal/lib/i...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/169703002/diff/100001/sdk/lib/_internal/lib/i...
sdk/lib/_internal/lib/isolate_helper.dart:275: /// Used to avoid considering the
isolate dead when if it has no open
"when if"

https://codereview.chromium.org/169703002/diff/100001/sdk/lib/_internal/lib/i...
sdk/lib/_internal/lib/isolate_helper.dart:406: print(error);
I would prefer using console.error when available.

https://codereview.chromium.org/169703002/diff/100001/sdk/lib/_internal/lib/i...
sdk/lib/_internal/lib/isolate_helper.dart:427: } catch (e, s) {
I'm afraid that this means that we won't have "break on uncaught exceptions"
anymore...

https://codereview.chromium.org/169703002/diff/100001/tests/isolate/handle_er...
File tests/isolate/handle_error2_test.dart (right):

https://codereview.chromium.org/169703002/diff/100001/tests/isolate/handle_er...
tests/isolate/handle_error2_test.dart:47: // Start paused so we have time to set
up the error handler.
Comment is at wrong place.

Add comment here that `spawn` makes errors non-fatal and establish bidirectional
communication.

https://codereview.chromium.org/169703002/diff/100001/tests/isolate/handle_er...
File tests/isolate/handle_error3_test.dart (right):

https://codereview.chromium.org/169703002/diff/100001/tests/isolate/handle_er...
tests/isolate/handle_error3_test.dart:48: // Start paused so we have time to set
up the error handler.
ditto (as in handle_error2)

[Lasse Reichstein Nielsen, 2014-05-19 10:55:23]

Committed patchset #7 manually as r36290 (presubmit successful).

[Lasse Reichstein Nielsen, 2014-05-19 11:13:36]

https://codereview.chromium.org/169703002/diff/100001/sdk/lib/_internal/lib/i...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/169703002/diff/100001/sdk/lib/_internal/lib/i...
sdk/lib/_internal/lib/isolate_helper.dart:406: print(error);
Done.

https://codereview.chromium.org/169703002/diff/100001/sdk/lib/_internal/lib/i...
sdk/lib/_internal/lib/isolate_helper.dart:427: } catch (e, s) {
True. 
That may have been one of the reasons for preferring onerror.
The problem is that we can't check before executing "code" whether errors are
fatal, since the code may be able to change that.

When we do synchronous sends to other isolates, throwing isn't an option anyway,
so this is the safest thing that works in all cases.
We could start counting how many isolates we are currently inside, and if only
one, do thrown there and catch it in onerror (and set some state so we know what
is happening in which isolate, and so we don't have to parse the crap that ends
up in onerror).

https://codereview.chromium.org/169703002/diff/100001/tests/isolate/handle_er...
File tests/isolate/handle_error2_test.dart (right):

https://codereview.chromium.org/169703002/diff/100001/tests/isolate/handle_er...
tests/isolate/handle_error2_test.dart:47: // Start paused so we have time to set
up the error handler.
Will do.